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: add support for asyncapi 3.0.0 operations and channels #654

Merged
merged 17 commits into from
Oct 26, 2022

Conversation

fmvilas
Copy link
Member

@fmvilas fmvilas commented Oct 13, 2022

This PR contains the following changes:

  • Move all the collections from v2 directory to the models directory because their implementation is always the same. Therefore we can reuse them for v3 and beyond. Check out 0791adc for more details.
  • Adds v3 models to work with v3 operations and channels.
  • Adds npm run dev so it recompiles on file change.

I split all the changes into meaningful commits to make the review more manageable.

@@ -20,7 +21,7 @@ async function operationsV2(parser: Parser, document: AsyncAPIDocumentInterface,
anonymousNaming(document);

if (options.applyTraits) {
applyTraitsV2(detailed.parsed);
applyTraitsV2(detailed.parsed as v2.AsyncAPIObject);
Copy link
Member Author

Choose a reason for hiding this comment

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

You'll find a bunch of these. Since now a DetailedAsyncAPI can be either v2 or v3, we need to make it explicit here.

@@ -3,7 +3,7 @@ import type { DetailedAsyncAPI } from '../types';

export interface ModelMetadata {
asyncapi: DetailedAsyncAPI;
pointer: string;
pointer?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

I found some cases in which pointer will be undefined and had to do this. Happy to hear about alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

In which places? Could you point to them in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines -6 to +7
id(): string;
hasOperationId(): boolean;
operationId(): string | undefined;
id(): string | undefined;
hasId(): boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this because I think it's confusing. An operation shouldn't have an id and an operationId. The operationId is the id of the operation 😅 So yeah, changed it and changed tests too.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍 . Would you mind adding such a change into asyncapi/parser-api#71 ?

Copy link
Member

Choose a reason for hiding this comment

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

id was treated as a key in the collection and not as operationId. The fact that in v3 operationId is mandatory (but not for operations in components - here keys are interpreted as a collection's key and not operationId), this remains a problem with v2 and how we filter collections -

return this.collections.find(operation => operation.id() === id);

Copy link
Member

Choose a reason for hiding this comment

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

We have to think how to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I understand 😅 Just to clarify, in v3 operationId is not mandatory, it just doesn't exist. The id comes from the key in the operations object.

Copy link
Member

Choose a reason for hiding this comment

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

from v2 spec point of view it makes sense and that's the problem 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it does, even from v2 point of view. operationId was there to mean the id of the operation. If it's present, it should be the operation identifier. If it's not, ok, we can generate one if you want, but it was never meant to be an "extra" id.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm 🤔 I have a problem in general because in the v3 version people will be able to define operations in components and how to distinguish this identifier in components.operations[x] from operationId - that's why we have these two methods.id() method is more of a method used to filter operations by identifier (used as key in the components.operations[x] and operations[x]), but... we'll see how to figure it out and id() should return operationId. Can we fix it in next PRs? We need also to fix that in parser-api and remove operationId() method.

To clarify: I wrote about components.operations[x] and operations[x], because in parser-api we have method allOperations() which should return all operations defined in given AsyncAPI document.

Copy link
Member

Choose a reason for hiding this comment

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

I realized no one updated the parser-api with any change regarding this.

Copy link
Member

Choose a reason for hiding this comment

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

All is fixed and there is no need to add anything else.

@fmvilas
Copy link
Member Author

fmvilas commented Oct 13, 2022

BTW, this PR is not yet done. It's still missing some implementations (challenge you to find it 😄) and, of course, tests. I hate writing tests so if someone in their divine presence wants to help this old weary man, please do it by writing some tests.

@jonaslagoni
Copy link
Member

I hate writing tests so if someone in their divine presence wants to help this old weary man

If you really hate writing tests, I can only suggest you try writing them first before the implementation. That way they don't become a chore but a positive experience 😄 At least in theory 😛

@fmvilas
Copy link
Member Author

fmvilas commented Oct 14, 2022

I hate writing tests so if someone in their divine presence wants to help this old weary man

If you really hate writing tests, I can only suggest you try writing them first before the implementation. That way they don't become a chore but a positive experience 😄 At least in theory 😛

Dude, I've been around for a while. Don't preach the TDD shit at me, please ✋ 😝

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Good! Do not forget to refer to my comments and please add unit tests for new v3 models - for channel and operation will be enough.

EDIT: I will write some tests 😄

package.json Show resolved Hide resolved
@@ -3,7 +3,7 @@ import type { DetailedAsyncAPI } from '../types';

export interface ModelMetadata {
asyncapi: DetailedAsyncAPI;
pointer: string;
pointer?: string;
Copy link
Member

Choose a reason for hiding this comment

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

In which places? Could you point to them in this PR?

src/custom-operations/index.ts Outdated Show resolved Hide resolved
src/models/channel-parameters.ts Show resolved Hide resolved
Comment on lines -6 to +7
id(): string;
hasOperationId(): boolean;
operationId(): string | undefined;
id(): string | undefined;
hasId(): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

id was treated as a key in the collection and not as operationId. The fact that in v3 operationId is mandatory (but not for operations in components - here keys are interpreted as a collection's key and not operationId), this remains a problem with v2 and how we filter collections -

return this.collections.find(operation => operation.id() === id);

Comment on lines -6 to +7
id(): string;
hasOperationId(): boolean;
operationId(): string | undefined;
id(): string | undefined;
hasId(): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

We have to think how to handle it.

Comment on lines 53 to 64
operations(): OperationsInterface {
const operations: OperationInterface[] = [];
// ['publish', 'subscribe'].forEach(operationAction => {
// const id = this._json[operationAction as 'publish' | 'subscribe'] && (this._json[operationAction as 'publish' | 'subscribe'] as v2.OperationObject).operationId || `${this.meta().id }_${ operationAction}`;
// if (this._json[operationAction as 'publish' | 'subscribe']) {
// operations.push(
// this.createModel(Operation, this._json[operationAction as 'publish' | 'subscribe'] as v2.OperationObject, { id, action: operationAction as OperationAction, pointer: `${this._meta.pointer}/${operationAction}` }),
// );
// }
// });
return new Operations(operations);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a problem with this implementation? I can help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 😄 #654 (comment)

Now seriously, just time. I wanted to share something as soon as possible for review because this PR was getting too big already.

src/models/v3/channel.ts Outdated Show resolved Hide resolved
Comment on lines 69 to 76
channels(): ChannelsInterface {
const channels: ChannelInterface[] = [];
const pointer = this._json.channel.address ? tilde(`/channels/${this._json.channel.address}`) : undefined;
channels.push(
this.createModel(Channel, this._json.channel, { id: this._json.channel['x-parser-id'], address: this._json.channel.address, pointer })
);
return new Channels(channels);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you doing it this way? Since we define with $ref channel in the operation, we only need to pass it to the collection without additional operations. Only problem is to retrieve proper pointer:

Suggested change
channels(): ChannelsInterface {
const channels: ChannelInterface[] = [];
const pointer = this._json.channel.address ? tilde(`/channels/${this._json.channel.address}`) : undefined;
channels.push(
this.createModel(Channel, this._json.channel, { id: this._json.channel['x-parser-id'], address: this._json.channel.address, pointer })
);
return new Channels(channels);
}
channels(): ChannelsInterface {
const channels = this._meta?.asyncapi?.parsed?.channels || {};
let pointer: string = '';
let channelId: string = '';
for (const [channelName, channel] of Object.entries(channels)) {
if (channel === this._json.channel) {
channelId = channelName;
pointer = `/channels/${channelName}`;
}
}
const channel = this.createModel(Channel, this._json.channel, { id: channelId, pointer })
return new Channels(channel);
}

If there is anything you don't understand then let me know :)

Or I don't understand something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you try this implementation? I have a couple of concerns with this:

  1. You're only checking for existence in this._meta?.asyncapi?.parsed?.channels but the reality is that a $ref can be pointing anywhere, even another file. Actually, most probably another file in many cases.
  2. if (channel === this._json.channel) { is this strict equality gonna work? With === the two have to be the same object instance in memory, right? Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

You're only checking for existence in this._meta?.asyncapi?.parsed?.channels but the reality is that a $ref can be pointing anywhere, even another file. Actually, most probably another file in many cases.

And that's my problem here, because I don't understand if operation's channel should be also included in the channels section of AsyncAPI document. If it won't be, how to check if this channel is involved in the described application? It seems to me that if a given operation is used on a given channel, it should also be defined in the document, even through the ref in the channels section.

if (channel === this._json.channel) { is this strict equality gonna work? With === the two have to be the same object instance in memory, right? Or am I missing something?

Yes, two objects should point to this same memory reference to check equality. We will check for example channel.servers in this same way to check if given server(s) is/are "available" in given application.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I pushed changes to the PR in latest commit - I know that there are quite a lot of changes, but we had a lot of missed methods or models didn't implement correctly interfaces. I will write unit tests and fix sonarcloud issues, don't worry. If you wanna, you can check implementation and ask if you have questions.

@sonarqubecloud
Copy link

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 20 Code Smells

No Coverage information No Coverage information
2.5% 2.5% Duplication

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

@fmvilas Ok, I wrote tests and also fixed all interfaces and corresponding models for v2 and v3 versions. I think we can merge it and fix all errors (especially sonar issues) in next PRs.

Also I changed title of PR to the chore, because:

  • we don't validate new version of spec
  • we don't have all implemented models

so package now is "broken". I hope that you understand :) Of course if you have any questions about implementation, why etc, feel free.

cc @smoya @jonaslagoni @derberg

Comment on lines -6 to +7
id(): string;
hasOperationId(): boolean;
operationId(): string | undefined;
id(): string | undefined;
hasId(): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

from v2 spec point of view it makes sense and that's the problem 😄

return this.action() === 'receive';
}

channels(): ChannelsInterface {
Copy link
Member

@magicmatatjahu magicmatatjahu Oct 24, 2022

Choose a reason for hiding this comment

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

Now that function returns empty collection, because we should discuss asyncapi/spec#856 problem. Also in current implementation that function creates circular references between import:

OperationTrait -> Channel -> Operation -> OperationTrait

I will try to fix it if we will agree what about channel and action in Operation Trait Object.

@magicmatatjahu magicmatatjahu changed the title feat: add support for asyncapi 3.0.0 operations and channels chore: add support for asyncapi 3.0.0 operations and channels Oct 24, 2022
@fmvilas
Copy link
Member Author

fmvilas commented Oct 26, 2022

@magicmatatjahu what do you mean that the package is broken now? Will it fail if I try to simply import it? Or you mean that not everything is implemented?

@magicmatatjahu
Copy link
Member

@fmvilas I wrote:

  • we don't validate new version of spec
  • we don't have all implemented models

so the package is not broken in this sense that you cannot use it, but due to the fact that we don't validate and parse custom schemas and don't apply traits then the returned model is useless and for now there is no point in using it at all, so I would prefer that the release 3.0.0-next-major-specX of parser be done when we will at least have 90% of the models implemented and validation (at least) based on JSON Schema.

@fmvilas
Copy link
Member Author

fmvilas commented Oct 26, 2022

Oh ok, I don't think that's a problem. Yeah, implementation is incomplete but that's expected in this branch.

@magicmatatjahu
Copy link
Member

For me it is also not a big problem to release, but why would anyone need a non-working model? Also simple validation is needed, because the parser-api itself is written in such a way that we have sure that the spec is valid.

@magicmatatjahu
Copy link
Member

@fmvilas So what? Can we merge it?

abongoblob

@fmvilas
Copy link
Member Author

fmvilas commented Oct 26, 2022

For me it is also not a big problem to release, but why would anyone need a non-working model? Also simple validation is needed, because the parser-api itself is written in such a way that we have sure that the spec is valid.

Nobody, except us, is using this version of the parser. It's not just that Parser is incomplete but v3 is also incomplete. It's ok, we're not gonna release it anytime soon 😄

So yeah, I approve it but I created this PR so I'm not sure I should be approving it 😅

@magicmatatjahu
Copy link
Member

@fmvilas I forgot about it 🤣 Sorry!

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Did not see anything jumping out, only smaller formatting issues, but since we have no code formatting configuration it's something that can be changed later 😄

pssst, smaller PRs preferred 😉

@magicmatatjahu
Copy link
Member

@jonaslagoni Thanks!

@magicmatatjahu
Copy link
Member

/rtm

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.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 2.2.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.

5 participants