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

feat!: split out definitions #128

Merged

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Nov 11, 2021

NOTICE: THIS WILL TRIGGER A MAJOR VERSION CHANGE

Description
This PR helps ease the maintainability of the JSON Schema document for the specification since we now have small sub-schemas where the bundling process is now automated.

Changes introduced:

  • All AsyncAPI versions are now fully separated into small sub-schemas.
  • The small sub-schemas are now accurately bundled together to include ALL referenced schemas through the bundler tool. Fixes Provide JSON Schema draft 7 file as part of schemas #120.
  • The generate:assets script that is run on each release triggers the generation of the bundled files, ensuring they are always up to date.
  • Provides a simple script startNewVersion to create new version.
  • We are changing all id's ($id and id properties) to follow http://asyncapi.com/definitions/<version>/<name>.json instead of <name> while providing a migration guide. The reason for this is to follow an accurate bundling behavior from JSON Schema and made the most sense.

Bugs it solves:

  • Specification extension pattern property regex needs to be valid against u flag. This means changing ^x-[\\w\\d\\.\\-\\_]+$ to ^x-[\\w\\d.\\-_]+$.
  • Specification components pattern property regex needs to be valid against u flag. This means changing ^x-[\\w\\d\\.\\-\\_]+$ to ^x-[\\w\\d.\\-_]+$
  • pattern property regex for parameter and correlationId location needs to be valid against u flag. This means changing ^\\$message\\.(header|payload)\\#(\\/(([^\\/~])|(~[01]))*)* to ^\\$message\\.(header|payload)#(\\/(([^\\/~])|(~[01]))*)*
  • Fixes $ref is incorrectly defined for a server.

Related issue(s)
Fixes #164
Fixes #127
Fixes #120

Paired PR's
Spec adapting release process: asyncapi/spec#659
Parser-js updating package and usage: asyncapi/parser-js#423
Website exposing all these files remotely: asyncapi/website#502

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.

@jonaslagoni
Copy link
Member Author

There is one blocker, that I am not quite sure how we solve...

For this to work, the separate schemas need to be bundled together again to provide the full file once again, i.e. automatically bundle all the schemas together to form https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/2.2.0.json.

For now, I have not found any library yet, that accurately provides this behavior... I could implement it myself, as it is quite an easy hack, but I kinda don't want to...

Whenever we do find a solution, we need to create a workflow that automatically bundles the files when changes to the files have been made.

@jonaslagoni
Copy link
Member Author

As the implementation of a bundling behavior is quite simple, lets try to focus on https://github.com/hyperjump-io/json-schema-bundler

It however does have one blocking "bug" - hyperjump-io/json-schema-bundle#2

@jonaslagoni
Copy link
Member Author

@jonaslagoni did you try with https://github.com/APIDevTools/json-schema-ref-parser/blob/main/docs/ref-parser.md#bundleschema-options-callback ?

hmm, I have not. Maybe this fits more our use-case 🤔

@jonaslagoni
Copy link
Member Author

Ignore the request, need to refactor some stuff to make parser compatible.

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Nov 29, 2021

So... The "problem" is that tooling has a very different way of handling the bundling, and I am not quite sure what to choose.

I am not able to get any of the tools to replicate the full AsyncAPI file we currently have. This means that we need to force a major version change. i.e. Our parser expects the schema object to be located under #/definitions/schema, with any of the underlying tools that are not possible.

  1. RefParser inlines everything. Making it one bundled mess, but would be possible to use. See https://github.com/jonaslagoni/spec-json-schemas/tree/230907940ff4c11fb178aff3c94399506f2de4a3
  2. Hyperjump bundler only works with URI $id fields, making it incompatible with the current parser implementation.
    This means we will have to do something like this:
  "patternProperties": {
    "^x-[\\w\\d.\\-_]+$": {
      "$ref": "http://asyncapi.com/draft-07/schemas/specificationExtension"
    }
  },
{
  "additionalItems": true,
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "http://asyncapi.com/draft-07/schemas/specificationExtension"
}

This will bundle it like:

    "definitions": {
        "http://asyncapi.com/draft-07/schemas/specificationExtension": {
            "$id": "http://asyncapi.com/draft-07/schemas/specificationExtension",
            "description": "Any property starting with x- is valid.",
            "additionalProperties": true,
            "additionalItems": true
        },

    }

