Fix XML flatten serialization: namespace loss, primitive formatting, document-root validity, and converter ordering#153
Conversation
Co-authored-by: gimlichael <8550919+gimlichael@users.noreply.github.com>
The PR shows 57 files because the merge base between this branch and To reduce the PR to just those 2 files, the branch needs to be rebased onto git fetch origin main
git rebase origin/main
git push --force-with-lease origin copilot/sub-pr-152After that, the PR will show only the 2 changed files from my review feedback commit. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 80.66% 80.74% +0.07%
==========================================
Files 600 601 +1
Lines 18932 19010 +78
Branches 1949 1964 +15
==========================================
+ Hits 15272 15349 +77
- Misses 3592 3593 +1
Partials 68 68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes correctness issues in Cuemon.Xml’s “flatten collection items” XML serialization by preserving namespaces/prefixes, producing schema-correct primitive text, ensuring well-formed document roots, and enforcing the intended enumerable-converter ordering when flattening is enabled.
Changes:
- Preserve namespace/prefix when emitting flattened container and item elements, including nested
WriteObjectroot names. - Switch primitive emission in the flatten path to
XmlWriter.WriteValueand guard against invalid XML when flattening a root-level enumerable. - Ensure
FlattenCollectionItems=truetakes effect by removing allIEnumerable-writer converters before inserting the flattened enumerable converter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs | Removes all IEnumerable writer converters when flattening is enabled, then re-adds the flattened enumerable converter to enforce effective ordering. |
| src/Cuemon.Xml/Extensions/Serialization/Converters/XmlConverterDecoratorExtensions.cs | Fixes flatten-path XML emission: namespace/prefix propagation, schema-correct primitive formatting via WriteValue, and document-root wrapping for root enumerables. |
| var isDocumentRoot = w.WriteState == WriteState.Start; | ||
| if (isDocumentRoot) { w.WriteStartElement(q.Prefix, q.LocalName, q.Namespace); } | ||
| var qe = new XmlQualifiedEntity(q.Prefix, q.LocalName, q.Namespace); | ||
| foreach (var item in o) | ||
| { | ||
| if (item == null) { continue; } | ||
| var itemType = item.GetType(); | ||
| if (Decorator.Enclose(itemType).IsComplex()) | ||
| { | ||
| var localName = q.LocalName; | ||
| Decorator.Enclose(w).WriteObject(item, itemType, opts => opts.Settings.RootName = new XmlQualifiedEntity(localName)); | ||
| Decorator.Enclose(w).WriteObject(item, itemType, opts => opts.Settings.RootName = qe); | ||
| } | ||
| else | ||
| { | ||
| w.WriteElementString(q.LocalName, Convert.ToString(item, CultureInfo.InvariantCulture)); | ||
| w.WriteStartElement(q.Prefix, q.LocalName, q.Namespace); | ||
| w.WriteValue(item); | ||
| w.WriteEndElement(); | ||
| } | ||
| } | ||
| if (isDocumentRoot) { w.WriteEndElement(); } |
There was a problem hiding this comment.
The updated flatten-items writer logic (namespace/prefix propagation, WriteValue formatting, and document-root wrapping) is not currently covered by unit tests. Please add tests that exercise: (1) FlattenCollectionItems=true with a root IEnumerable where Settings.RootName is set (ensuring the output is well-formed XML with a single document element), (2) primitive formatting (e.g., bool emits "true/false"), and (3) namespace/prefix preservation on the emitted elements.
| var converters = Decorator.Enclose(Settings.Converters); | ||
| while (true) | ||
| { | ||
| var existing = converters.FirstOrDefaultWriterConverter(typeof(IEnumerable)); | ||
| if (existing == null) { break; } | ||
| Settings.Converters.Remove(existing); | ||
| } | ||
| converters.AddEnumerableConverter(flattenItems: true); |
There was a problem hiding this comment.
RefreshWithConverterDependencies now removes all writer converters that can convert IEnumerable before inserting the flattened enumerable converter. Please add a unit test that registers multiple IEnumerable-capable converters ahead of the default one and verifies that when FlattenCollectionItems=true, serialization actually uses the flattened converter (and that all prior IEnumerable writer converters are removed as intended).
Four correctness issues in the
flattenItemsXML serialization path identified during review.Changes
XmlConverterDecoratorExtensions.csw.WriteStartElement(q.Prefix, q.LocalName, q.Namespace)instead ofq.LocalNameonly. NestedWriteObjectroot names carry namespace:new XmlQualifiedEntity(keyName, q.Namespace)(dict) andnew XmlQualifiedEntity(q.Prefix, q.LocalName, q.Namespace)(list).WriteElementString(name, Convert.ToString(value, CultureInfo.InvariantCulture))withWriteStartElement / WriteValue / WriteEndElement.XmlWriter.WriteValueproduces schema-correct output (true/falsefor booleans, ISO 8601 round-trip forDateTime) consistent with the rest of the stack.w.WriteState == WriteState.Startcheck in the non-dictionary flatten branch. Without it, a bare list at document root emits multiple top-level elements (invalid XML). When at root, items are now wrapped in a container element usingq's full qualified name.XmlFormatterOptions.csRefreshWithConverterDependenciespreviously removed only the first matchingIEnumerablewriter converter before inserting the flattened one. Changed to a loop that removes all matching converters, soFlattenCollectionItems = truetakes effect regardless of how manyIEnumerable-capable converters are registered ahead of it.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.