Unify macro attributes between serialization and deserialization derive macros #1119
Conversation
This file contains 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
|
|
wprzytula
added
area/proc-macros
wprzytula
force-pushed
the
unify-ser-deser-macro-attrs
branch
2 times, most recently
from
a504b58 to
3711afc
Compare
Lorak-mmk
reviewed
Nov 13, 2024
wprzytula
force-pushed
the
unify-ser-deser-macro-attrs
branch
from
b6a7b6d to
d9e0c6b
Compare
|
Rebased on main after #1117 was merged. |
muzarski
suggested changes
Nov 13, 2024
There was a problem hiding this comment.
I stopped review at: macros: SerializeValue supports #[allow_missing]. Will continue later.
Also a small request: could you please add missing comments for fields of Attributes and FieldAttributes? I remembered what each attribute is responsible for when I reviewed your PR for deserialization macros. I managed to forget it since then, thus such comments would be helpful during review.
This unifies the name of the attribute across SerializeValue and DeserializeValue. `forbid_excess_udt_fields` is arguably a better name for what it does than `force_exact_match`.
wprzytula
force-pushed
the
unify-ser-deser-macro-attrs
branch
from
607cc78 to
7d4d91d
Compare
|
Rebased on main. |
They are taken from DeserializeValue.
They are taken from DeserializeRow.
Instead of taking `enforce_order` as boolean flag, DeserializeValue now takes a string (e.g., `flavor = "enforce_order"`) and converts it to a variant of the Flavor enum. This way DeserializeValue is unified with SerializeValue wrt flavor selection.
Instead of taking `enforce_order` as boolean flag, DeserializeRow now takes a string (e.g., `flavor = "enforce_order"`) and converts it to a variant of the Flavor enum. This way DeserializeRow is unified with SerializeRow wrt flavor selection.
The `#[allow_missing]` attibute is intented to allow a flexible transition period when schema is altered. Namely, if a UDT is extended with a new field and the transition begins with migrating clients to the new schema (extending the Rust struct with a new field), then server may still have the old schema and provide metadata that is missing the new field. In such case, `#[allow_missing]` can be attached to the new Rust struct's field and this way handle the situation: - in deserialization, the field missing from DB metadata will be default-initialized, - in serialization, such a field will be ignored and not serialized. This commit adds support for this attribute: for parsing it using `darling`, and for its semantics in both `match_by_name` and `enforce_order` flavors. Also, corresponding (doc)tests for attribute sanity verification are added, fully based on those from DeserializeValue.
The `#[default_when_null]` attibute is intented to allow a flexible
transition period when schema is altered. Namely, if a UDT is extended
with a new field, then server will populate the new column with NULLs.
If the data model does not abstractly permit NULLs in that column, one
may not want to represent the field with `Option<T>`, not to have to
handle the case of `None`. In such case, `#[default_when_null]` can be
attached to the new Rust struct's field and this way handle the
situation:
- in deserialization, the NULL field received from the DB will be
default-initialized,
- in serialization, there is no corresponding situation, so this
attribute does nothing.
This commit adds support for this attribute: for parsing it using
`darling`, and that's it - because it's a no-op.
Also, a corresponding (doc)test is added to confirm that the attibute is
parsed correctly.
As those macros are complex and feature many switches and flavors, tests are added that verify that when a UDT struct is derived both SerializeValue and DeserializeValue: - attributes are parsed correctly and trait impls are generated without errors, - after serialization and deserialization, the result is what is expected.
Even though these macros are less complex and feature fewer switches and
flavors than their {Ser,De}Value counterparts, tests are added that
verify that when a row struct is derived both SerializeRow and
DeserializeRow:
- attributes are parsed correctly and trait impls are generated without
errors,
- after serialization and deserialization, the result is what is
expected.
Also, no longer mention the limitations of old derive macros. The new macros have no limitations compared to Serialize macros.
Some comments were missing, so I imported them from DeserializeValue.
wprzytula
force-pushed
the
unify-ser-deser-macro-attrs
branch
from
7d4d91d to
0c9a8cc
Compare
|
Addressed @muzarski comments:
|
Lorak-mmk
requested changes
Nov 14, 2024
muzarski
approved these changes
Nov 14, 2024
Lorak-mmk
approved these changes
Nov 14, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
When deserialization macros were introduced in #1024, their attributes differed in naming and usage from those introduced in #851 for serialization derive macros.
This PR aims to unify attributes between
SerializeValueandDeserializeValue, and betweenSerializeRowandDeserializeRow, so that both traits can be derived for a struct without a clash in attribute parsing.Fixes: #462
FINALLY!
What's done
SerializeValue:force_exact_match->forbid_excess_udt_fields(attribute renamed for consistency withDeserializeValue);Flavoris shared between serialization and deserialization derive macros, and both have the same syntax for choosing the flavor:flavor = "enforce_order";allow_missingis supported forSerializeValue, as it is inDeserializeValue;default_when_nullis parsed and ignored inSerializeValue, as it's a no-op for serialization (it's only useful for deserialization, as it says: "If I got NULL for a non-Option Rust field, let's fill it with Default::default()"; currently, it would cause a parse error inSerializeValue.Deserialize{Value,Row}) for sanity verification of provided attributes.Pre-review checklist
./docs/source/.[ ] I added appropriateFixes:annotations to PR description.