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

style: perform schema formatting with the new JSON Schema CLI #549

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jviotti
Copy link

@jviotti jviotti commented Jun 11, 2024

Hey there from a JSON Schema TSC and ex-Postman! We are developing an open-source CLI (https://github.com/intelligence-ai/jsonschema) specifically targeted at helping maintain repositories of schemas, just like this one. The idea is to make it super smooth to work with schemas.

The tool is already capable of doing formatting, linting (which revals a couple of issues already in this repo), testing, bundling, and more, which can replace a few of the tools and scripts you already have here.

Instead of sending a big PR, here is a small one just making use of formatting. The formatting implementation will re-organize keywords in a schema to make them easier to read. For example, bumping $schema to the top, ensuring consistent, indentation, etc.

If you like it, I'd love to continue working together to integrate more things, like the linter, the schema test framework, etc.

Let me know what you think and if you have any requirement or idea, please let me know and we'll happily implement it for you! We want to make it super smooth to maintain repos like this one, so any feedback is very welcomed!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@jviotti jviotti changed the title Integrate with the Intelligence.AI JSON Schema CLI for formatting style: perform schema formatting the Intelligence.AI JSON Schema CLI Jun 11, 2024
@jviotti
Copy link
Author

jviotti commented Jun 11, 2024

The diff is huge, as all the schemas are getting formatted, but no semantics are changed. Just indentation and keyword ordering. At least they seem much nicer to the eye already!

@jviotti
Copy link
Author

jviotti commented Jun 11, 2024

I also added this repo to my https://github.com/sourcemeta/awesome-jsonschema list as a showcase, as its doing quite interesting things with schemas.

@jviotti jviotti changed the title style: perform schema formatting the Intelligence.AI JSON Schema CLI style: perform schema formatting with the Intelligence.AI JSON Schema CLI Jun 11, 2024
@jviotti jviotti changed the title style: perform schema formatting with the Intelligence.AI JSON Schema CLI style: perform schema formatting with the new JSON Schema CLI Jun 11, 2024
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to integrate this 🙏

Once #540 is merged, I think this is a good follow-up PR. If the test succeed for v3, I think it's safe to assume that none of the other versions got corrupted by this formatting.

I see the library is v0.5, what is the current state of it? 🤔

@jviotti
Copy link
Author

jviotti commented Jun 11, 2024

Hi @jonaslagoni ,

Once #540 is merged, I think this is a good follow-up PR. If the test succeed for v3, I think it's safe to assume that none of the other versions got corrupted by this formatting.

Sounds like a plan. Let me know when the other one is merged!

I see the library is v0.5, what is the current state of it?

Very actively working on it and hoping to reach v1 extremely soon (i.e. by the end of the week). We are using it ourselves plus we have some initial users like https://github.com/krakend/krakend-schema and it was promoted by JSON Schema on social media just today (https://www.linkedin.com/feed/update/urn:li:activity:7206211519922028544/).

While the CLI is relatively new, it is based on my battle tested ~2 year old implementation (https://github.com/sourcemeta/jsontoolkit) that is running on production.

@smoya
Copy link
Member

smoya commented Jun 12, 2024

@jviotti I really like the idea of the tool; having all our must-have tools for our JSON Schema schemas in once.

I +1 what @jonaslagoni said earlier. BTW, I've been testing the validate command with our current schemas and I couldn't make it work. All seem valid even though I add changes that should make it complain (such as specifying an invalid type foobar).

This is the command I ran, which I understand is enough for validating against the metaschema:

jsonschema validate schemas/3.0.0.json

@Pakisan
Copy link
Member

Pakisan commented Jun 12, 2024

@smoya @jonaslagoni I'll assign it to may self, if you don't mind, to not forget to check after #540

@jviotti awesome work man, can you reduce changes only to v3.0.0? I'm ready to review schemas and test them

@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

Thanks for all your comments! I'll address all of them later today.

@smoya

BTW, I've been testing the validate command with our current schemas and I couldn't make it work. All seem valid even though I add changes that should make it complain

Ah, good catch. Looks like it's still requiring --metaschema to be present. I'll send a quick fix today. While the underlying JSON Schema library is mature, the CLI interface is relatively out of oven, so this kind of feedback really helps polishing it fast! 🙏🏻

@Pakisan

can you reduce changes only to v3.0.0? I'm ready to review schemas and test them

I will!

@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

@smoya

I really like the idea of the tool; having all our must-have tools for our JSON Schema schemas in once.

Talking about that, I'm also working on extending the current bundling implementation to optionally also do bundling without relying on $id (which you do here due to some limited tooling out there), which hopefully can replace some other scripts you have in the repo :)

@smoya
Copy link
Member

smoya commented Jun 12, 2024

Ah, good catch. Looks like it's still requiring --metaschema to be present.

Yeah, I noticed that when passing --metaschema seems to try validating. However, It shows errors where other validators don't 🤔 For example, validating the schemas/3.0.0.json:

Show validation output
error: The target document is expected to be one of the given values
    at instance location "/definitions/http:~1~1asyncapi.com~1bindings~1jms~10.0.1~1server.json/definitions/property/properties/value/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/type/anyOf/0/$ref/enum"
error: Mark the current position of the evaluation process for future jumps
    at instance location "/definitions/http:~1~1asyncapi.com~1bindings~1jms~10.0.1~1server.json/definitions/property/properties/value/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/type/anyOf/0/$ref"
error: The target document is expected to be one of the given values
    at instance location "/definitions/http:~1~1asyncapi.com~1definitions~13.0.0~1channel.json/properties/address/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/type/anyOf/0/$ref/enum"
error: Mark the current position of the evaluation process for future jumps
    at instance location "/definitions/http:~1~1asyncapi.com~1definitions~13.0.0~1channel.json/properties/address/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/type/anyOf/0/$ref"
error: The target document is expected to be of one of the given types
    at instance location "/definitions/http:~1~1asyncapi.com~1definitions~13.0.0~1messageObject.json/properties/traits/items/oneOf/2/items"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/items/anyOf/0/$ref/properties/oneOf/$ref/items/$ref/properties/items/anyOf/0/$ref/type"
error: Jump to another point of the evaluation process
    at instance location "/definitions/http:~1~1asyncapi.com~1definitions~13.0.0~1messageObject.json/properties/traits/items/oneOf/2/items"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/properties/additionalProperties/$ref/properties/items/anyOf/0/$ref/properties/oneOf/$ref/items/$ref/properties/items/anyOf/0/$ref"
error: The target document is expected to be one of the given values
    at instance location "/definitions/http:~1~1json-schema.org~1draft-07~1schema/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/type/anyOf/0/$ref/enum"
error: Mark the current position of the evaluation process for future jumps
    at instance location "/definitions/http:~1~1json-schema.org~1draft-07~1schema/type"
    at evaluate path "/properties/definitions/additionalProperties/$ref/properties/type/anyOf/0/$ref"
error: The target document is expected to be one of the given values
    at instance location "/type"
    at evaluate path "/properties/type/anyOf/0/$ref/enum"
error: Mark the current position of the evaluation process for future jumps
    at instance location "/type"
    at evaluate path "/properties/type/anyOf/0/$ref"
error: The target document is expected to be of the given type
    at instance location "/type"
    at evaluate path "/properties/type/anyOf/1/type"
error: The target is expected to match at least one of the given assertions
    at instance location "/type"
    at evaluate path "/properties/type/anyOf"
error: The target is expected to match all of the given assertions
    at instance location ""
    at evaluate path "/properties"
The schema is NOT valid with respect to its metaschema

Json Schema Validator online says it's OK (same with hyperjump): https://www.jsonschemavalidator.net/s/eoRmFDzn

@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

@smoya

However, It shows errors where other validators don't 🤔

Oh, that's an output issue. The CLI is outputting errors from internal anyOf non matching branches even if the outer schema does validate well. I'll clean up the CLI output when dealing with anyOf so that it doesn't print unnecessary warnings that are not real errors.

As you can see on my prompt, the exit code is still 0 (success):

Screenshot 2024-06-12 at 11 20 31 AM

Hey there from a JSON Schema TSC and ex-Postman! We are developing an
open-source CLI (https://github.com/intelligence-ai/jsonschema)
specifically targeted at helping maintain repositories of schemas, just
like this one. The idea is to make it super smooth to work with schemas.

The tool is already capable of doing formatting, linting (which revals a
couple of issues already in this repo), testing, bundling, and more,
which can replace a few of the tools and scripts you already have here.

Instead of sending a big PR, here is a small one just making use of
formatting. The formatting implementation will re-organize keywords in a
schema to make them easier to read. For example, bumping `$schema` to
the top, ensuring consistent, indentation, etc.

If you like it, I'd love to continue working together to integrate more
things, like the linter, the schema test framework, etc.

Let me know what you think and if you have any requirement or idea,
please let me know and we'll happily implement it for you! We want to
make it super smooth to maintain repos like this one, so any feedback is
very welcomed!

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

@Pakisan I updated the PR to only format definitions/3.0.0! Please let me know what you think!

jviotti added a commit to sourcemeta/jsonschema that referenced this pull request Jun 12, 2024
jviotti added a commit to sourcemeta/jsonschema that referenced this pull request Jun 12, 2024
…ances (#82)

See: asyncapi/spec-json-schemas#549
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
jviotti added a commit to sourcemeta/jsonschema that referenced this pull request Jun 12, 2024
jviotti added a commit to sourcemeta/jsonschema that referenced this pull request Jun 12, 2024
@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

I released v0.5.1 fixing both the --metaschema issue and improving the validate output: https://github.com/Intelligence-AI/jsonschema/releases/tag/v0.5.1. Thanks a lot for the extremely valuable feedback! 🙏🏻

I'll be running additional testing on your (big!) schemas just to rule out any other potential issue.

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jviotti
Copy link
Author

jviotti commented Jun 12, 2024

This is an interesting issue I discovered while working on this: #550. I'm aiming to incorporate these things into my bundling implementation.

@jviotti
Copy link
Author

jviotti commented Jun 22, 2024

Still working hard on this one. We are sorting out various known issues and improving our validation output. I'll resume this PR hopefully very soon.

@Pakisan
Copy link
Member

Pakisan commented Sep 13, 2024

@jviotti hi!

Can you resolve conflicts?

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.

4 participants