refactor: port message, operation, correlation-id and security-requirements models #542

magicmatatjahu · 2022-05-04T14:31:25Z

Description

port Message, MessageTrait, Operation, OperationTrait, CorrelationId and SecurityRequirements models
add unit tests

Related issue(s)
Part of #481
Part of #482

cc @smoya @Souvikns

jonaslagoni

👍

Souvikns

I haven't read all the files

smoya · 2022-05-05T07:53:58Z

src/models/channel-parameter.ts

-export interface ChannelParameterInterface extends BaseModel {}
+export interface ChannelParameterInterface extends BaseModel, DescriptionMixinInterface, ExtensionsMixinInterface {
+  id(): string;
+  hasSchema(): boolean;


If this should stay, should be added to https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#channelparameter

smoya · 2022-05-05T07:54:24Z

src/models/channel-parameter.ts

+  hasSchema(): boolean;
+  schema(): SchemaInterface | undefined;
+  hasLocation(): boolean;
+  location(): string | undefined;


Same as before, If this should stay, should be added to https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#channelparameter

The channel parameter has a location so it has to stay 😄

Would you mind then creating a PR on Parser-API repo?

smoya · 2022-05-05T08:09:27Z

src/models/message-trait.ts


-export interface MessageTraitInterface extends BaseModel {}
+export interface MessageTraitInterface extends BaseModel, BindingsMixinInterface, DescriptionMixinInterface, ExtensionsMixinInterface, ExternalDocumentationMixinInterface, TagsMixinInterface {


There are some missing methods from the Parser-API that I think should be implemented here (otherwise, discarded from the Parser-API).

hasKnownSchemaFormat() : boolean

schemaFormat() : string

See https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#message

I didn't add it on purpose for a simple reason. Defining schemaFormat at the messag level on a lot of problems from a schema reuse point of view, and I will try to move schemaFormat to the Schema object - then this will be removed. Here is my issue for 3.0.0 asyncapi/spec#622. If we want it I can add it, but I hope that in 99% we will remove it.

It makes sense indeed. How would you implement that? I think this should be then done in this PR

I can add schemaFormat in this PR to Message model, but we can always remove it.

Yeah, makes sense.

I like the message.payload().schemaFormat() you suggested in asyncapi/spec#622. Maybe we can move forward with it later on, and now just adding an issue to work on it next?

Issue :) #544

smoya · 2022-05-05T08:15:30Z

src/models/operation-trait.ts

-export interface OperationTraitInterface extends BaseModel {}
+export interface OperationTraitInterface extends BaseModel, BindingsMixinInterface, DescriptionMixinInterface, ExtensionsMixinInterface, ExternalDocumentationMixinInterface, TagsMixinInterface {
+  id(): string;
+  kind(): OperationKind;


This should be added into the Parser-API https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#operation, as well as removing some of the methods related to Publish/Subscribe

Also, kind VS type. For example, in Security Schemes, we use type https://github.com/asyncapi/parser-js/pull/542/files#diff-dfe2dcce2b8c529ef868faf21548e2ba16163b0c4b9ef33129ac286346f2e008R15

You're right, we have inconsistencies. type in SecurityRequirements and here kind. In 3.0.0 we have for example called it as action (asyncapi/spec#618) so maybe we should call it action?

Indeed, action sounds like a better fit.

smoya · 2022-05-05T08:23:40Z

src/models/v2/message-trait.ts

+  }
+
+  id(): string {
+    return this.messageId() || this._id;


Noice!!! 👍

Here I have a problem, because on one side we call it id, and next to it we have messageId. Maybe we should rename this method to uid and for id return the value for messageId? Same for other objects - where we call the method id we should call it uid?

The messageId is the real unique id (uid) across the document, so I think it would be the other way around.
Anyway, I see the issue you are mentioning.
Maybe, adding a new UniqueIdentifiableInterface, with the uid method would make sense. In the case of message, to return this.messageId(), this.operationId for Operation, etc.
The id field will then just return the key of the map which the object belongs to.

Makes sense?

It makes sense, but I wonder if we need 3 methods, messageId, id and uid.

The id could be for messageId
The uid for the messageId or key or some generated unique id for given reference.

I think we are considering a different thing for what the id value is.

From my pov, id is just the key on the map. For example, myMessage in the following example:

components: messages: myMessage: # etc

Of course, currently messages declared inside the operation won't have an id (at least on spec v2.x) but this could change for next versions (and makes sense to me).

smoya · 2022-05-05T08:31:02Z

src/models/v3/security-requirements.ts

+
+export class SecurityRequirements extends Collection<SecurityRequirementInterface> implements SecurityRequirementsInterface {
+  override get(id: string): SecurityRequirementInterface | undefined {
+    // return this.collections.find(trait => trait.id() === id);


Is this a TODO?

Yes, and no. Because of the fact that these objects do not have id I wonder if we should return undefined, or change this collection to a regular array and not collection class.

Good point. Can't we do just directly use Record<string, { schema: SecuritySchemeInterface, scopes: Array<string> }> ?

We cannot, security field (in security and operation) is an array, so we ca change it to the

Array<Record<string, { schema: SecuritySchemeInterface, scopes: Array<string> }>>

I think makes sense

smoya

Mainly looks good to me! 🎉 Left some comments and suggestions

sonarcloud · 2022-05-05T11:06:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

magicmatatjahu · 2022-05-05T11:07:04Z

@smoya I propagated suggestions from your review. I didn't add uid and stay with id and messageId. I think that now it's good approach, we can always change it when we will see how 3.0.0 will look like.

I also updated parser-api asyncapi/parser-api#64

smoya

Super great work @magicmatatjahu! 🚀 🌔 I approve it but I'm not code owner so...

magicmatatjahu · 2022-05-06T07:58:40Z

/rtm

…ements models (asyncapi#542)

…ements models (#542)

magicmatatjahu added 2 commits April 29, 2022 15:36

refactor: port message model

37cac0f

add security-requirements objects

1798972

magicmatatjahu added the enhancement New feature or request label May 4, 2022

magicmatatjahu requested review from fmvilas, jonaslagoni, derberg and asyncapi-bot-eve as code owners May 4, 2022 14:31

jonaslagoni approved these changes May 4, 2022

View reviewed changes

add jest coverage

35f1b95

Souvikns approved these changes May 4, 2022

View reviewed changes

smoya reviewed May 5, 2022

View reviewed changes

smoya requested changes May 5, 2022

View reviewed changes

magicmatatjahu mentioned this pull request May 5, 2022

docs: update parser-api asyncapi/parser-api#64

Merged

after review

ebba039

magicmatatjahu mentioned this pull request May 5, 2022

[2.0.0] Add schemaFormat method to the Schema model #544

Closed

smoya approved these changes May 5, 2022

View reviewed changes

asyncapi-bot added the ready-to-merge label May 6, 2022

asyncapi-bot merged commit f70b5c0 into asyncapi:next-major May 6, 2022

magicmatatjahu deleted the next/message-model branch May 6, 2022 07:59

magicmatatjahu mentioned this pull request May 19, 2022

Road to v2.0.0 #481

Closed

20 tasks

magicmatatjahu added a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022

refactor: port message, operation, correlation-id and security-requir…

9c339ec

…ements models (asyncapi#542)

derberg pushed a commit that referenced this pull request Oct 4, 2022

refactor: port message, operation, correlation-id and security-requir…

3627d02

…ements models (#542)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: port message, operation, correlation-id and security-requirements models #542

refactor: port message, operation, correlation-id and security-requirements models #542

magicmatatjahu commented May 4, 2022 •

edited

Loading

jonaslagoni left a comment

Souvikns left a comment

smoya May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

smoya May 5, 2022 •

edited

Loading

magicmatatjahu May 5, 2022

smoya May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

magicmatatjahu May 5, 2022

smoya May 5, 2022

smoya left a comment

sonarcloud bot commented May 5, 2022

magicmatatjahu commented May 5, 2022 •

edited

Loading

smoya left a comment •

edited

Loading

magicmatatjahu commented May 6, 2022


		export interface MessageTraitInterface extends BaseModel {}
		export interface MessageTraitInterface extends BaseModel, BindingsMixinInterface, DescriptionMixinInterface, ExtensionsMixinInterface, ExternalDocumentationMixinInterface, TagsMixinInterface {

refactor: port message, operation, correlation-id and security-requirements models #542

refactor: port message, operation, correlation-id and security-requirements models #542

Conversation

magicmatatjahu commented May 4, 2022 • edited Loading

jonaslagoni left a comment

Choose a reason for hiding this comment

Souvikns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoya May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoya left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 5, 2022

magicmatatjahu commented May 5, 2022 • edited Loading

smoya left a comment • edited Loading

Choose a reason for hiding this comment

magicmatatjahu commented May 6, 2022

magicmatatjahu commented May 4, 2022 •

edited

Loading

smoya May 5, 2022 •

edited

Loading

magicmatatjahu commented May 5, 2022 •

edited

Loading

smoya left a comment •

edited

Loading