Type the string-valued protocol enums, and restore the README helper index - #21
Merged
Conversation
Fields carrying ObsOutputState or ObsMediaInputAction were generated as strings, so every caller ran FromWireValue on the way in and ToWireValue on the way out. Which fields draw from an enum is not in the definition, which types them all as String, so the association is an explicit table like the numeric one. Both transports needed a converter: System.Text.Json can map the member names, but MessagePack's generator would have written the ordinal, which OBS rejects and which no round trip through this library alone would notice. Covered by tests that assert the bytes, not just the round trip. An unrecognised value maps to the enum's zero member rather than throwing, so a state added by a newer OBS does not fail the whole message. mediaState, monitorType, sceneItemBlendMode and inputKind carry fixed vocabularies too, but the protocol declares no enum for them, so they stay strings rather than ones this library would maintain by hand. Restores the helper index dropped in the README rewrite, folds the numbers and enums sections together now that neither needs conversion explained, and covers five more helpers in the example.
Moving the streams onto the groups left the events themselves flat, so half of observing an event was grouped and half was not: client.Scenes.SceneCreatedStream() beside client.SceneCreated. The handler is what most callers actually write, so the inconsistency was in the more used half. Events cannot go in a C# extension block, which is what the earlier reasoning stopped at. The groups are ordinary structs, and a struct can declare an event whose explicit accessors forward to the client. That detail matters: the property hands out a fresh struct on every access, so a field-like event would add the handler to a temporary and lose it. Tested for removal as well as addition, since a remove that quietly did nothing would leak every handler. The client keeps its events for the low-level path, and connection lifecycle events stay there because they belong to no category.
MSTest defaults to one worker per processor, and dotnet test runs every target framework at once, so the suite asked for several times the machine's parallelism. The tests that suffered were the ones asserting a timeout: their continuations wait on a saturated pool, so they failed on a busy machine rather than on a defect, and raising one test's budget only moved the failure to the next one.
ObsWebSocketRequestException carried both Status and StatusCode for the same thing. The record added nothing on a thrown exception: Result is false by definition, Comment is already on the exception, and Code is the number behind StatusCode, which casting to int still recovers. Only StatusCode remains. A request whose fields are all optional needed an empty record at the call site, so GetSceneListAsync(new(), ct) became GetSceneListAsync(ct). Requests offering a name or a uuid are excluded: the protocol marks both optional because either will do, but one of the pair has to be supplied, so a no argument call would fail at runtime rather than at compile time.
The rule behind them was wrong. "Every field is optional" does not mean omitting everything does anything useful: SetVideoSettings documents each field as not changed when omitted, so the overload was a write that silently did nothing, TriggerHotkeyByKeySequence pressed no key, and Sleep had no duration and is only valid inside a serial batch. A structural rule does derive the three harmless cases without a list, by requiring response fields as well, but it only reaches GetSceneList, GetInputList and GetInputKindList. Not worth a codegen branch, so those keep their empty record.
The parallel note said which execution type to avoid and stopped there. It now says what a caller who picks it anyway can still do: only the labelling is wrong, requestStatus and responseData come from the same object, so Raw plus GetData recovers every payload with its own status, and what is unknowable is which request produced which row. Verified against OBS by comparing the recovered set against the serial truth. Concurrent requests are documented as the answer when results have to be attributed, since the client multiplexes on request id. Adds a low level section for CallAsync, CallAsyncValue and a hand rolled batch, and three example checks covering all of it.
Reading a payload as the wrong record throws on JSON, so probing types works there, but MessagePack maps by key name and quietly returns an object with every unmatched property left at its default. A reading of cpu=0.00 is indistinguishable from a real one, so a heterogeneous parallel batch cannot be sorted out afterwards on that transport.
Reading a payload as a record it did not come from was silent. On MessagePack the format maps by key name, so every unmatched property was left at its default and a fabricated reading of cpu=0.00 looked genuine. JSON was no better for the 42 of 72 response records that have no required member for it to miss. Codegen emits the field names OBS sends per record, and GetData checks a payload carries at least one of them before deserializing. Response records almost never share field names, so this catches the mistake on both transports and they now fail alike. It rejects rather than identifies: partial overlap still passes, and the five shapes shared by more than one record cannot be told apart. Those records are field for field identical, so reading one as another gives the right values, and a test pins that so the check cannot regress it.
Every read of a response went through a "!" or a "?.", 73 of them in the example alone. The cause was that the protocol never marks a response or event field optional: all 152 response fields and all 149 event fields carry a null valueOptional, so the generator fell back to the C# type and made every string and array nullable while value types became required. That split came from C#, not from OBS. The prose is where the protocol records a genuinely absent field, and it is the same signal that fixed GetMediaInputStatus. Strings and arrays are non-nullable unless the description says the field can be null. Response records go from 57% nullable to 16%, events to 7%, and every one that remains is either documented as null or a settings blob where nullable distinguishes absent from empty. The hand-written stubs had no protocol definition to derive from, so they were checked against the payloads a live OBS 32.2.2 actually returns. That found isGroup really is null for a non-group item, and that crops, alignments and indices arrive as integers rather than floats. Safe against older OBS: no field has ever been added to a request or event that already existed, so an older build never sends a partial payload for a request it supports, it rejects the request instead. A side effect worth naming: MessagePack does enforce required members, so both transports now reject a payload missing them. The earlier finding that MessagePack is never strict held only for records that had no required member to miss. Also fixes a README example that could not have worked: CallAsync request data must be a JsonElement or a type the serializer context knows, and an anonymous object throws at runtime. The compile check could not catch it because it only compiles. Renames ObsWebSocketClientHelpers to ObsWebSocketClientOperations, since it holds the client level operations rather than helpers.
Rereading it found three things that could not work. The grouped surface example awaited an IAsyncEnumerable, which does not compile. The errors example still read ex.Status?.Code, removed when Status collapsed into StatusCode. The example app section described a fraction of the suite it now runs. The low level examples are verified against OBS 32.2.2 rather than asserted: CallAsync with a reference type response, CallAsyncValue with none, CallAsyncValue with a JsonElement body, and the anonymous object that throws as documented. WaitForEventAsync threw the BCL TimeoutException while a request timeout threw ObsWebSocketTimeoutException, so catch (ObsWebSocketException) did not cover a wait that timed out, and the doc comment claimed it did. One timeout type now. sceneItemTransform is a concrete stub for an object OBS always sends rather than a settings bag, so it follows the other non-nullable fields, and the dead null assertions the analyzer flagged are gone. Four of those were narrowing genuinely nullable members and are restored.
An array whose item type the protocol does not state is generated as List<JsonElement>, and nothing in the resolver chain could build a formatter for one. GetCanvasList therefore could not be read at all on MessagePack while JSON read it fine, and the swallowed FormatterNotRegisteredException surfaced only as "OBS reported success but returned no payload". Found by covering the Canvases category in the example, which was the one category the validation run never touched. Also closes the gaps that audit turned up: the request shape with neither a payload nor a response, the generated nested request record, the monitor, output and transition stubs, the classic += handler on the validation client, and a check that sends one request six ways, including a hand built JsonElement body and the anonymous object that is refused.
The typed settings helpers have taken a JsonTypeInfo since v0.3, so a consumer can serialize a type this library does not model. CallAsync, CallAsyncValue and CallRequiredAsync did not, which left hand building a JsonElement as the only way to send an unmodelled payload, and made the low level path the one place a consumer context did not work. The optional parameter is a non generic JsonTypeInfo, so no call site gains a type argument and existing named calls are unaffected. Measured the alternatives before settling on this. Only two ways of producing a JsonElement are AOT safe: JsonDocument.Parse, and SerializeToElement with a JsonTypeInfo. SerializeToElement without one, and the JsonNode and JsonObject routes, all carry IL2026 and IL3050, so recommending them would have broken the AOT target. The example now sends one request seven ways and asserts all seven agree, the seventh being a consumer context.
The README gained a preferred way to send an unmodelled payload without a matching compile check, which is exactly how the two broken snippets this PR already fixed got in. Every API the README references now appears in the check: 27 code blocks, 28 distinct references, none unguarded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enums on the wire
ObsOutputStateandObsMediaInputActionhad a C# enum, but the fields carrying them were generated as strings, so every caller ranFromWireValuein andToWireValueout. The property is now the enum on both the read and the write side:Which fields draw from an enum is not recoverable from the definition, which types them all as
String, so the association is an explicit table like the numeric one.Both transports needed a converter. System.Text.Json could have mapped the member names, but MessagePack's generator would have written the ordinal, which OBS rejects and which no round trip through this library alone would notice.
WireEnumTestsasserts the bytes rather than the round trip.An unrecognised value maps to the enum's zero member rather than throwing, so a state added by a newer OBS does not fail the whole message.
mediaState,monitorType,sceneItemBlendModeandinputKindcarry fixed vocabularies too, but the protocol declares no enum for them, so they stay strings rather than ones this library would have to keep correct by hand.Docs
The README rewrite in #20 dropped the helper index that #12 added, which was a regression. It is back and corrected for the current surface. The separate "Typed protocol enums" and "Numbers" sections are folded into one "Protocol types" section, since with nothing left to convert the enum section was mostly explaining ceremony that no longer exists. It ends with how to drop to the wire, which is the part worth keeping.
Example
Five more helpers covered: preview scene switching,
SourceExistsAsync,SetInputMutesAsync(asserting a success beside a failure), transition settings, and the canvas screenshot helper.Two of those failed on first run. One was a bad assertion of mine: Fade legitimately reports no settings. The other looked like a library bug but was not: a targeted probe showed preview switching works correctly, and the fault was the check enabling Studio Mode and switching immediately, since OBS points Preview at Program after
StudioModeStateChanged. Both switches now wait for their own confirmation.Verification
240 tests, and both transports validated live against OBS 32.2.2 with 76 rows each and zero failures.