Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement JsonSchemaAs for KeyValueMap #713

Merged
merged 1 commit into from
Mar 10, 2024

Conversation

swlynch99
Copy link
Contributor

@swlynch99 swlynch99 commented Mar 7, 2024

I'm back with yet another PR! This time we're doing KeyValueMap.

I think this one is probably the most complicated conversion that needs to be done to make the schema work. We need to convert a schema that looks like this

{
  "type": "object",
  "properties": {
    "$key$": "...",
    "a": "...",
  },
  "required": ["$key$", "a"]
}

into this

{
  "type": "object",
  "additionalProperties": {
    "type": "object",
    "required": ["a"],
    "properties": { "a": "..." }
  }
}

On its own that is not to bad but it gets more complicated since we also have to deal subschemas which can be generated from types that make use of #[serde(untagged)] and #[serde(flatten)]. See the doc comment on kvmap_transform_schema for more details.

Notes

  • I'm not entirely sure this is correct in the presence of an anyOf subschema that makes use of minProperties and maxProperties (normal ones where they have a $key$ field should work fine). I'm not entirely sure there is one correct behaviour in the case either but to me this seems like a edge case that is pretty far out there so it should be fine.
  • It also isn't correct if the $key$ property ends up being defined in some external schema (i.e. $ref points to some random document on the internet). This seems like an edge case and I don't think there's any reasonable behaviour here anyway.

@swlynch99
Copy link
Contributor Author

swlynch99 commented Mar 7, 2024

The CI errors all seem to be related to the #[warn(unused_qualifications)] lint in Cargo.toml. That doesn't seem related to this PR and when I went to fix them the suggestions from cargo clippy --fix seemed like they would be invalid under #[no_std]. I'll leave this one up to you to fix and/or disable the lints and then rebase once that is done.

@swlynch99 swlynch99 force-pushed the schemars-key-value-map branch 2 times, most recently from 65609d6 to 16a1908 Compare March 8, 2024 20:33
This one is probably the most complicated of all the schema adaptors. We
have to process the inner schemas until we find the one with the `$key$`
field, remove that field, and then recreate it as an enum.
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 75.64103% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 66.04%. Comparing base (4c05768) to head (a804257).
Report is 1 commits behind head on master.

Files Patch % Lines
serde_with/src/schemars_0_8.rs 75.64% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #713      +/-   ##
==========================================
+ Coverage   65.64%   66.04%   +0.40%     
==========================================
  Files          38       38              
  Lines        2343     2421      +78     
==========================================
+ Hits         1538     1599      +61     
- Misses        805      822      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

impl<T, F: FnOnce(T)> Drop for DropGuard<T, F> {
fn drop(&mut self) {
// SAFETY: value is known to be initialized since we only ever remove it here.
let value = unsafe { ManuallyDrop::take(&mut self.value) };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit: I think you can use a Option for the value. Then you take() the option in the drop implementation. The Deref implementation would need a match with panic, in case the Option is None, but it should be possible to get rid of the None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm occasionally a little prone to using unsafe for stuff like this. If you would prefer that I use an option here I can definitely submit a PR to change that.

@jonasbb
Copy link
Owner

jonasbb commented Mar 10, 2024

Thank you, this looks good. Your comments are good, explain what is going on, and I could follow them. Thanks for getting this to work :)

@jonasbb jonasbb merged commit c5c35db into jonasbb:master Mar 10, 2024
22 checks passed
@swlynch99 swlynch99 deleted the schemars-key-value-map branch March 11, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants