XmlSerializer - Fire UnknownAttribute for stray attributes on built-in typed elements - #131302
Conversation
XmlSerializer's UnknownAttribute event only fired for unknown attributes on elements mapped to user-defined struct types. Elements mapped to built-in types (primitives, arrays, and collections) read their content directly and never inspected attributes, so stray attributes on them were silently dropped. Walk the attributes on such elements and raise UnknownNode/UnknownAttribute for any non-namespace attribute, mirroring the existing struct handling in WriteAttributes. xsi:nil elements are skipped (via GetNullAttr) since the nil marker is consumed by ReadNull, matching the struct path. The change is applied to all three readers: reflection, IL-gen, and code-gen. When no handler is subscribed UnknownNode is a no-op, so there is no behavioral change unless the UnknownAttribute event is used. Fixes dotnet#96683. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: be1fcace-9193-484e-8e3b-ed6a4ca70d8d
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a behavioral gap in XmlSerializer deserialization events: unknown (non-namespace) attributes on elements mapped to built-in types (primitives, arrays, collections) are now surfaced via UnknownNode / UnknownAttribute, aligning these paths with existing struct-mapped behavior.
Changes:
- Add attribute-walking logic for primitive / array / collection element readers so stray attributes raise
UnknownNode/UnknownAttribute(skippingxsi:nilelements). - Apply the same behavior across the three reader implementations (reflection-based, IL-gen, and source-gen).
- Add regression tests validating unknown attributes are reported on built-in typed members/items, and that
xsi:nilis not reported.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs | Adds regression coverage for unknown attributes on built-in typed members/items and verifies xsi:nil is not surfaced as unknown. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs | Emits IL to walk attributes for primitive/array/collection element paths and raise unknown-attribute events consistently. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs | Updates source-gen writer to generate attribute-walking logic for primitive/array element paths. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs | Adds reflection-path attribute walking for primitive/array/collection element reads. |
TypeWithBuiltInTypedMembers and TypeWithNullableBuiltInTypedMembers were defined in XmlSerializerTests.cs. That file is also compiled into the sgen (XMLSERIALIZERGENERATORTESTS) test leg, whose SerializableAssembly baselines are sensitive to newly added types. These types are only exercised by the reflection and IL-gen readers and are not interesting for sgen. Define them in SerializationTypes.RuntimeOnly.cs (a runtime-only, non-sgen shared file) instead, and guard the two new regression tests with '#if !XMLSERIALIZERGENERATORTESTS' so the sgen leg neither compiles nor needs them. The tests still run in the default (IL-gen) and reflection-only legs, and the types resolve via the existing 'using SerializationTypes;' import. No product behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: be1fcace-9193-484e-8e3b-ed6a4ca70d8d
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
…efinitions - Moved TypeWithBuiltInTypedMembers and TypeWithNullableBuiltInTypedMembers from SerializationTypes.RuntimeOnly.cs to SerializationTypes.cs for better organization. - This change enhances code clarity and maintains consistency in the test structure.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Summary
XmlSerializer'sUnknownAttributeevent only fired for unknown attributes on elements mapped to user-defined struct types. Elements mapped to built-in types (primitives, arrays, and collections) read their content directly and never inspected attributes, so stray attributes on those elements were silently dropped and never surfaced through the event.This fixes #96683 by making the primitive/array/collection paths walk the element's attributes the same way the struct path already does.
Approach
When an element is mapped to a primitive, array, or collection, the reader now walks the current element's attributes and raises
UnknownNode/UnknownAttributefor any non-namespace attribute, mirroring the existing struct handling inWriteAttributes. The same change is applied consistently to all three readers:ReflectionXmlSerializationReader)XmlSerializationReaderILGen, the default runtime reader)XmlSerializationReader/sgen)xsi:nilelements are deliberately skipped (guarded byGetNullAttr()). The nil marker is consumed by the subsequentReadNull()call, and surfacing it would be wrong; this matches the struct path, where a nil element returns before its attributes are inspected.Compatibility
There is no app-compat switch and no new public API surface. When no handler is subscribed,
UnknownNodeis a no-op, so behavior is unchanged unless a caller has explicitly subscribed to theUnknownAttributeevent, in which case they now also get notified about attributes on built-in typed elements. Anyone subscribed to that event almost certainly wants this to work correctly.Tests
Added two regression tests to
XmlSerializerTests:XmlUnknownAttributeOnBuiltInTypedMembersTestverifies that stray attributes on string/int/nullable-int/list/array members and their items are all reported, and that they do not disrupt deserialization of the element content.XmlNilAttributeOnBuiltInTypedMembersNotReportedTestverifies thatxsi:nilis not surfaced as an unknown attribute and still drives the expected null results.Verified across the IL-gen and reflection-only test legs; the full
XmlSerializerTestsclass passes (324 tests, 0 failures). The tests fail without the fix and pass with it.Fixes #96683
Note
This pull request was authored with the assistance of GitHub Copilot.