feat: XML value type correction via xsi:type / XSD (2.0.0, breaking)#37
Open
jefim wants to merge 6 commits into
Open
feat: XML value type correction via xsi:type / XSD (2.0.0, breaking)#37jefim wants to merge 6 commits into
jefim wants to merge 6 commits into
Conversation
Add Options.TypeCorrection (None/Attributes/Schema) to convert numeric and boolean XML values into native JSON types, driven either by inline xsi:type attributes or by the supplied XSD. BREAKING: the XSD parameter moves from the Input tab to the Options tab and is only used in Schema mode. migration.json copies Input.XSD -> Options.XSD and sets TypeCorrection=Schema; tenants without migration support must apply the manual upgrade path documented in CHANGELOG. Schema mode with a blank XSD is a graceful no-op rather than an error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors the ConvertXMLStringToJToken task to accept an Options parameter with TypeCorrectionMode, moves XSD from Input to Options (used only when TypeCorrection=Schema), adds schema-aware validation/hint injection, implements post-serialization type correction for xsi:type/XSD types, expands parsing utilities, adds tests, and releases v2.0.0 with migration metadata. ChangesXML→JSON Type Correction with Options-Driven Schema Processing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs (1)
43-51: ⚡ Quick winRename
XSDtoXsdbefore locking the new public contract.This is a new public task parameter, and the all-caps abbreviation is out of line with the repository's C# naming rule. Since this PR already introduces a 2.0.0 breaking change, this is the cheapest point to align the API surface and update the related migration/docs in one pass. As per coding guidelines, "Proper naming for abbreviations (Csv, Url, Api)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs` around lines 43 - 51, Rename the public property XSD to Xsd in the Options class (change the property symbol from XSD to Xsd in Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs), update its XML doc/comments to use the new name, and update any references/usages (UIHint attribute remains unchanged) including serialization/mapping, tests, samples and documentation/migration notes to reflect the new public API name so the public contract aligns with the repository naming rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.cs`:
- Around line 92-99: Tests access result.Jtoken and nested JTokens (e.g., price,
scores) before verifying conversion succeeded, which can throw null/cast
exceptions; update the affected tests (e.g., NoneMode_ShouldKeepValuesAsStrings,
SchemaMode_ShouldConvertValuesUsingXsdTypes,
SchemaMode_WithoutXsd_ShouldNoOpAndKeepStrings) to first
Assert.IsTrue(result.Success) and/or Assert.IsNotNull(result.Jtoken) then
retrieve the nested token, and add Assert.IsNotNull(price) or equivalent before
using price.Type or indexing into price["`#text`"]; apply the same guarded pattern
around any deep dereference of result.Jtoken after calling
JSON.ConvertXMLStringToJToken to ensure clear assertion failures instead of
exceptions.
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.cs`:
- Around line 64-77: The current code always calls CorrectTypes(token) for any
non-None TypeCorrection, but per the comment we must treat Schema mode with a
blank XSD as a no-op; update the call site so CorrectTypes is skipped when
options.TypeCorrection == TypeCorrectionMode.Schema and options.XSD is
null/empty. Concretely, change the condition that guards CorrectTypes(token) to:
only run CorrectTypes when options.TypeCorrection != TypeCorrectionMode.None AND
NOT (options.TypeCorrection == TypeCorrectionMode.Schema &&
string.IsNullOrWhiteSpace(options.XSD)); keep existing logic around useSchema,
LoadXmlDocumentWithSchemaHints/LoadXmlDocument, JsonConvert.SerializeXmlNode and
JToken.Parse unchanged.
- Around line 226-233: ApplyTypeHint currently only looks for the literal
property name XsiTypePropertyName (e.g., "`@xsi`:type") which fails when a
different prefix is used; change ApplyTypeHint to search the JObject for any
property matching the pattern "@{prefix}:type", verify that the corresponding
"xmlns:{prefix}" property exists and equals XsiNamespace, then extract the local
type name and use TypeConverters.TryGetValue as before (keep existing LocalName
usage). Also update the Schema-mode check to detect any xmlns:* bound to
XsiNamespace rather than requiring an "xsi" prefix. Add unit tests that assert
conversions succeed when the xsi namespace is aliased (e.g., "i:type" with
"xmlns:i" set) for both TypeCorrectionMode.Attributes and
TypeCorrectionMode.Schema.
---
Nitpick comments:
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs`:
- Around line 43-51: Rename the public property XSD to Xsd in the Options class
(change the property symbol from XSD to Xsd in
Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs), update its XML
doc/comments to use the new name, and update any references/usages (UIHint
attribute remains unchanged) including serialization/mapping, tests, samples and
documentation/migration notes to reflect the new public API name so the public
contract aligns with the repository naming rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bddfa45-6463-4d9c-b7b9-b35f48809c7d
📒 Files selected for processing (7)
Frends.JSON.ConvertXMLStringToJToken/CHANGELOG.mdFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Input.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.csprojFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/migration.json
💤 Files with no reviewable changes (1)
- Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Input.cs
Contributor
|
Look at e.g. SchemaMode_ShouldConvertValuesUsingXsdTypes() test. As a response it gives us I think we need to add logic to strip this additional xmln:xsi added by schema |
MatteoDelOmbra
requested changes
May 28, 2026
Adds a BadValueAction enum (Ignore | Throw) shown via UIHint when TypeCorrection is Attributes or Schema. When Throw, an unparseable value with a known numeric/boolean xsi:type raises a FormatException identifying the element path and target type. Default stays Ignore to preserve existing behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ParseBoolean is now case-insensitive for 'true'/'false' (Matteo). - Schema-without-XSD is a true no-op: CorrectTypes is no longer invoked, so inline xsi:type can no longer sneak through (CodeRabbit). - In Schema mode, inline xsi:type attributes are stripped from the document before validation, so XSD is the single source of truth and an unqualified xsi:type can't trip the validator (Matteo). - ApplyTypeHint resolves @<prefix>:type against the active xmlns scope, so authors using xmlns:i (instead of the conventional xsi) get their type hints honoured in both Attributes and Schema modes (CodeRabbit). - Tightened nullable guards in older tests; added 9 new tests covering the above (case-insensitive booleans, schema-no-op-with-inline, schema-wins, schema-strips-on-string, aliased-prefix Attributes/Schema, unrelated-prefix ignored, prefix-on-inner-element). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
There is also edgy case we actually gonna need to handle. Test that shows the problem |
Newtonsoft preserves the wrapper for both xsi:nil="true" and "false" (it does not auto-collapse nil=true to null), so the task now post-processes the JSON tree to give authors control over how empty elements appear: - xsi:nil="true" empty -> JSON null (collapsing Newtonsoft's wrapper) - xsi:nil="true"+content -> content wins, nil flag stripped - xsi:nil="false" empty -> default(T) for the xsi:type or schema type (long 0 / double 0 / decimal 0 / bool false / "" for string) - xsi:nil="false"+content-> nil stripped, content flows through normally - xsi:nil="false" empty with no type info -> no-op (Attributes mode); for Schema mode the schema is itself type info, so empty xs:string yields "" Lookups are prefix-agnostic (xmlns:i + i:nil works the same as xsi:nil), threaded through an active xmlns scope that mirrors XML namespace scoping. None mode is untouched - xsi:nil wrappers pass through verbatim. Adds 12 unit tests covering each mode + edge case (33 tests total). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.cs (2)
184-190:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEmpty
xs:stringelements still have no path to""in schema mode.
AddSchemaHintsonly preserves schema type information for convertible numeric/boolean types. For a validated<name/>declared asxs:string, no hint survives intoCorrectTypes, so the empty element still falls through asnullunless the XML also carriesxsi:nil='false'. That misses the schema-string case called out in the PR discussion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.cs` around lines 184 - 190, Add handling for schema-declared xs:string empty elements: in AddSchemaHints, when typeCorrection == TypeCorrectionMode.Schema and schemaInfo?.SchemaType indicates an xs:string (or equivalent string type), set the xsi:type attribute (using GetConvertibleTypeName or directly "xs:string") on the element just like numeric/boolean convertibles so the hint survives; then ensure CorrectTypes treats xsi:type="xs:string" as meaning empty element => empty string (not null). Specifically update the logic around GetConvertibleTypeName / element.SetAttributeValue in AddSchemaHints to include string types and ensure CorrectTypes checks xsi:type for "string" and converts empty nodes to "" accordingly.
198-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the injected root-level
xmlns:xsiand fix emptyxs:stringoutputWhen
TypeCorrectionMode.Schemainjectsxmlns:xsiondocument.Root, the returned JSON includes the corresponding root-level@xmlns:xsiattribute;CorrectTypesonly uses@xmlns:*for prefix scoping and removes@xmlns:*only when collapsing specific element wrappers, so the injected namespace can leak into the payload shape.Also, non-convertible schema types (e.g.,
xs:string) never get a persistedxsi:typehint fromAddSchemaHints, andApplyXsiNilonly coerces empty values when anxsi:nilattribute is present—so emptyxs:stringelements can remainnullinstead of becoming"".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.cs` around lines 198 - 201, The injected root-level xmlns:xsi leaks into the JSON and empty xs:string elements become null; change the logic so we do not pin a global xmlns:xsi on document.Root unnecessarily and ensure xs:string gets handled: only add the xsi prefix/namespace on the specific element(s) that actually receive xsi:type (avoid calling document.Root.SetAttributeValue for the global root in the TypeCorrectionMode.Schema path and instead set the namespace on the element returned/processed), or remove the injected xmlns:xsi before finalizing the document in CorrectTypes; additionally, ensure AddSchemaHints or ApplyXsiNil records an xsi:type for non-convertible xs:string (or treats empty xs:string elements as empty string) so empty xs:string elements are coerced to "" rather than null (refer to IsNamespaceDeclared, SetAttributeValue usage, AddSchemaHints, ApplyXsiNil and CorrectTypes to locate and update the behavior).
🧹 Nitpick comments (1)
Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.cs (1)
153-177: ⚡ Quick winRename this test to match the behavior it actually verifies.
The assertions prove boolean parsing is case-insensitive, so
ShouldBeCaseSensitiveis misleading and makes failures harder to read.As per coding guidelines, "Tests should: Follow Microsoft unit testing naming and structuring conventions".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.cs` around lines 153 - 177, Rename the test AttributesMode_BooleanParsing_ShouldBeCaseSensitive to reflect its actual verification of case-insensitive boolean parsing (e.g., AttributesMode_BooleanParsing_ShouldBeCaseInsensitive); update the test method name wherever referenced (the test method itself and any test runners) while leaving the body intact — this affects the test method AttributesMode_BooleanParsing_ShouldBeCaseSensitive that calls JSON.ConvertXMLStringToJToken with new Options { TypeCorrection = TypeCorrectionMode.Attributes } and asserts that "true", "TRUE", and "True" all parse as booleans.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs`:
- Around line 67-68: The public property named XSD in the Options class violates
the project's abbreviation casing rules; rename the property XSD to Xsd and
update all references to it (including attributes like
UIHint(nameof(TypeCorrection), "", TypeCorrectionMode.Schema) if they reference
it indirectly) to preserve the public API and follow Microsoft C# naming
conventions; ensure any serialization, mapping, tests, XML/JSON contract
consumers, and docs that reference Options.XSD are updated to Options.Xsd and
run tests to confirm no regressions.
---
Outside diff comments:
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.cs`:
- Around line 184-190: Add handling for schema-declared xs:string empty
elements: in AddSchemaHints, when typeCorrection == TypeCorrectionMode.Schema
and schemaInfo?.SchemaType indicates an xs:string (or equivalent string type),
set the xsi:type attribute (using GetConvertibleTypeName or directly
"xs:string") on the element just like numeric/boolean convertibles so the hint
survives; then ensure CorrectTypes treats xsi:type="xs:string" as meaning empty
element => empty string (not null). Specifically update the logic around
GetConvertibleTypeName / element.SetAttributeValue in AddSchemaHints to include
string types and ensure CorrectTypes checks xsi:type for "string" and converts
empty nodes to "" accordingly.
- Around line 198-201: The injected root-level xmlns:xsi leaks into the JSON and
empty xs:string elements become null; change the logic so we do not pin a global
xmlns:xsi on document.Root unnecessarily and ensure xs:string gets handled: only
add the xsi prefix/namespace on the specific element(s) that actually receive
xsi:type (avoid calling document.Root.SetAttributeValue for the global root in
the TypeCorrectionMode.Schema path and instead set the namespace on the element
returned/processed), or remove the injected xmlns:xsi before finalizing the
document in CorrectTypes; additionally, ensure AddSchemaHints or ApplyXsiNil
records an xsi:type for non-convertible xs:string (or treats empty xs:string
elements as empty string) so empty xs:string elements are coerced to "" rather
than null (refer to IsNamespaceDeclared, SetAttributeValue usage,
AddSchemaHints, ApplyXsiNil and CorrectTypes to locate and update the behavior).
---
Nitpick comments:
In
`@Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.cs`:
- Around line 153-177: Rename the test
AttributesMode_BooleanParsing_ShouldBeCaseSensitive to reflect its actual
verification of case-insensitive boolean parsing (e.g.,
AttributesMode_BooleanParsing_ShouldBeCaseInsensitive); update the test method
name wherever referenced (the test method itself and any test runners) while
leaving the body intact — this affects the test method
AttributesMode_BooleanParsing_ShouldBeCaseSensitive that calls
JSON.ConvertXMLStringToJToken with new Options { TypeCorrection =
TypeCorrectionMode.Attributes } and asserts that "true", "TRUE", and "True" all
parse as booleans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90b62f3d-db61-4926-a4be-302069c18d4d
📒 Files selected for processing (3)
Frends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken.Tests/UnitTests.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/ConvertXMLStringToJToken.csFrends.JSON.ConvertXMLStringToJToken/Frends.JSON.ConvertXMLStringToJToken/Definitions/Options.cs
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
2 participants
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.
Summary
Adds an
Optionsparameter withTypeCorrection(None/Attributes/Schema) so the task can emit native JSON numbers and booleans instead of stringifying everything.Attributes— reads inlinexsi:typehints from the XML (e.g.xsi:type="float").Schema— derives value types (and array mapping) from the supplied XSD.None(default) — unchanged behavior; every value stays a string.Full numeric + boolean set is supported (
float/double/decimal, the integer family,boolean); unparseable values are left as strings.The
XSDparameter moved from the Input tab to the Options tab and is only used/shown inSchemamode.migration.jsonships with the package: copiesInput.XSD→Options.XSDand setsTypeCorrection = Schema, preserving the 1.2.0 array-mapping behavior.TypeCorrection = Schemaand paste the XSD intoOptions.XSD. Steps that never used an XSD need no change.Schemamode with a blank XSD is a graceful no-op (returns strings) rather than an error, which keeps the unconditional migration safe.See
CHANGELOG.mdfor the full rationale and upgrade path.Changes
Definitions/Options.cs(Options+TypeCorrectionMode);XSDcarries[UIHint(nameof(TypeCorrection), "", TypeCorrectionMode.Schema)]for conditional display.XSDremoved fromDefinitions/Input.cs.ConvertXMLStringToJToken.cs.migration.json+CHANGELOG.mdwired into csproj packaging; version →2.0.0.Testing
dotnet test— 9/9 passing (None/Attributes/Schema modes, keep-wrapper, unparseable fallback, schema-typed arrays, no-op-without-xsd). Also verified the real hotels XML+XSD convertsstar_rating→Integer andopen_for_booking→Boolean via a throwaway harness.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores