Skip to content

fix(docgen): honour System.Text.Json ignore conditions and property names - #12858

Merged
dipkakwani merged 3 commits into
masterfrom
fix/docgen-wire-shape
Aug 17, 2026
Merged

fix(docgen): honour System.Text.Json ignore conditions and property names#12858
dipkakwani merged 3 commits into
masterfrom
fix/docgen-wire-shape

Conversation

@dipkakwani

@dipkakwani dipkakwani commented Aug 17, 2026

Copy link
Copy Markdown
Member

Partially Fixes #12836 #12828

Changes

  • tools/DocGen looked up Newtonsoft's [JsonIgnore]/[JsonProperty], but the RPC layer has serialized with System.Text.Json. Both libraries declare a JsonIgnoreAttribute, so the lookups compiled, always returned null, and silently reported that nothing was ever hidden or renamed. Retargeted to System.Text.Json.Serialization.
  • Only JsonIgnoreCondition.Always hides a member; Never and the conditional forms still reach the wire and stay documented.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Documentation

Requires documentation update

  • [] Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

Partially addresses #12836 (fields renamed or omitted on the wire) and #12828 (internal CLR members leaking into the schema): [JsonIgnore]members are no longer published, and [JsonPropertyName] renames are now honoured. Both close once the generator also resolves [JsonConverter]-backed types, in a follow-up PR.

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review: fix(docgen): honour System.Text.Json ignore conditions and property names

  • Gather context (diff, DocGen sources, serializer options)
  • Analyze correctness / edge cases
  • Post review

Verdict: looks correct — no Critical/High/Medium findings. The diagnosis in the description checks out and I verified each claim against the tree.

What I verified

  • Newtonsoft lookups were indeed dead. No Newtonsoft reference remains anywhere in tools/DocGen/, Nethermind.JsonRpc, or Nethermind.Facade, so the old GetCustomAttribute<Newtonsoft.Json.JsonIgnoreAttribute>() / JsonPropertyAttribute calls could only ever return null. Dropping the using is safe — nothing else in the file used it.
  • JsonIgnoreCondition.Always is the right predicate. JsonIgnoreAttribute.Condition defaults to Always, so a bare [JsonIgnore] (e.g. Error.SuppressWarning, Nethermind.JsonRpc/Error.cs:19) is now correctly dropped from the docs.
  • Keeping Never / WhenWritingNull / WhenWritingDefault documented is correct and matters in practice — those forms are used widely (ReceiptForRpc, ParityTransaction, FeeHistoryResults, PeerInfo, EthCapabilities, …), and all of them can reach the wire. Never in particular is an opt-in override of the global DefaultIgnoreCondition = WhenWritingNull (EthereumJsonSerializer.cs:96), so hiding it would have been a real regression.
  • Null-safety of the new expression is fine: with no attribute, ?.Condition is null, and null is not JsonIgnoreCondition.Always → property kept.
  • [JsonPropertyName] renames now resolve (Error, SignTransactionResult, EthProtocolInfo, JsonRpcSubscriptionResponse, …), and STJ gives [JsonPropertyName] precedence over the naming policy, matching the fallback order here.

Findings

Low — camelCase fallback still diverges from the serializer's policy (JsonRpcGenerator.cs:356). Posted as an inline suggestion: delegate to JsonNamingPolicy.CamelCase.ConvertName(...), the same policy EthereumJsonSerializer uses, instead of the hand-rolled first-char lowercase. No current DTO is affected (YParity converts identically either way), so it's cosmetic today. Fix this →

Low — public fields are still invisible to the generator. EthereumJsonSerializer sets IncludeFields = true (EthereumJsonSerializer.cs:95), but GetSerializableProperties only walks GetProperties(...). No RPC result DTO currently exposes a public field, so there is no doc gap today — worth folding into the follow-up PR alongside the [JsonConverter] work.

Low — OrderBy(p => p.Name) sorts by CLR name, not the serialized name. Now that renames are honoured, a rename can put the docs list out of alphabetical order relative to what a reader sees. No current DTO hits this (e.g. Raw/Transactionraw/tx stays ordered).

Low — no regression test (repo rule: bug fixes get one). tools/DocGen has no test project, so a test would mean standing one up — reasonable to skip for a tools-only fix, but please eyeball the regenerated docs diff (members disappearing should only be the [JsonIgnore]/Always ones) before merging.

Notes

Nothing consensus-, security-, or hot-path-relevant here — this is a docs generator, not client runtime code. I was not able to run dotnet build tools/DocGen/DocGen.csproj in this environment (command not permitted), so compilation is unverified locally; given System.Text.Json.Serialization is in the BCL and no other Newtonsoft symbol is used in the file, I expect CI to be green.

View job run • branch fix/docgen-wire-shape

Comment thread tools/DocGen/JsonRpcGenerator.cs Outdated
@dipkakwani
dipkakwani merged commit 219d999 into master Aug 17, 2026
490 checks passed
@dipkakwani
dipkakwani deleted the fix/docgen-wire-shape branch August 17, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

documented response fields are renamed, nested, or omitted from the actual JSON-RPC payload

4 participants