Repository navigation
Conversation
GROUP_ORDER was a module constant written unconditionally on both wire shapes, and `Subscribe` had no field for it, so no caller -- internal or public -- could ask for Ascending. The decode side already read the value and validated it, and then dropped it on the floor: a relay built on this parsed the subscriber's requested order correctly and could not act on it. Add `groupOrder` to `Subscribe` and its constructor options, defaulting to Descending, which is what every subscription sent before the field existed. Both encode paths write the field instead of the constant, and both decode paths pass the parsed value through. `GroupOrder.Default` (0x0) is the wire's "no preference". It is normalized to the client default in one place, the constructor, so the field a caller reads back is always a real order and 0 can never reach an encoder. That matters from draft-15, where GROUP_ORDER is a message parameter and Parameters rejects 0 as a protocol violation -- only draft-14's inline field can carry it at all, and that path is covered. Tests run on draft-14 and draft-20. The ascending case is a differential: it encodes the same message both ways and requires exactly one byte to differ, 0x02 -> 0x01, since a plain byte search would pass on requestId 1n alone. Each assertion is mutation-checked -- hardcoding either encode site, dropping the constructor normalization, and dropping the decode pass-through each turn a distinct test red.
Collaborator
|
Thank you @kkroo! Your commit is the first one in #5385, with your authorship kept. That PR finishes the job: a (Written by Claude Opus 5.5) |
Collaborator
|
Landed in #5385, which carries this work with your authorship and co-author credit. Thank you! (Written by Claude Opus 5.5) |
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
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.
Problem
GROUP_ORDER was a module constant written unconditionally on both wire shapes, and Subscribe had no field for it, so no caller could ask for Ascending. The decode side already read and validated the value, then dropped it.
Approach
Add groupOrder to Subscribe and its constructor options, defaulting to Descending, which preserves existing callers. Both encode paths write the selected field instead of the constant, and both decode paths pass the parsed value through.
GroupOrder.Default is normalized to the client default in the constructor, so the field a caller reads is always a real order and zero cannot reach an encoder. This matters from draft-15, where GROUP_ORDER is a message parameter and Parameters rejects zero as a protocol violation; the draft-14 inline path is covered too.
Impact
Alternatives
Leaving the constant and exposing a session-level default was rejected because group order is a per-subscription property on the wire, and a session-level knob cannot express mixed subscriptions.
Follow-ups
None.
Tests
Tests run on draft-14 and draft-20. The ascending case is differential: it encodes the same message both ways and requires exactly one byte to differ. Assertions cover both encode sites, constructor normalization, and decode pass-through.
Need help on this PR? Tag
@codesmith-botwith what you need.