Handle optional Vec and BTreeMap as Option<t> #41
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
alex-hunt-materialize
force-pushed
the
handle_optional_vec
branch
from
f22c161
to
36d4fe6
Compare
Previously, optional array properties are required in the generated struct, but skipped if serializing. This is incorrect behavior, since an empty Vec is not the same as not supplying a Vec. This breaks server-side apply-ing new resources to remove things from the Vec, as well as cases where a property is required in a one-of, but you actually want to send the empty Vec as one of those properties. This change instead makes it an Option<Vec<T>> with #[serde(default, skip_serializing_if = "Option::is_none")]. The above also applies to BTreeMaps. Resolves kube-rs#40 Signed-off-by: Alex Hunt <alex.hunt@materialize.com>
alex-hunt-materialize
force-pushed
the
handle_optional_vec
branch
from
36d4fe6
to
1183a11
Compare
clux
approved these changes
Apr 2, 2022
| @@ -17,7 +17,6 @@ Requirements: | |||
| ## Features | |||
|
|
|||
| - **Instantly queryable**: generated type uses [`kube-derive`](https://docs.rs/kube/latest/kube/derive.CustomResource.html) to provide api integration with `kube` | |||
| - **Ergonomic Rust types**: `#[serde(default)]` on `Vec`/`BTreeMap` over `Option` wrapping | |||
There was a problem hiding this comment.
This breaks server-side apply-ing new resources to remove things from the Vec, as well as cases where a property is required in a one-of, but you actually want to send the empty Vec as one of those properties.
While there are ways to get around apply (json patch, merge patch), this definitely make the current setup less attractive. There's a similar issue in kube-rs on what we should do with it, but I think this is ultimately the right decision 😞
| @@ -210,32 +210,14 @@ fn analyze_object_properties( | |||
| docs: member_doc, | |||
| }) | |||
| } else { | |||
| // option wrapping possibly needed if not required | |||
There was a problem hiding this comment.
ah lol, foreshadowing comment haunting me
clux
reviewed
Apr 2, 2022
Comment on lines
-51
to
-52
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub groups: Vec<PrometheusRuleGroups>, |
There was a problem hiding this comment.
thanks for doing this, and regenerating the docs!
There was a problem hiding this comment.
No problem, thanks for reviewing it!
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.
Previously, optional array properties are required in the generated struct, but skipped if serializing. This is incorrect behavior, since an empty Vec is not the same as not supplying a Vec.
This breaks server-side apply-ing new resources to remove things from the Vec, as well as cases where a property is required in a one-of, but you actually want to send the empty Vec as one of those properties.
This change instead makes it an Option<Vec> with #[serde(default, skip_serializing_if = "Option::is_none")].
The above also applies to BTreeMaps.
Resolves #40