Skip to content

feat: add models and methods for AsyncAPI 3.0.0#86

Closed
magicmatatjahu wants to merge 13 commits into
asyncapi:masterfrom
magicmatatjahu:feat/3.0.0-models
Closed

feat: add models and methods for AsyncAPI 3.0.0#86
magicmatatjahu wants to merge 13 commits into
asyncapi:masterfrom
magicmatatjahu:feat/3.0.0-models

Conversation

@magicmatatjahu

@magicmatatjahu magicmatatjahu commented Feb 9, 2023

Copy link
Copy Markdown
Member

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
Comment thread docs/v1.md
Comment thread docs/v1.md

@smoya smoya left a comment

Copy link
Copy Markdown
Member

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 💯 !

Comment thread docs/v1.md Outdated
- flows(): `OAuthFlows` | `undefined`
- hasOpenIdConnectUrl(): `boolean`
- openIdConnectUrl(): `string` | `undefined`
- scopes(): `string[]` | `undefined`

Copy link
Copy Markdown
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

@smoya smoya Jul 21, 2023

Copy link
Copy Markdown
Member

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

Comment thread docs/v1.md
@smoya

smoya commented Apr 18, 2023

Copy link
Copy Markdown
Member

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

smoya commented Apr 18, 2023

Copy link
Copy Markdown
Member

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

smoya commented Jul 21, 2023

Copy link
Copy Markdown
Member

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

Amzani commented Jul 21, 2023

Copy link
Copy Markdown

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

@smoya

smoya commented Jul 21, 2023

Copy link
Copy Markdown
Member

@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

smoya commented Jul 24, 2023

Copy link
Copy Markdown
Member

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

smoya commented Jul 24, 2023

Copy link
Copy Markdown
Member

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

smoya commented Jul 26, 2023

Copy link
Copy Markdown
Member

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

smoya commented Jul 26, 2023

Copy link
Copy Markdown
Member

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

Comment thread docs/v1.md
@jonaslagoni

Copy link
Copy Markdown
Member

Ping ping 😄

@smoya

smoya commented Aug 10, 2023

Copy link
Copy Markdown
Member

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