Repository navigation
feat(renderer)!: add Serde omission step options - #326
Conversation
8532385 to
83fb4f0
Compare
Bergmann89
left a comment
There was a problem hiding this comment.
Thanks! Being able to omit None values and empty vectors is a nice addition, and having it as opt-in options on the serde render steps is the right place for it.
Please also add with_serde_quick_xml_config / with_serde_xml_rs_config helpers, similar to with_quick_xml_deserialize_config, and let the existing helpers delegate to them (see the comment below). Otherwise users have to build the render steps by hand as soon as they want to enable one of the options.
Besides one correctness issue with Vec aliases, most of my remarks are about the structure of the serde attribute rendering: every field should get a single generated #[serde(...)] attribute, and the logic that builds it is duplicated in a lot of places. I suggest a small helper type for this, see the comment in steps/serde/mod.rs. A few smaller things that I did not add inline:
DynamicData::render_type_serde_*takes an_options: SerdeOptionsparameter in all three backends that is never used. Please remove it.use super::{field_overrides, SerdeOptions};sits above thecrate::imports in the three backends, and there is already ause super::{get_derive, get_dyn_type_traits};. Please merge them.let default = if has_default { quote!(default,) } else { quote!() };can simply belet default = has_default.then(|| quote!(default,));.- The helpers in
large_arrays.rsas well asSerdeOptionsandFieldOverridesare only used insidesteps/serde, sopub(super)is enough instead ofpub(crate). - In the workspace
Cargo.tomlserde-xml-rsshould come beforeserde_json/serde_with, like inxsd-parser/Cargo.toml. tests/feature/serde_large_arrays/mod.rsimportsRenderStepagain inside the first test, and several tests call.with_derive(["Debug", "PartialEq"])althoughconfig()already sets it. The 3-line comment inwrappers.rs:87-89can be one line.- A user-supplied
serde(default ...)now suppresses the generateddefaultinstead of producing a duplicate attribute. That's a behaviour change for users that do not use the new options, please mention it in the PR description. - The snapshots
none.rs,vecs.rs,both.rs,serde_xml_rs_v7.rsandserde_xml_rs_v8.rsdiffer only in a few lines, and the threeschema_default_*snapshots pin the same negative case.disabled+both+ one snapshot per backend is enough, the runtime test already covers that both options work independently. - The parse/interpret/optimize/generate/render pipeline is copied three times in the tests (
serde_skip/mod.rs,aliases.rs,schema_defaults.rs). Please move it into one helper intests/utils.rs. - The XML checks in
serde_skip/mod.rs:340-382probably pass without the options as well, because the XML backends do not writeNoneor empty sequences anyway. Either add adisabledrow or do not claim omission for XML in the PR description. round_trip_jsonasserts inside a helper, so failures point to the helper. Please return the decoded value and assert at the call site.
f3944ba to
2175b39
Compare
Add opt-in skip_none and skip_empty_vecs options to the Serde type steps and configurable Serde helpers while leaving both options disabled by default. Keep plain type and direct quick-xml rendering unaffected. Collect each field's generated adapter, default, skip predicate and rename into one Serde attribute. Resolve typedef shapes once for array adaptation and omission, preserve explicit field policies, and default empty vector aliases so omitted values can be deserialized again. Let explicit field defaults replace backend defaults, including when omission is disabled. Cover required vector aliases and list attributes, custom codecs and defaults, boxed fields, payloads and XML wrappers. Share test pipeline setup and default-method naming, consolidate omission fixtures, and preserve plain and direct quick-xml isolation checks. Compare equivalent render pipelines using array and schema-default fixtures. Demonstrate the configuration helpers in the array example. Keep generated schema defaults separate from user overrides, avoid alias resolution allocations for plain fields, and treat v8 enum content as a scalar when selecting omission predicates. BREAKING CHANGE: TypesSerdeQuickXml and TypesSerdeXmlRs require skip_none and skip_empty_vecs fields. Concrete Serde steps gain the same fields; their Default implementations and existing configuration helpers keep omission disabled.
2175b39 to
96c4cfc
Compare
|
Thanks for the thorough review, points should be adressed now |
Adds independent, opt-in omission of
Nonefields and empty vectors during Serde serialization.Changes
skip_noneandskip_empty_vecsto the Serde type render steps, both disabled by default. Plain types and direct quick-xml generation remain unaffected.with_serde_quick_xml_configandwith_serde_xml_rs_configconvenience methods. Existing helpers delegate to them with all options disabled.#[serde(...)]attribute per field. User-supplied extra attributes remain separate; explicit codecs, defaults, and skip predicates take precedence. Share schema-default method naming between backends.skip_noneoutput change for quick-xml and serde-xml-rs v0.7, and unchanged native omission for the tested v0.8 message.serde_large_arraysexample to demonstrate array adapters and omission options in one model, using the new configuration helper. Omission requires only Serde; large-array adapters additionally requireserde_with.Behavior change with omission disabled: explicit field-level
#[serde(default ...)]attributes now take precedence over generated backend defaults. Previously, both defaults could be emitted, causing duplicate Serde attribute errors. This also applies when neither omission option is enabled.Breaking configuration changes:
RenderStep::TypesSerdeQuickXmlandRenderStep::TypesSerdeXmlRsgain requiredskip_noneandskip_empty_vecsfields. Set both tofalseto leave omission disabled. Concrete Serde steps gain the same fields; theirDefaultimplementations and existing configuration helpers keep omission disabled.Custom default functions retain empty values to prevent substitution with a different default. Custom deserializers require a plain field default before automatic omission is applied. Conditional
cfg_attroverrides are not evaluated; select unconditional field attributes during generation instead. Struct-level custom defaults require explicit field defaults. The example README documents these limitations.Checklist
update-expectationsand reviewedcargo fmt,cargo clippyandcargo testpass (see CONTRIBUTING.md)