-
Notifications
You must be signed in to change notification settings - Fork 66
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
Include support for schemars #666
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #666 +/- ##
==========================================
+ Coverage 64.97% 65.26% +0.29%
==========================================
Files 36 38 +2
Lines 2187 2240 +53
==========================================
+ Hits 1421 1462 +41
- Misses 766 778 +12 ☔ View full report in Codecov by Sentry. |
Thank you for picking up that PR :) Overall, this already looks good. Some more tests would be good. I will need some more time to look at the PR, but I can't give you a timeline right now. If I see it correctly, the code is even supposed to handle conditional implementations of Do you have any suggestion how best to test the new schema implementations? There are two basic tests types I can think of: 1) Generate a schema for a type and verify that it is as expected (like the current test) and 2) generate a schema and serialize a value to JSON, then validate the JSON using the schema. |
No rush on the PR review :) I figured that this might take a while. I have added tests for the handling of conditional As for testing, I see three different kinds of tests that can be done. You've already got the first two
I would lean towards a mix of 2 and 3 when writing tests. For more complicated impls I think it should mostly be type 3 tests. I do think that it is reasonable to skip the test cases when the Ultimately the decision on what should have tests is up to you :) Now it looks like I need to figure out the right dependency version to add to get a |
I agree with this assessment. I think type 1 and 2 work for more basic implementations that basically just forward the definition to another type. You can quickly check that Your type 3 is good. I forgot that there are cases that do not delegate to another schema. For these more complex implementations, it makes sense to have positive and negative cases. So far, I have used
I would not totally skip tests for them, but they can be reduced. For example, you could have one test with If the current MSRV is making trouble for you, feel free to bump it. |
This commit adds support for emitting #[schemars(with = "::serde_with::Schema<T, ...>")] field annotations when any of the following are true: - there is a #[derive(JsonSchema)] attribute, or, - the user explicitly requests it via #[serde_as(schemars)] This requires a bunch of extra work to properly scan for a few different possible paths to JsonSchema (see the added docs for details) and also to properly handle the case where the derive is within a #[cfg_attr]. eg #[cfg_attr(feature = "blah", derive(schemars::JsonSchema))] While rustc will evaluate the cfgs for attributes declared by derive macros unfortunately it does not do the same for attribute macros so we need to do it ourselves. The code here started from jonasbb#623 as a base and all changes have been made on top of that. Co-Authored-By: Jonas Bushart <jonas@bushart.org>
This commit adds two main new types to the crate: - trait JsonSchemaFor<T> - this is the main trait needed to make things work. It is meant to be an analogue of the SerializeAs and DeserializeAs traits but for the JsonSchema trait. - struct Schema<T, TA> - this implements JsonSchema if TA: JsonSchemaFor<T>. It is automatically used by the #[serde_as] macro if it finds a #[derive(JsonSchema)] annotation. It then adds a bunch of JsonSchemaFor<T> impls which forward things through the relevant JsonSchema impl. By combining these two types we can use them to unwrap into the types making up the #[serde_as(as = "T")] annotation. As an example, suppose we have #[serde_as(as = "Option<Same>")] field: Option<u32> the type that will get emitted into the #[schemars(with = ...)] annotation is Schema<Option<u32>, Option<Same>>. To get the actual schema we use the following impls: impl JsonSchema for Schema<Option<u32>, Option<Same>> -> forwards to impl JsonSchemaFor<Option<u32>> for Option<Same> -> forwards to impl JsonSchema for Option<Schema<u32, Same>> -> which has its own custom impl This design has the unfortunate consequence that we need to do a lot of forwarding trait impls but I have been unable to figure out a design which automatically makes Schema<T, T> work for any T: JsonSchema while still allowing #[serde_as(as = "Option<Same>")] attributes to generate the correct schema.
These are enough to show the basic workings and to add some basic test cases verifying that everything fits together properly.
This tests that the various different use cases work properly and produce the expected schema.
Better to remove a public trait if it is not needed. This simplifies the public API at the expense of making the internal implementations a bit more verbose.
It was not being used by the test so there's no point in having it right now.
It has no members so there's no point in adding it to the public API.
Since this is done via a compile_fail I've needed to introduce the trybuild crate.
As it turns out, rustc's "trait unimplemented" error messages have not stayed perfectly stable across the last 10 versions. This is likely an issue that would keep happening with trybuild which would make it somewhat unsuitable. With some thinking I was able to get the same test coverage without requiring a compile error.
I've gone and added snapshot tests that cover most of the forwarding impls for std types. These are done through a macro so it should be pretty easy to write more snapshot tests in the future. The MSRV is no issue. Generating the lockfile was inconvenient since I had to use the latest rust nightly for the msrv-aware resolver but once it is generated then everything works as normal. |
I saw the existing PR for this over in #623 and figure that since I'm interested in using
schemars
andserde_with
together then I might as well push this over the finish line.What's been kept the same as #623:
Schema<T, TA>
JsonSchema
forSchema<T, TA>
#[serde_as]
macro can add#[schemars(with = "Schema<T, ...>")]
attributes to fields annotated with#[serde_as(as = ...)]
What's new here:
JsonSchema
impls for various std wrapper types.JsonSchema
forSchema<T, Same>
andSchema<T, DisplayFromStr>
#[serde_as]
will now automatically detect a#[derive(JsonSchema)]
attribute and use that to determine if it should add#[schemars(with = ...)]
to fields. This only happens if theschemars_0_8
feature is enabled and can be overridden by doing#[serde_as(schemars = true/false)]
.I have a branch where I'm working to implement
JsonSchema
for all the types in this crate but figured it would be best to make multiple (still large) PRs instead of one enormous one.