Percent-encode JSON pointer reference tokens in JsonSchemaExporter $ref values

[Copilot]

JsonSchemaExporter emitted $ref pointers that applied only the RFC 6901 ~0/~1 escaping but did not percent-encode characters illegal in a URI fragment. A property named hello%20world produced #/.../hello%20world instead of #/.../hello%2520world, yielding schemas that fail $ref resolution.

Per RFC 6901 §6, a JSON Pointer used as a URI fragment (as $ref is) must be UTF-8 encoded with all characters outside the RFC 3986 fragment production percent-encoded — in addition to the existing ~0/~1 token escaping. Since ~, 0, 1 are unreserved, the pointer escapes remain intact.

Changes

• JsonSchemaExporter.FormatJsonPointer: reference tokens now percent-encode any character not permitted unescaped in a URI fragment (e.g. %→%25, ^→%5E, space→%20), emitting UTF-8 octets and handling surrogate pairs. ~0/~1 escaping is preserved.
  • Switched the local ValueStringBuilder from a using declaration to try/finally so it can be passed by ref to the new AppendEscapedReferenceToken / AppendPercentEncoded helpers.
• Tests: extended ClassWithJsonPointerEscapablePropertyNames — property name ~/path/to/value → ~/path/to/value%20^, with the expected $ref becoming #/properties/~0~1path~1to~1value%2520%5E/properties/Next, covering both the pointer escaping and the new percent-encoding.

Example

[JsonPropertyName("hello%20world")]
public required Dictionary<string, string[]> SpecialChars { get; set; }

// before: "$ref": "#/anyOf/0/properties/hello%20world/additionalProperties"   // invalid
// after:  "$ref": "#/anyOf/0/properties/hello%2520world/additionalProperties" // valid

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

[jeffhandley]

Note

This review was generated by the holistic code review workflow being iterated on as part of #130339. It was produced by GitHub Copilot (Claude Opus 4.8) and independently cross-checked with two additional models (gpt-5.3-codex, gemini-3.1-pro-preview). Please treat its findings as assistive input for human review.

Holistic Review

Motivation: Justified. Issue #130162 is a real, reproducible bug with a validator link showing that a $ref containing a literal % (property hello%20world) currently emits #/.../hello%20world, which fails JSON Schema $ref resolution because a consumer decodes %20 back to a space. Per RFC 6901 §6, a JSON Pointer embedded in a URI fragment must percent-encode characters outside the RFC 3986 fragment production.

Approach: Sound. Applying the RFC 6901 ~0/~1 token escaping first and then Uri.EscapeDataString is the correct order and the right built-in (the earlier hand-rolled UTF-8 encoder was correctly dropped during review). ~ is unreserved, so existing pointer escapes survive untouched, and % → %25 fixes the reported bug. The round-trip holds: the raw property name is the schema key, and percent-decode → ~1/~0 unescape returns to that key.

Summary: ⚠️ Needs Changes. The source change is correct and all three review models agree it fixes the bug without regressing the existing ~/path/to/value test. But the earlier test addition was reverted (commit 7dac552), leaving the new percent-encoding behavior with no regression coverage, and the PR description is now stale (it still describes removed helpers and a test change that is not in the diff). Add a regression test and refresh the description before merge; minor notes below are for the author/maintainer to weigh.

---

Detailed Findings

✅ Correctness — fix is right and round-trips (corroborated by both cross-check models)

The two-step order is correct: RFC 6901 (~ → ~0, / → ~1) then Uri.EscapeDataString. Because ~ is unreserved in RFC 3986, ~0/~1 survive percent-encoding intact — confirmed empirically: Uri.EscapeDataString("~0~1path~1to~1value") returns the string unchanged, so the existing ClassWithJsonPointerEscapablePropertyNames test is not regressed. The reported case round-trips: hello%20world → hello%2520world, which a compliant consumer percent-decodes back to hello%20world and resolves against the raw property key. The two Replace calls are also correctly ordered (~ before /), so ~1 is never double-escaped into ~01.

❌ Test coverage — the new behavior is untested

The current diff changes only the source file; the test that was added earlier was reverted, so nothing exercises the actual fix. The existing ~/path/to/value test passes both before and after this change (every character is unreserved after ~0/~1 escaping), so it does not cover percent-encoding at all. This is a behavioral bug fix and needs a regression test to lock it in and stop a future refactor from silently reintroducing the bug. Rather than mutate the existing ClassWithJsonPointerEscapablePropertyNames expectation (which you asked to keep intact), add a separate test type whose property name requires fragment encoding — e.g. the issue's own repro hello%20world (expecting hello%2520world), plus ideally a space/caret and a non-ASCII char such as ö (%C3%B6) — and assert the emitted $ref.

⚠️ PR description is stale / inaccurate

The description still documents the abandoned approach: hand-rolled AppendEscapedReferenceToken / AppendPercentEncoded helpers, a try/finally ValueStringBuilder refactor, and an extended ClassWithJsonPointerEscapablePropertyNames test emitting #/properties/~0~1path~1to~1value%2520%5E/properties/Next. None of that is in the current diff, which uses Uri.EscapeDataString and reverts the test. Please update the description to match the shipped change so reviewers — and the eventual squash commit message — are accurate.

💡 Over-escaping is valid but broader than the fragment grammar requires

Uri.EscapeDataString encodes per the RFC 3986 unreserved set, so it also escapes characters that are legal in a fragment (:, @, and sub-delims !$&'()*+,;=), e.g. a:b@c → a%3Ab%40c. This is spec-valid — RFC 6901 §6 requires consumers to percent-decode before pointer evaluation, so a compliant validator resolves it fine — and it is the pragmatic BCL choice (there is no built-in "escape for URI fragment" helper). Worth being aware that it encodes more aggressively than strictly necessary and leans on consumers doing correct fragment decoding; not a blocker.

💡 Allocation & down-level notes (for maintainer sign-off)

• The previous span-based implementation was allocation-free. The new code allocates only when a segment actually contains special characters — string.Replace (no match) and Uri.EscapeDataString (nothing to escape) both return the original instance — so the common case stays cheap, and this is a cached, non-hot schema-export path. Given STJ's strict zero-allocation conventions, a maintainer may still want to explicitly bless the tradeoff (a zero-alloc version would need a custom encoder for the netstandard2.0/net462 targets).
• Low-confidence: on net462, Uri.EscapeDataString historically throws UriFormatException for inputs beyond ~32,766 chars. The two cross-check models split on whether this is worth flagging; it is realistically unreachable here since segments are member / [JsonPropertyName] names, but noted for completeness.

[eiriktsarpalis]

@copilot what about testing that exercises the actual reported issue?

[Copilot]

@copilot what about testing that exercises the actual reported issue?

Added a regression test ClassWithPropertyNameRequiringFragmentEncoding using the issue's own repro property name hello%20world, asserting the emitted $ref is #/properties/hello%2520world/properties/Next (registered in the source-gen context too). I verified it genuinely exercises the reported bug: without the fix it fails, and the schema validator throws RefResolutionException : Could not resolve '...#/properties/hello world/properties/Next' (the %20 decoding back to a space); with the fix it passes. Reflection: 384 pass / 0 fail, source-gen: 383 pass / 0 fail. (a19a...)

Note

This reply was generated by GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates JsonSchemaExporter to emit $ref JSON Pointer fragments that are safe to embed in a URI fragment by adding percent-encoding on reference tokens (in addition to the existing RFC 6901 ~0 / ~1 escaping). It also extends the schema exporter test matrix to cover property names containing % that would otherwise break $ref resolution.

Changes:

• Update FormatJsonPointer to percent-encode reference tokens via Uri.EscapeDataString after RFC 6901 token escaping.
• Add a new test type with [JsonPropertyName("hello%20world")] and validate the resulting $ref contains %25-escaped %.
• Register the new test type in the source-gen JsonSerializerContext.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchemaExporter.cs	Percent-encodes JSON Pointer reference tokens used in $ref fragments.
src/libraries/System.Text.Json/tests/Common/JsonSchemaExporterTests.TestTypes.cs	Adds a regression test case for % in property names and a corresponding test type.
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/JsonSchemaExporterTests.cs	Adds the new test type to the source generation context.

[adamsitnik]

The changes look good, but I need to know first whether this can ever be on a hot path and whether we should care more about perf.