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: add binding validation #239

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jul 8, 2022

Description

This PR is a suggestion on how to handle binding versions as well as integrate them into the specification.

Binding versioning

Each binding should now be located as the following folder structure for the json-schema files:

bindings:
  amqp:
    0.2.0:
    x.x.x:
  kafka:
    ...

The binding $ids will now contain the accurate version, so instead of "$id": "http://asyncapi.com/bindings/amqp/channel.json" it will have "$id": "http://asyncapi.com/bindings/amqp/0.2.0/channel.json".

This setup will allow us to work on bindings alongside the spec itself, and continuously release new binding versions.

Integrating with spec

One of the core issues with bindings is that some binding versions might only fit together with specific versions of the spec. To allow for this finetuned control, we can now reference the binding versions exactly as they are compatible and leave out versions when necessary:

    "amqp": {
      "oneOf": [
        {
          "$ref": "http://asyncapi.com/bindings/amqp/0.2.0/channel.json"
        }
      ]
    },

Related issues

Related to asyncapi/spec#590
Related to asyncapi/parser-js#315
Related to asyncapi/bindings#113
Blocked by #289

@jonaslagoni
Copy link
Member Author

Before converting all the bindings to this exact setup, I think we need to agree on the approach beforehand. Tagging code owners and known interested parties: @fmvilas @smoya @derberg @dalelane @magicmatatjahu

@fmvilas
Copy link
Member

fmvilas commented Jul 18, 2022

We considered this setup back then and ditched this idea because it allows having more than one version of a single binding, which is weird. However, since you're using oneOf and the version key is part of the binding, it may make more sense 🤔 Love it because it makes the tooling easier.

Referencing asyncapi/bindings#62 because it's related.

@smoya
Copy link
Member

smoya commented Jul 18, 2022

+1 to this.

A quick clarification, something we should have in mind from now on:

With this approach, the only hard requirement is that JSON Schemas for bindings should contain bindingVersion property. WIthout it, none of the versions included in oneOf could be determined.

As a side note, maybe, we could do a similar script as you @jonaslagoni did for creating a new JSON Schema file for new AsyncAPI spec version but for creating a new binding or a new version of a binding. This could go into a new issue/feature request.

@derberg
Copy link
Member

derberg commented Jul 19, 2022

This looks so simple 😄 I waited few day with review as I was afraid of the complexity...that is not here. Clean 💪🏼

@jonaslagoni
Copy link
Member Author

With this approach, the only hard requirement is that JSON Schemas for bindings should contain bindingVersion property. WIthout it, none of the versions included in oneOf could be determined.

We default to the latest binding version if nothing is specified.

As a side note, maybe, we could do a similar script as you @jonaslagoni did for creating a new JSON Schema file for new AsyncAPI spec version but for creating a new binding or a new version of a binding. This could go into a new issue/feature request.

We should definitely do that yes 🙂

@smoya
Copy link
Member

smoya commented Jul 20, 2022

We default to the latest binding version if nothing is specified.

That could potentially break documents if a new major version of the binding is released. Documents that were working with binding A without specifying the version, assuming the latest was 1.0.0 but suddenly 2.0.0 is released

@jonaslagoni
Copy link
Member Author

That could potentially break documents if a new major version of the binding is released. Documents that were working with binding A without specifying the version, assuming the latest was 1.0.0 but suddenly 2.0.0 is released

Yea you are right, might need to redefine that behavior when introducing this.

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 like a good idea to me 👍

@jonaslagoni jonaslagoni marked this pull request as ready for review July 25, 2022 13:16
@sonarcloud
Copy link

sonarcloud bot commented Jul 25, 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

Alright adapted all the bindings and other changes. Here are the changes I made:

  1. Fixed an issue with binding schemas using an incorrect regex that we solved earlier for all the other core definitions.
  2. Fixed the references of bindings to target 3.0.0 i.e. for AsyncAPI Schema and specification extensions. This might become a problem if a binding version targets multiple major versions i.e. 2.0.0 and 3.0.0 (if the referenced schemas change that is). When referencing version 3.0.0 files we can be sure that the specification extension does not contain any major changes that would resolve in the schemas no longer working for something like 3.3.0.
  3. I have added a specific sunshine test that loads the 3.0.0 schema and an example AsyncAPI document with different binding protocols to test if it actually validates "correctly".
  4. I have adapted the bundler to now load all the binding version schemas so they are accurately bundled together.
  5. I have committed the bundled 3.0.0 schema for the sole purpose that you can see the bundling behavior in effect, even though each release automatically does this anyway.
  6. I have adapted the components section to reference the new binding schemas.

This PR is ready to be reviewed and merged 🙂

@derberg
Copy link
Member

derberg commented Jul 28, 2022

why it targets next-major-spec and not master? I don't see it relying on any changes related to 3.0 spec changes, or did I miss something? we can release it sooner right? just another major, or?

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jul 28, 2022

why it targets next-major-spec and not master? I don't see it relying on any changes related to 3.0 spec changes, or did I miss something? we can release it sooner right? just another major, or?

