jonasbb / serde_with

This crate provides custom de/serialization helpers to use in combination with serde's `with`-annotation and with the improved `serde_as`-annotation.

https://docs.rs/serde_with

Apache License 2.0

667 stars 72 forks source link

Generated schemas are not valid when using rust style names in ref #799

Closed waltronix closed 2 weeks ago

waltronix commented 2 weeks ago

798

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.54%. Comparing base (2d70b70) to head (98bcdb7). Report is 19 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #799 +/- ## ========================================== + Coverage 67.34% 67.54% +0.20% ========================================== Files 40 40 Lines 2468 2499 +31 ========================================== + Hits 1662 1688 +26 - Misses 806 811 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

swlynch99 commented 2 weeks ago

I'm not a maintainer of serde-with, just the person who originally wrote the schemars integration. So feel free to ignore this if you want.

With that said, it would be really helpful to document this on the schema_id and schema_name methods of the JsonSchemaAs trait. That the characters need to be valid as a URI path segment for refs to work is very much not apparent unless you are familiar with the json schema spec.

jonasbb commented 2 weeks ago

Hi, thanks for the PR. The change looks fine overall. Adding the extra validation check to the test makes sense. Do you have a reference which names are allowed? In #798 you listed the three characters <, >, and ,. The , is still used here and seems to pass the tests.

If the names are invalid anyway, then backwards compatability is not a problem.

swlynch99 commented 2 weeks ago

I think it needs to be a URI path segment, so it would be defined in RFC 3986 section 3.3. That refers back to section 2 and section 2.2 seems to indicate that all 3 of (, ,, and ) are valid.

waltronix commented 2 weeks ago

Thanks for your fast response!

I had a mistake in my bug report. There is no problem with the , itself, the problem arises from the space after it.

I also extended the doc comment for schema_name(). As schema_id() is not part of the generated schema according to current documentation I don't think have to require it to be a URI path segment.