Conversation
72ee7d7 to
5bafc14
Compare
kstich
requested changes
Feb 14, 2020
smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/DeconflictingStrategy.java
Outdated
Show resolved
Hide resolved
smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/DefaultRefStrategy.java
Outdated
Show resolved
Hide resolved
smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/DefaultRefStrategy.java
Outdated
Show resolved
Hide resolved
smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java
Outdated
Show resolved
Hide resolved
This commit significantly cleans up the JSON schema conversion and automatically inlines primitive references rather than inlining them through a mapper. The previous JSON schema implementation code had several issues: 1. It did not do a good job at handling shape ID conflicts when using namespace stripping. We had to add some pretty bad hacks to achieve this. For examplem, it had implicit state that was tricky to handle (like temporarily setting a ref strategy based on a converted shape index). 2. It didn't detect errors early in the process, resulting in strange errors when you try to use the schema. 3. It exposed too much public API (for example RefStrategy should not be public). Ideally with this trimmed down API surface area, we won't need another breaking change. 4. JSON schema names by default should not include a namespace. 5. Simple shapes by default should always be inlined. Things like list and set shapes aren't that important for generating good JSON schema or OpenAPI schemas. By inlining them, we also ensure that any member documentation attached to members that target list or set shapes isn't lost since that documentation comes from either the member or the targeted shape. This also reduces the possibility for naming conflicts when dropping the namespace from the Smithy shape ID and converting it to JSON Schema. 6. We were dropping member traits in some scenarios like documentation, pattern, range, length. This is now fixed. Because converting shape IDs to JSON pointers can now result in a nested JSON pointer, the ability to select schemas from a SchemaDocument using a JSON pointer has been implemented. Further, the Smithy document shape is actually meant to be a simple type, but it was correctly subclassing SimpleShape, resulting in JSON schema conversions not working correctly (document types were creating distinct named shapes, whereas they are intended to be inlined). Finally, this commit fixes a bug where JSON schema extensions weren't being injected.
5bafc14 to
f6b45a4
Compare
kstich
approved these changes
Feb 14, 2020
Merged
This file contains hidden or 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
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.
This commit significantly cleans up the JSON schema conversion and
automatically inlines primitive references rather than inlining them
through a mapper.
The previous JSON schema implementation code had several issues:
namespace stripping. We had to add some pretty bad hacks to achieve
this. For examplem, it had implicit state that was tricky to handle
(like temporarily setting a ref strategy based on a converted shape
index).
errors when you try to use the schema.
public). Ideally with this trimmed down API surface area, we won't
need another breaking change.
and set shapes aren't that important for generating good JSON
schema or OpenAPI schemas. By inlining them, we also ensure that
any member documentation attached to members that target list or
set shapes isn't lost since that documentation comes from either
the member or the targeted shape. This also reduces the possibility
for naming conflicts when dropping the namespace from the Smithy
shape ID and converting it to JSON Schema.
documentation, pattern, range, length. This is now fixed.
Because converting shape IDs to JSON pointers can now result in a nested
JSON pointer, the ability to select schemas from a SchemaDocument using a
JSON pointer has been implemented.
Finally, the Smithy document shape is actually meant to be a simple type,
but it was correctly subclassing SimpleShape, resulting in JSON schema
conversions not working correctly (document types were creating distinct
named shapes, whereas they are intended to be inlined).
Issue #, if available:
Description of changes:
Note: this will fail due to OpenAPI not being updated yet. That'll follow soon.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.