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 models and methods for AsyncAPI 3.0.0 #86

Closed

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Feb 9, 2023

Description

Add models and methods for AsyncAPI 3.0.0:

  • title, summary, description, tags, externalDocs and corresponding has* methods for core models
  • filterByTags method for core models
  • reply method in Operation model
  • host and pathname methods in Server model
  • new component methods
  • new OperationReply and OperationReplyAddress model
  • update Readme models
  • fix some models from 2.x.x perspective - add missed methods

cc @smoya @jonaslagoni

@magicmatatjahu magicmatatjahu marked this pull request as ready for review February 9, 2023 10:03
docs/v1.md Show resolved Hide resolved
docs/v1.md Show resolved Hide resolved
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.

Wrote some questions. The rest is 💯 !

docs/v1.md Outdated
@@ -398,6 +452,7 @@
- flows(): `OAuthFlows` | `undefined`
- hasOpenIdConnectUrl(): `boolean`
- openIdConnectUrl(): `string` | `undefined`
- scopes(): `string[]` | `undefined`
Copy link
Member

Choose a reason for hiding this comment

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

This is already in SecurityRequirement

Copy link
Member

@smoya smoya Jul 21, 2023

Choose a reason for hiding this comment

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

I'm removing this because it is already in the security requirement object, so afaik it is not really need here. I know this got simplified in v3 but still compatible. We don't need to mimic exactly what new versions do.

WDYT @derberg @fmvilas @jonaslagoni @magicmatatjahu

docs/v1.md Show resolved Hide resolved
@smoya
Copy link
Member

smoya commented Apr 18, 2023

We should create a lighter version of this PR adding/changing only the necessary for releasing Parser-JS v2. Meaning all features of v3 that are not BC are out.

@smoya
Copy link
Member

smoya commented Apr 18, 2023

We should create a lighter version of this PR adding/changing only the necessary for releasing Parser-JS v2. Meaning all features of v3 that are not BC are out.

Created a PR for this matter #93 @magicmatatjahu

@smoya
Copy link
Member

smoya commented Jul 21, 2023

Those are the changes I recently made to the API.
There are breaking changes but I believe are worth:

  • Additions to Schema object:
    • Add schemaFormat(): string. This holds the schema format after converting to AsyncAPI Schema. It means it will only be application/vnd.aai.asyncapi+json;version=<version>.
    • Add originalSchemaFormat(): string. This holds the original schema format before converting to AsyncAPI Schema. It means it could be any of the formats, including the AsyncAPI one as well.
  • Removal of schemaFormat(): string from Message object. This is BC. This can be fetched from the message.payload().originalSchemaFormat().
    • Alternatives:
      • Change signature to schemaFormat(): string | undefined, because the payload of a message is optional. This will require to add the hasSchemaFormat(): boolean + the methods for the original format such as originalSchemaFormat(): string | undefined.
  • I removed the scopes() method that @magicmatatjahu added to the SecurityScheme since those can be fetched from the SecurityRequirement objects. More on feat: add models and methods for AsyncAPI 3.0.0 #86 (comment)

I need your thoughts on this folks.

cc @fmvilas @derberg @magicmatatjahu @jonaslagoni

@Amzani
Copy link

Amzani commented Jul 21, 2023

@smoya is schemaFormat(): string from Message object. the only breaking change we have ? can this justify pumping the API ?

@smoya
Copy link
Member

smoya commented Jul 21, 2023

@smoya is schemaFormat(): string from Message object. the only breaking change we have ?

Yes, it is as far as I can see.

can this justify pumping the API ?

It is a breaking change since libraries using that method will mostly break.
The alternative I mentioned was changing the return value for a union of string | undefined. That will potentially also break some (maybe not all) libraries anyway.

The reality behind that is that there is no other implementers of this api than our Parser-JS afaik, so in practice it wont really be a big problem if we consider a minor. But technically, it is a breaking change.

@smoya
Copy link
Member

smoya commented Jul 24, 2023

I updated my comment and remove the originalSchema methods so we keep doing the same behaviour until today, that is returning the original schema format right directly when calling schemaFormat().

@smoya
Copy link
Member

smoya commented Jul 24, 2023

I realized no one took care of this https://github.com/asyncapi/parser-js/pull/654/files#r1272712253 and we might want to implement the change here.

cc @fmvilas @magicmatatjahu @jonaslagoni

@smoya
Copy link
Member

smoya commented Jul 26, 2023

In order to reduce the breaking change impact regarding the message.schemaFormat(), I suggest we keep the function and only change the signature to become string | undefined. In that way, current users of message.schemaFormat() won't have a breaking change potentially. I also added the hasSchemaFormat(): boolean for the cases where the payload is empty (so no format is specified).

cc @jonaslagoni

@smoya
Copy link
Member

smoya commented Jul 26, 2023

@jonaslagoni @magicmatatjahu would you mind doing a last review?

@jonaslagoni
Copy link
Member

Ping ping 😄

@smoya
Copy link
Member

smoya commented Aug 10, 2023

Closing in favor of #101

@smoya smoya closed this Aug 10, 2023
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