I actually did not count on integrating the bindings for version 2, which is why I targeted next-major-spec.

The only reason for this is to err on caution, as we are removing a schema bindingsObject.json if we targeted version 2, as it might already be in use.

fmvilas
fmvilas previously approved these changes Jul 29, 2022
@derberg
Copy link
Member

derberg commented Aug 16, 2022

@jonaslagoni sorry, I mean that we do not need to wait for 3.0 as we can just release spec-json-schemas 4.0.0

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Aug 16, 2022

@jonaslagoni sorry, I mean that we do not need to wait for 3.0 as we can just release spec-json-schemas 4.0.0

I know er can do it, but as i said, I did not plan to backtrack this to AsyncAPI 2.x, that said its definitely possible, using the same setup as here.

Are you implying that is what we should do? Or? 😃

@derberg
Copy link
Member

derberg commented Aug 16, 2022

Are you implying that is what we should do? Or? 😃

yeah, kinda 😄

  • it is a great feature, and it would be nice if we ship it sooner to people, get feedback, adjust
  • it is a breaking change. I'm in a club of people that prefer not to introduce too many changes at once, so it is easier to debug errors

but I'm definitely not planning to die on that hill if others don't share my opinion 😃

@jonaslagoni
Copy link
Member Author

Alright so here are the options:

  1. We keep the PR as it is now, and only introduce this change for 3.0.0 (for now, we can decide to do it later if we want to). It will mean we delay when users get access to the binding schemas and it prevents a new major version.
  2. We re-create this PR to target the master branch and release it as a new major version.

@fmvilas @dalelane @derberg those are your possibilities in short 😅 Let me know what you would like to do, I have no strong feelings either other then option 1 is the easiest from my point of view 😆

@smoya
Copy link
Member

smoya commented Aug 17, 2022

I also see more value in releasing this sooner rather than waiting for 3.0. +1 to @derberg 's reasoning. It provides a lot of value.

@fmvilas
Copy link
Member

fmvilas commented Aug 22, 2022

Yeah, don't have a strong opinion either but Lukasz's point makes sense.

dalelane
dalelane previously approved these changes Aug 25, 2022
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.

sooner sounds good to me too 👍

@jonaslagoni jonaslagoni requested review from fmvilas and dalelane and removed request for fmvilas January 11, 2023 10:28
@jonaslagoni
Copy link
Member Author

I have updated the PR with the latest changes on next and added the new pulsar bindings, should be ready for final review ✌️

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.

👍

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 16, 2023

@fmvilas @derberg @smoya @char0n do you want to review it or can we mark it as ready to merge?

@derberg
Copy link
Member

derberg commented Jan 16, 2023

just to clarify the previous discussion. So the final decision is to put it in 3.0 and not sooner?

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 16, 2023

So the final decision is to put it in 3.0 and not sooner?

Tbh... I just don't want to spend the time splitting up the PR into one for next and wait for it to be released, and then add support for it in next-major-spec 😅 Especially considering the time this PR took.

@jonaslagoni
Copy link
Member Author

We can merge this and backtrack it to next if anybody wants to drive it 👍

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 16, 2023

And we still dont know what to do about the specification extension, as it's currently explicitly linked to spec 3.0 in each binding version. So it's not as easy as saying "let's do it", problems will arise when we try to backport it.

Of course, it's something we still have to figure out for the next version anyway, but at least it's not right now that will further delay it.

But in the end it's up to you 🤷

@derberg
Copy link
Member

derberg commented Jan 16, 2023

I do not have super hard feelings about it.
I know that there are people using bindings as they want (not following binding schemas entirely) on production, so releasing these before spec 3.0 will cause them some headaches.

So yeah, let's just merged it for 3.0 only for now

@jonaslagoni
Copy link
Member Author

@derberg I will let you folks decide when adequate enough time has passed for reviewers to respond and for merge it.

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
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

🚀🌔 LGTM!

@derberg
Copy link
Member

derberg commented Jan 20, 2023

@char0n we are just missing your approval

@jonaslagoni
Copy link
Member Author

So yea

@jonaslagoni
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit f5a2346 into asyncapi:next-major-spec Feb 1, 2023
@asyncapi-bot
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

@jonaslagoni jonaslagoni deleted the feature/version_example_for_bindings branch February 2, 2023 22:44
@char0n
Copy link
Collaborator

char0n commented Feb 5, 2023

Great work here! 🎉

Regarding AsyncAPI 2.x.y - I'm assuming that every binding version that is available is allowed to be used for any AsyncAPI 2.x.y version right? Or do we have somewhere the matching map of which binding versions is allowed for what AsyncAPI 2.x.y spec?

@jonaslagoni
Copy link
Member Author

@char0n yea there is still no consensus or progress in the discussion regarding that, as the spec 3.0 changes need to be done first before figuring out what to do about the compatibility between versions of bindings: asyncapi/bindings#182

@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 📦🚀

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.

9 participants