fix(docgen): probe member converters to document their real JSON kind - #12869
fix(docgen): probe member converters to document their real JSON kind#12869hudem1 wants to merge 2 commits into
Conversation
|
Claude finished @hudem1's task in 4m 6s —— View job Review: docgen integer types
The diagnosis is correct and the fix is in the right place. I verified each claim against the serializer:
FindingsMedium — two raw-number converters are still unhandled ( Low — converter matching by Low — Low — no regression test Notes
Nothing here is a correctness or security risk in the client itself; the Medium is an incomplete-fix gap in the generated docs. |
| // A property may override the default numeric serialization with a converter that writes a raw JSON | ||
| // number (e.g. trace/debug block numbers) instead of the hex-quantity string implied by its CLR type. | ||
| private static string GetJsonTypeName(PropertyInfo prop) => | ||
| prop.GetCustomAttribute<JsonConverterAttribute>()?.ConverterType?.Name is "ULongRawJsonConverter" or "LongRawJsonConverter" | ||
| ? "_integer_" | ||
| : GetJsonTypeName(prop.PropertyType); |
There was a problem hiding this comment.
Medium — this covers only two of the four raw-number converters in the repo. NullableRawLongConverter / NullableRawULongConverter (src/Nethermind/Nethermind.Serialization.Json/NullableLongConvertercs.cs:16,39) also WriteNumberValue, and they are used on step in both BlockForRpc.cs:139 and BlockHeaderForRpc.cs:77 — both of which the generator expands (they are eth_getBlockBy* / AuRa results). Those fields will still be documented as _string_ (hex integer) while the wire value is a raw number, i.e. exactly the bug this PR fixes.
Second, matching converters by Name string literal is fragile: DocGen already project-references Nethermind.Runner (and uses Nethermind.JsonRpc.Modules types directly), so Nethermind.Serialization.Json types are reachable and a typeof comparison would be compile-checked. With no test project behind this tool, a rename/move would silently reintroduce wrong docs.
| // A property may override the default numeric serialization with a converter that writes a raw JSON | |
| // number (e.g. trace/debug block numbers) instead of the hex-quantity string implied by its CLR type. | |
| private static string GetJsonTypeName(PropertyInfo prop) => | |
| prop.GetCustomAttribute<JsonConverterAttribute>()?.ConverterType?.Name is "ULongRawJsonConverter" or "LongRawJsonConverter" | |
| ? "_integer_" | |
| : GetJsonTypeName(prop.PropertyType); | |
| // A property may override the default numeric serialization with a converter that writes a raw JSON | |
| // number (e.g. trace/debug block numbers) instead of the hex-quantity string implied by its CLR type. | |
| private static readonly Type[] _rawNumberConverters = [ | |
| typeof(LongRawJsonConverter), | |
| typeof(ULongRawJsonConverter), | |
| typeof(NullableRawLongConverter), | |
| typeof(NullableRawULongConverter) | |
| ]; | |
| private static string GetJsonTypeName(PropertyInfo prop) => | |
| prop.GetCustomAttribute<JsonConverterAttribute>()?.ConverterType is Type converterType && | |
| Array.IndexOf(_rawNumberConverters, converterType) >= 0 | |
| ? "_integer_" | |
| : GetJsonTypeName(prop.PropertyType); |
(needs using Nethermind.Serialization.Json; at the top; the field would read better next to _assemblies/_objectTypeName at the top of the class.)
6a44210 to
8f93b50
Compare
The base branch was changed.
dipkakwani
left a comment
There was a problem hiding this comment.
Need to rebase, since the base PR got merged before the stacked PR 😅
A member's CLR type does not determine its wire form. A field carrying an explicit [JsonConverter] can emit something other than the hex-quantity string its type implies - e.g. two `ulong` fields where one has a raw-number converter (struct-log gas vs tx gas). Instead of hard-coding the raw-number converter types, serialize a sample value through the member's converter and read back the JSON token: a Number/Boolean overrides the type-based label, while a String keeps its editorial flavour (hex data, hash, ...). Probing is scoped to explicit member converters; probing a type's default serialization is unsound for value-dependent unions (e.g. eth_syncing returns `false` or an object). Completes #12838 on top of #12868: trace_* result blockNumber and debug_* struct-log entries (gas/gasCost/pc/refund/step) now render as JSON integers, matching the wire format and Geth/OpenEthereum/Erigon. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0bd5c8c to
3747b96
Compare
| } | ||
| } | ||
|
|
||
| private static void InvokeConverterWrite(Type converterType, Type valueType, object sample, Utf8JsonWriter writer) |
There was a problem hiding this comment.
InvokeConverterWrite looks up Write using the member's CLR type after the nullable unwrap, so for BlockForRpc.Step it asks for Write(..., ulong, ...) against a JsonConverter<ulong?> and succeeds only because the reflection binder widens. A JsonConverterFactory declares no Write, so GetMethod returns null and the ! throws into the blanket catch. Taking the value type from the converter's own JsonConverter<T> base would make both explicit.
private static Type? ConverterValueType(Type converterType)
{
for (Type? t = converterType; t is not null; t = t.BaseType)
if (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(JsonConverter<>))
return t.GetGenericArguments()[0];
return null;
}Address review (wurdum): the sample write looked up `Write` by the member's CLR type after nullable unwrap, so a `JsonConverter<ulong?>` (e.g. NullableRawULongConverter on BlockForRpc.Step) matched only via binder widening, and a JsonConverterFactory - which declares no `Write` - would null-deref into the blanket catch and silently mislabel the field. Take the value type from the converter's own `JsonConverter<T>` base instead: it makes the nullable case explicit and returns cleanly for factories. Output is unchanged (verified by regenerating and diffing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolves #12838
Follow-up to #12868 (now merged), addressing the per-member converter case raised in review (#12868 (comment)).
Changes
#12868 derives JSON-RPC docs from the serializer contract and already fixes the plain-
intcase (int/uint→_integer_). The remaining #12838 gap is that a member's CLR type alone doesn't determine its wire form: a field with an explicit[JsonConverter]can emit something other than the hex-quantity string its type implies — the classic case being twoulongfields where one carries a raw-number converter (struct-loggasvs transactiongas).Rather than hard-code the raw-number converter types, this probes the member's converter: it serializes a sample value through that converter and reads back the JSON token.
Number/True/False→ override the type-based label (_integer_/_number_/_boolean_).String→ fall through and keep the editorial flavour (hex data,hash, …).It works for any converter, not a fixed list — the same "ask the serializer, don't guess" principle as #12868. Probing is deliberately scoped to explicit member converters: probing a type's default serialization is unsound for value-dependent unions (e.g.
eth_syncingreturnsfalseor an object; a default-value probe would collapse it to_boolean_).Result / verification
Diffed the generated docs against
master. The only changes are the intended fields flipping hex-string →_integer_:trace_*resultblockNumberdebug_*Geth struct-log entries:gas,gasCost,pc,refund,stepeth_syncing, byte-buffer fields, hashes, addresses and all other shapes are unchanged. Together with #12868, every field reported in #12838 is now correct and matches the wire format (Geth/OpenEthereum/Erigon).No RPC-module code changes: the implementation already matched the reference clients; only the generated docs were wrong.
Types of changes
Testing
Requires testing
Notes on testing
The DocGen tool has no test project (consistent with #12858/#12868). Verified by regenerating the docs on this branch and diffing against
master, confirming only the raw-number-converter fields change.Documentation
Requires documentation update
Regenerating the docs from this branch updates the
NethermindEth/docsJSON-RPC pages.Requires explanation in Release Notes
🤖 Generated with Claude Code