asyncapi / spec-json-schemas

AsyncAPI schema versions
Apache License 2.0
53 stars 54 forks source link

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

Open jviotti opened 4 months ago

jviotti commented 4 months ago

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!

jviotti commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Hi @jonaslagoni ,

Once https://github.com/asyncapi/spec-json-schemas/pull/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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 ```sh 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 commented 4 months ago

@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
jviotti commented 4 months ago

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

jviotti commented 4 months ago

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.

sonarcloud[bot] commented 4 months ago

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 commented 4 months ago

This is an interesting issue I discovered while working on this: https://github.com/asyncapi/spec-json-schemas/issues/550. I'm aiming to incorporate these things into my bundling implementation.

jviotti commented 4 months ago

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 commented 1 month ago

@jviotti hi!

Can you resolve conflicts?