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

Support comparison of Schemas generated by TapirSchemaToJsonSchema #179

Merged

Conversation

seveneves
Copy link
Contributor

@seveneves seveneves commented Jul 26, 2024

TapirSchemaToJsonSchema generates JsonSchema specification http://json-schema.org/draft/2020-12/schema# and uses #/$defs/ prefixes to locate reference to schemas stored in Schema.$defs field.

This change adds support for both #/$defs/ and #/components/schemas/

Two helper methods are provided that can be used to create SchemaResolver from

  • named schemas that are available in a form of a Map[String, Schema]
  • from a schema's $defs

Closes #172 and #170

Test for SchemaResolver.defsSchemas relies on TapirSchemaToJsonSchema from tapir project and I will provide a PR with specification there

…SchemaToJsonSchema`

`TapirSchemaToJsonSchema` generates JsonSchema specification `http://json-schema.org/draft/2020-12/schema#` and uses `#/$defs/` prefixes to locate reference to schemas stored in `Schema.$defs` field.

This change adds support for both `#/$defs/` and `#/components/schemas/` and the choice is made by the user of `SchemaComparator` via the following helper methods

* `SchemaResolver.components`
* `SchemaResolver.defsSchemas`

Closes softwaremill#172 and softwaremill#170
@seveneves
Copy link
Contributor Author

@adamw @ghik

Let me know if you need full binary compatibility here or if it is ok to not have it given it is 0.x.x version.

@adamw
Copy link
Member

adamw commented Jul 28, 2024

Bin compat is not required, exactly for this reason - it's a 0.x version. The mima check is currently informative, suggesting that we might want to bump the minor version

@adamw
Copy link
Member

adamw commented Aug 5, 2024

Thanks for the patch and sorry for the delay in reviewing :)

To simplify a bit, maybe we could simply have a predefined list of SchemaResolvers, so that by default we would check both #/$defs/ and #/components/schemas/? I guess schemas which have references in both of these places are still valid, right? It's not an either-or?

@seveneves
Copy link
Contributor Author

To simplify a bit, maybe we could simply have a predefined list of SchemaResolvers, so that by default we would check both #/$defs/ and #/components/schemas/? I guess schemas which have references in both of these places are still valid, right? It's not an either-or?

Yes, supporting both #/$defs/ and #/components/schemas/ in one resolver is not a problem. But what steered me into this direction is the following code

schema.$defs.getOrElse(Map.empty).collect { case (name, s: Schema) => name -> s },

when initializing a resolver from schema.$defs.

I can resolve references by both #/$defs/ and #/components/schemas/ in the resolver. and also provide helper methods of initialising a resolver from schema.$defs, same as it is done now.

Is this a good direction?

@ghik
Copy link
Contributor

ghik commented Aug 6, 2024

A few doubts from me:

  • What if $defs appear in a schema that is deeply nested within the overall document, as opposed to a schema being the document itself? Is the #/$defs/ reference still valid? In general, what does # resolve to? Is it relative to the current schema or to the toplevel document? This needs more investigation.
  • Can paths starting with # refer to arbitrary locations within the JSON, not just #/components/schemas/ or #/$defs/? If yes, we should probably support that properly, instead of just selectively supporting #/components/schemas/ and #/$defs/ references

@seveneves
Copy link
Contributor Author

For me, the main goal for this PR change is to support TapirSchemaToJsonSchema. Since tapir and sttp-apispec are closely related, it makes sense to me that schemas generated by TapirSchemaToJsonSchema can be validated by schema comparator in this project.

It is possible to completely decouple this project form decisions made in TapirSchemaToJsonSchema and not rely on any reference used there, ie #/$defs/, and implement comparison functionality to fully adhere to OpenApi and JsonSchema specs. But it potentially will have impact on how Schema value is represented to support dynamic references like #/$defs/, which can point to any part of json and not just $defs and possibly to external resources (URI based). So then there has to be a mechanism of looking up paths in a similar way xpath does it for xml but also resources to external json schemas.

In the principles of KISS, I think supporting of just TapirSchemaToJsonSchema is what has to be done first. My two cents as an external user of both projects.

Please let me know if it makes sense to you, @ghik @adamw

@adamw
Copy link
Member

adamw commented Aug 7, 2024

@seveneves yes supporting what we generate definitely makes sense :) What @ghik writes might be true in a wider perspective, but we've got to start somewhere. So maybe just change the default to check both resolvers by default for references, and we'll be good to go?

@seveneves
Copy link
Contributor Author

@adamw I pushed a change and changed the description for the PR

@adamw
Copy link
Member

adamw commented Aug 7, 2024

@seveneves Thanks! One last thing - maybe it would be possible to add a test case checking that indeed resolving #/$defs/ schemas works?

@adamw adamw merged commit 08eaa08 into softwaremill:master Aug 7, 2024
3 of 4 checks passed
@adamw
Copy link
Member

adamw commented Aug 7, 2024

Great, thanks!

@seveneves seveneves deleted the feature-#172/allow-comparson-json-schema branch August 9, 2024 06:01
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.

SchemaComparator does not handle references to $defs
3 participants