Repository navigation
fix: percent-encode keys in local reference pointers - #62
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
URI-fragment pointer helpers src/ByteBard.AsyncAPI/JsonPointer.cs, test/ByteBard.AsyncAPI.Tests/JsonPointer_Should.cs |
Adds token escaping and URI-fragment serialization helpers. Tests cover percent encoding, pointer escaping, decoding, and round trips. |
Workspace key normalization src/ByteBard.AsyncAPI/AsyncApiWorkspace.cs, test/ByteBard.AsyncAPI.Tests/Models/AsyncApiReference_Should.cs |
Changes workspace registries to ordinal string keys. Valid URI-fragment pointers are normalized before registration, containment checks, and reference resolution. Adds a test for distinct fragments on absolute references to the same resource. |
V2 reference construction and resolution src/ByteBard.AsyncAPI.Readers/V2/*Deserializer.cs, test/ByteBard.AsyncAPI.Tests/Models/PercentEncodedReferenceKeys_Should.cs |
V2 readers escape reference keys for URI fragments and decode final tokens when matching channels and messages. Tests cover encoded keys, reference resolution, round trips, and V2/V3 conversions. |
Priority:
Estimated code review effort: 3 (Moderate) | ~22 minutes
Change: Bug fix · Severity of issue fixed: Low
Possibly related PRs
- ByteBardOrg/AsyncAPI.NET#60: Adds JSON Pointer escaping for local keys in the workspace and V2 reader paths, which this change extends with URI-fragment percent encoding.
- ByteBardOrg/AsyncAPI.NET#57: Adds local V2 channel server references; this change updates matching and workspace registration for percent-encoded server keys.
- ByteBardOrg/AsyncAPI.NET#56: Adds the local JSON Pointer parser and URI-fragment parsing that this change extends with escaping and serialization.
Merge Risk: 
5d438
-
No actionable issue is established for this change. It is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 9 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "fix: percent-encode keys in local reference pointers" matches the main change. The PR adds URI-fragment escaping helpers, switches workspace registrations and 2.x references to escaped toke… Description check The description follows the template. It has an About section with a detailed explanation of the problem and the fix, a Changes section, and a Tests section. It references the linked issue (Fixes #61)…Linked Issues check Issue #61is the only active direct link. Its coding requirements are met at the reviewed head, based on the whole-PR diff and the head file read.
1. Add the reverse of FromUriFragment (ToUriFragment…Out of Scope Changes check No demonstrated unrelated change was found. Each changed area connects to #61or to the mechanism#61requires.
- JsonPointer helpers (EscapeUriFragmentToken, ToUriFragment): required by#61.
- Async…Full details: Title check
-
Explanation
The title "fix: percent-encode keys in local reference pointers" matches the main change. The PR adds URI-fragment escaping helpers, switches workspace registrations and 2.x references to escaped tokens, and fixes percent-encoded reference keys that failed to round-trip. The title is short, specific, and follows a conventional-commit style. It omits the registry keying change and the absolute-URI fix, which are secondary.
Full details: Description check
-
Explanation
The description follows the template. It has an About section with a detailed explanation of the problem and the fix, a Changes section, and a Tests section. It references the linked issue (Fixes
#61), which satisfies the Related Issues part of the template. The description does not use the template's Changelog section with Add/Remove entries, so that part of the template is missing.
Full details: Linked Issues check
-
Explanation
Issue
#61is the only active direct link. Its coding requirements are met at the reviewed head, based on the whole-PR diff and the head file read.-
Add the reverse of FromUriFragment (ToUriFragment): met. src/ByteBard.AsyncAPI/JsonPointer.cs at head adds EscapeUriFragmentToken (EscapeToken, then Uri.EscapeDataString) and ToUriFragment (per-token "/" + EscapeUriFragmentToken).
-
Normalize the fragment of every location into one form in the workspace: met. AsyncApiWorkspace.cs replaces ToLocationUrl with static NormalizeLocation. It rewrites the fragment through TryFromUriFragment and ToUriFragment. RegisterComponent, Contains and ResolveReference all call it. The registries are now string-keyed, so encoded and unencoded references (
#/servers/ab and#/servers/a%20b) map to one entry. -
Build workspace registrations with the same helper instead of EscapeToken: met. The whole-PR diff shows every component, channel, server and message registration location using EscapeUriFragmentToken.
-
Build the 2.x reader's references with the same helper: met per the whole-PR change summary. AsyncApiChannelDeserializer (GetServerReferenceKey), AsyncApiDocumentDeserializer (reassigned channel references, message tokens), AsyncApiOperationDeserializer (LoadMessages) and AsyncApiSecurityRequirementDeserializer use EscapeUriFragmentToken. The reader source diffs were not read directly. The 2.x reader matches channels and messages by the decoded last pointer token (GetLastReferenceToken).
-
Tests for a key containing % and a key containing a space, through 2.x to 2.x, 3.x to 3.x, 2.x to 3.x and 3.x to 2.x: met per the change summary. PercentEncodedReferenceKeys_Should contains V2_RoundTrips, V3_RoundTrips, V3_DowngradesToV2 and V2_UpgradesToV3. Its key pair is 'prod eu' and 'prod%20eu'. The test bodies were not read. The author reports 8 new tests fail without the fix and the 503-test suite passes. These results were not reproduced.
Valid-document behavior is unchanged for keys that match the spec key pattern. Such keys contain only unreserved characters, so EscapeUriFragmentToken returns the same text as EscapeToken.
Evidence limits: the diff for JsonPointer.cs was unavailable in the checkout, so the helper code was read from the head file. The 2.x reader and test file contents were assessed from the supplied change summary.
-
Full details: Out of Scope Changes check
-
Explanation
No demonstrated unrelated change was found. Each changed area connects to
#61or to the mechanism#61requires.- JsonPointer helpers (EscapeUriFragmentToken, ToUriFragment): required by
#61. - AsyncApiWorkspace string-keyed registries and NormalizeLocation:
#61asks that every location fragment be normalized through ToLocationUrl. Storing the normalized fragment needs string keys, because Uri equality ignores the fragment of absolute URIs. The absolute-URI fix is therefore a consequence of the same change, and the PR states it as a related bug. - 2.x reader changes (channel and message key matching by decoded last token, escaped references): needed so that encoded keys round-trip through 2.x.
#61asks for 2.x references to use the same helper. - Tests (PercentEncodedReferenceKeys_Should, the absolute-URI test, JsonPointer_Should): support the requirements in
#61and the absolute-URI fix above.
One behavior change goes beyond
#61and is disclosed in the PR description. External resource locations are now compared as written. Scheme and host case no longer normalize, so HTTP://Example.com/s.json and http://example.com/s.json are distinct. This follows from the string-key design and is not a requested outcome. Reviewers should decide whether this change is acceptable. - JsonPointer helpers (EscapeUriFragmentToken, ToUriFragment): required by
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
The workspace registered keys escaped with only "~0" / "~1", while names recovered from references were percent-decoded (RFC 6901 section 6). Keys containing "%" or characters that are percent-encoded in a URI fragment did not round-trip: - A 2.x server named "a%20b" was written back as "a b", and upgraded to a 3.x "#/servers/a%20b" reference that points at a server named "a b". - A 3.x "$ref: '#/servers/a%20b'" missed the workspace entry for "a b" and was resolved from the raw document into a separate copy of the server. The workspace now normalizes every location's fragment into one percent-encoded form, so encoded and unencoded references find the same entry, and registrations and the 2.x reader's references are built with JsonPointer.EscapeUriFragmentToken. Registry keys are now compared as strings instead of Uris. Uri equality ignores the fragment of absolute URIs, so "https://x/s.json#/A" and "https://x/s.json#/B" shared an entry and the second reference silently resolved to the first schema. Fixes ByteBardOrg#61 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AsyncApiChannelReference.SerializeV2 writes a reference to a root channel with the channel's address, escaped with EscapeToken only. A channel addressed "a%20b" was written as "#/channels/a%20b", which decodes to "a b" and no longer resolves. It now uses EscapeUriFragmentToken, as does the 2.x reader when it points such a reference at the normalized channel key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d438c6 to
e0a976b
Compare
|
Thanks, both done:
|
Fixes #61.
Since #60, the workspace registered keys escaped with only
~0/~1, while names recovered from references go throughJsonPointer.FromUriFragment, which percent-decodes (RFC 6901 §6). Keys containing%or characters that are percent-encoded in a URI fragment did not round-trip:a%20bwas written back to 2.x asa b, with no error. Upgrading to 3.x produced$ref: '#/servers/a%20b', which points at a server nameda b.$ref: '#/servers/a%20b'missed the workspace entry fora band was resolved from the raw document into a separate copy, so changes todocument.Servers["a b"]didn't reach it.While investigating, I found a related bug that also affects valid documents. The registry was keyed by
Uri, andUriequality ignores the fragment of absolute URIs. Sohttps://example.com/s.json#/Aandhttps://example.com/s.json#/Bshared an entry, and withResolveAllReferencesthe second reference silently resolved to the first schema.Changes
JsonPointer.EscapeUriFragmentToken(token)andJsonPointer.ToUriFragment()are added as the reverse ofFromUriFragment. They escape each token with~0/~1, then applyUri.EscapeDataString.AsyncApiWorkspacekeys its registries by string. Each location's fragment is normalized into that one percent-encoded form, so encoded and unencoded references (#/servers/a b,#/servers/a%20b) find the same entry, and fragments of absolute URIs are compared.EscapeUriFragmentTokeninstead ofEscapeToken.Only keys with characters that get percent-encoded are affected. Keys that match the spec's key patterns produce the same references as before. One side effect: the resource part of an external location is now compared as written, so
HTTP://Example.com/s.jsonandhttp://example.com/s.jsonare no longer treated as the same resource.Tests
PercentEncodedReferenceKeys_Shouldcovers a key with a space (prod eu) next to a key that is literallyprod%20eu, through 2.x→2.x, 3.x→3.x, 2.x→3.x and 3.x→2.x, in the style ofEscapedReferenceKeys_Should. It also checks that encoded and unencoded 3.x server references resolve to the root server rather than a copy.AsyncApiReference_Should.AbsoluteExternalReferences_ToTheSameResource_ResolveTheirOwnFragmentscovers the absolute URI case.JsonPointer_Shouldhas unit tests for the new helpers.8 of the new tests fail without the fix. The full suite (503 tests) passes on net8.0, net9.0 and net10.0.
🤖 Generated with Claude Code
Summary by CodeRabbit