See https://github.com/jonaslagoni/spec-json-schemas/blob/feature/improve_maintainability/schemas/2.2.0.json.

  1. We could go with our "own" implementation and it would be quite "easy" to do 😄 But another tool, and as bundling behavior have finally been formalized, we would be going against this logic. This would however not require us to do a major version change.

So what do you think we should do? 🤔

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Feb 4, 2022

If https://github.com/jonaslagoni/spec-json-schemas/tree/feature/improve_maintainability/schemas are result of the bundling, product of the release, do we want to commit them into repo? shouldn't these be only in a package and on GitHub release as an additional assets?

Technically no I guess 🤔 Let me try that approach, now that we have npm run build it also makes sense.

Also can you explain why we do http://asyncapi.com/definitions/2.3.0/specificationExtension.json if in the end there is nothing really under the given URL.

@derberg I made a suggestion to expose them, see asyncapi/website#502.

This also means, if the library you use supports fetching references, you don't need the bundled version, as they will be fetchable. Enables more use-cases, such as if you just want to validate a part of the AsyncAPI file 🙂

Is this why you want to mark it as breaking change?

Yes, changing the id's of the schemas is the breaking change 🙂

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Feb 4, 2022

Technically no I guess 🤔 Let me try that approach, now that we have npm run build it also makes sense.

@derberg not quite sure how we should handle the Go library as it expects the schemas to be present, and I dont see a way it can use the bundle library 😅

Reverting.

@smoya
Copy link
Member

smoya commented Feb 7, 2022

@smoya your change in 6b65cb8 I have not applied as I used ^x-[\\w\\d.\\-_]+$.

I am not sure which one to keep?

You should update your documents with the changes I made to the regular expressions. As you can see here, your version is not compliant: https://regex101.com/r/tlOY2C/1

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Can you also configure release candidates setup for next-major?

as for your comment about Go package needs. Actually, we need to move it out, same with NPM package that stores schemas. Have a look at #130

Or we end up with more and more complex monorepo that will require us to break current workflows approach we have. It is doable, we just need to forget about GH releases, and do tags like these folks for example https://github.com/actions/toolkit/tags but does it make sense. Easier for us is to do a split

"generate:assets": "npm run build",
"prepublishOnly": "npm run build",
"bundle": "cd tools/bundler && npm i && npm run bundle",
"startNewVersion": "newVersion=$npm_config_new_version && latestVersion=$(ls -d ./definitions/* | sort -V -r | head -1 | xargs -n 1 basename) && [ -d ./definitions/${newVersion} ] && echo Directory ./definitions/$newVersion already exist and cannot be overwritten. Please create a different version. || (cp -R ./definitions/$latestVersion ./definitions/$newVersion && find ./definitions/$newVersion -name '*.json' -exec sed -i '' \"s+definitions/$latestVersion+definitions/$newVersion+g\" {} +)",
Copy link
Member

Choose a reason for hiding this comment

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

😱 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeea, not sure if we should just leave it, or create something like a script folder? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I definitely recommend moving it out into separate script, it is scary, and in separate script you can add comments 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make sure to split it out in another PR to simplify it 🙂

@sonarcloud
Copy link

sonarcloud bot commented Mar 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jonaslagoni
Copy link
Member Author

Can you also configure release candidates setup for next-major?

Done 👍

as for your comment about Go package needs. Actually, we need to move it out, same with NPM package that stores schemas. Have a look at #130
Or we end up with more and more complex monorepo that will require us to break current workflows approach we have. It is doable, we just need to forget about GH releases, and do tags like these folks for example https://github.com/actions/toolkit/tags but does it make sense. Easier for us is to do a split

Do you want me to remove the Go package now and remove the bundled schema files? Or should that be in another PR?

We are generating the assets on releases, so all we need to do is removing Go package and bundled schema files.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@jonaslagoni this PR is complex enough, so do not remove Go package, especially that NPM package much also be moved out.

@dalelane @fmvilas anything from your side. I think we are ready to merge. It goes to release branch so any overlooked issues will not affect official packages anyway

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

looks good!

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Mar 14, 2022

Thanks for the review folks!

/rtm

@asyncapi-bot asyncapi-bot merged commit 3039448 into asyncapi:next-major Mar 14, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants