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

chore: merge master into next-major-spec + fix bug in bundler #406

Merged
merged 33 commits into from
Jul 11, 2023

Conversation

smoya
Copy link
Member

@smoya smoya commented Jun 30, 2023

Description

This PR, besides merging master into next-major-spec, does the following:

  • Fixes a bug in the bundler to -without-$id where $ref pointing to definitions without .json extension fail. For example, "$ref": "http://asyncapi.com/definitions/3.0.0/operation.json#/properties/title" like the OperationTrait in v3 has now
  • It adds support for bindings $id to without-$id transformation. For example http://asyncapi.com/bindings/mqtt/0.1.0/operation.json to bindings-mqtt-0.1.0-operation.

Both matching are now done through regex, since there is no easy silver bullet solution without doing tons of hacks.

Pinging @derberg who made the changes in the bundler for creating schemas without $id. So I expect you can review my last commits where the fix is done.

Warning
Manually merge and not rebase!

asyncapi-bot and others added 29 commits March 28, 2023 09:37
Co-authored-by: asyncapi-bot <info@asyncapi.io>
…i#298)

Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
…release/github, @semantic-release/npm and semantic-release (asyncapi#381)
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: asyncapi-bot-eve <bot+eve@asyncapi.io>%0ACo-authored-by: asyncapi-bot <info@asyncapi.io>
@smoya smoya requested a review from fmvilas as a code owner June 30, 2023 16:50
@smoya smoya changed the title Merge master into next-major-spec + fix bug in bundler chore: merge master into next-major-spec + fix bug in bundler Jun 30, 2023
@smoya
Copy link
Member Author

smoya commented Jun 30, 2023

@derberg @jonaslagoni I need some help debugging what's the issue with the produced 3.0.0-without-$id.json.
Here you can find the validator error: https://www.jsonschemavalidator.net/s/3OO3lvSb

Could not resolve schema reference '#/operation#/properties/title'. Path 'definitions.operationTrait.properties.title', line 4675, position 26.

However, in the 3.0.0.json file, things like "$ref": "http://asyncapi.com/definitions/3.0.0/operation.json#/properties/title" do work.

@derberg
Copy link
Member

derberg commented Jul 5, 2023

Interesting, it is only with operationTrait, not a serious stuff imho

the oryginal schema has http://asyncapi.com/definitions/3.0.0/operation.json#/properties/title and the no-id bundler is dummy and replace http://asyncapi.com/definitions/3.0.0/operation.json with a local reference. So simple fix is just have an if that if there is a url with anchors, then not only http://asyncapi.com/definitions/3.0.0/operation.json but http://asyncapi.com/definitions/3.0.0/operation.json# should be replaced new local reference so it looks like "$ref": "#/definitions/operation/properties/title"

@smoya
Copy link
Member Author

smoya commented Jul 11, 2023

"$ref": "#/definitions/operation/properties/title"

@derberg The issue is with JSON pointer here and the fact we should scape / characters with ~1, which doesn't look pretty good from DX perspective.

I decided to use - instead now (See the last commit I pushed). For example,http://asyncapi.com/bindings/http/0.1.0/message.json in regular file is bindings-http-0.1.0-message in -without-$id file.

I also added support for testing and validating the schemas without $id which was not added before.

@derberg
Copy link
Member

derberg commented Jul 11, 2023

@smoya I don't get why you need ~1? we use it in parser-js only if we need to handle encoding of / in the channel names that may have it. But in case of stuff like "#/definitions/operation/properties/title" there is no need to do any tilde encoding really 🤔

@smoya
Copy link
Member Author

smoya commented Jul 11, 2023

@smoya I don't get why you need ~1? we use it in parser-js only if we need to handle encoding of / in the channel names that may have it. But in case of stuff like "#/definitions/operation/properties/title" there is no need to do any tilde encoding really 🤔

My bad for the confusing explanation. Gonna try again.

1. Bug with #/operation#/properties/title

The issue was that the extra # was not required. That's fixed, and now it is #/operation/properties/title.

2. Bug with definition ids with / characters

Ids can have extra / characters. That's the case for the bindings, for example, where you can find things like http://asyncapi.com/bindings/mqtt/0.1.0/operation.json.
If we convert that to bindings/mqtt/0.1.0/operation, then references will need to use ~, such as "#/definitions/bindings~1mqtt~10.1.0~1operation". Otherwise, the JSON Pointer will try to find in the path definitions.bindings.mqtt.0.1.0.operation. Meaning it will only work if a definition like the following is in the file (which is not our case):

 "bindings": {
            "mqtt": {
                "0.1.0": {
                    "operation": {
                        "type": "object"
                    }
                }   
            }
        },

@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 2023

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

@derberg
Copy link
Member

derberg commented Jul 11, 2023

"0.1.0

ah yeah, makes a lot of sense

@derberg derberg merged commit e1c1bbc into asyncapi:next-major-spec Jul 11, 2023
11 checks passed
@asyncapi-bot
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.0.0 🎉

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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants