refactor: add models for server object #497

Souvikns · 2022-03-16T13:35:59Z

Description

add server model
add unit tests

Related issue(s)

Part of #482

github-actions

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Souvikns · 2022-03-17T12:33:49Z

I am opening this for review, I think I am done with most of the work, will be adding tests now.

magicmatatjahu

Good, but we need some improvements. Please @smoya look at my comments, because we probably should change Parser API.

magicmatatjahu · 2022-03-17T12:59:04Z

src/models/asyncapi.ts

+ servers(): Record<string, ServerInterface>;
+ hasServers(): boolean;
+ serverNames(): Array<string>;
+ server(name: string): ServerInterface;


First one, we don't have such a hasServers method in the https:/asyncapi/parser-api/blob/master/docs/v1.md#asyncapidocument API.

Also I think that we should go with direction like in mixins with rest of methods:

hasServer(name: string): boolean; servers(): ServerInterface[]; servers(name: string): ServerInterface | undefined;

you can check at example https:/asyncapi/parser-js/blob/next-major/src/models/mixins/bindings.ts#L24

@smoya What do you think?

We did an initial implementation in plain JS in this PR #453.
Btw, that PR is like a first implementation draft on the parser-api and we should stick as much as we can with it.

In particular, this is how we solve all the collection of models:

We create an intermediate model, in this case servers which has some methods on it, like isEmpty().
You can see: https:/asyncapi/parser-js/pull/453/files#diff-e871b9dd78d395cf3023273d1db969ee7638c3f5079fb1ac9323a921f46d6016

Sorry I didn't know about that 😆

Yeah, my bad. I created a new issue in parser-api to let parser-api to keep up with that implementation: asyncapi/parser-api#53

magicmatatjahu · 2022-03-17T13:04:40Z

src/models/server-security-requirement.ts

+import { BaseModel } from "./base";
+
+export interface ServerSecurityRequirementInterface extends BaseModel {
+
+}


ServerSecurityRequirementInterface is unnecessary because it should not be a model, but only type https:/asyncapi/spec/blob/master/spec/asyncapi.md#securityRequirementObject

magicmatatjahu · 2022-03-17T13:08:10Z

src/models/server.ts

+ url(): string;
+ protocol(): string;
+ protocolVersion(): string
+ variables(): Record<string, ServerVariableInterface>;
+ variable(name: string): ServerVariableInterface;
+ hasVariables(): boolean;
+ security(): [ServerSecurityRequirementInterface] | undefined;


url and protocol are required, rest of fields are optional, so they should return ... | undefined.

Also I opt to have methods like:

hasVariables(): boolean; hasVariables(name: string): boolean; variables(): Record<string, ServerVariableInterface> | undefined; variables(name: string): ServerVariableInterface | undefined;

cc @smoya

also type like [ServerSecurityRequirementInterface] is a tuple, not array of the items.

Please also read #497 (comment).

Intermediate models are the preferred solution (unless better alternative).

test/models/v2/server.spec.ts

src/models/server.ts

src/models/asyncapi.ts

smoya · 2022-03-18T13:36:55Z

@Souvikns I'm not sure if I mentioned already, if not I'm so sorry, but @jonaslagoni and me did an initial implementation of the new Intent API but in the current plain javascript parser. It is here and has been through several iterations getting feedback: #453

So we would need to port that to TS "mainly".

magicmatatjahu · 2022-03-18T13:39:54Z

@smoya I didn't know about that 😆

…r-js into next/model-server

Souvikns · 2022-03-21T06:00:56Z

I have a question @smoya

In operations.js file here https:/jonaslagoni/parser-js/blob/259446323a31786a867f3714457445278eecbf45/lib/models/operations.js#L26 we have operations.type() but I don't see any implementation of it in https:/jonaslagoni/parser-js/blob/feature%2Fmock_implementation/lib/models/operation.js also I can't find any definition of type in operation object in spec - https:/asyncapi/spec/blob/master/spec/asyncapi.md#operationObject

Can you give me some insight on this?

magicmatatjahu · 2022-03-21T09:34:32Z

@Souvikns type is type of operation. In 2.x.x it will be publish/subscribe, in the 3.x.x it will be send/receive :) You must treat the operation key as a type.

Souvikns · 2022-03-21T10:40:46Z

@magicmatatjahu I read asyncapi/spec#618 now this kind of makes sense, so are we following this proposal for v3 completely (I didn't read all the 77 comments kinda hard to track 😄 ).

The Doubt I have

why does the server have operations? what I get from reading asyncapi/spec#618 is operations is going to reuse channels how does server object has a relation to operation object is there another proposal for this?

magicmatatjahu · 2022-03-21T10:53:42Z

@Souvikns At the moment you should only focus on 2.x.x not on 3.x.x (we don't have any merged PR for 3.x.x yet), so in the ^2.2.x versions you have servers field in the ChannelObjects so you should retrieve all operations defined for given server. Empty servers field means that all operations defined in all channels belong to the server, and vice versa, defined servers means that given channel belongs only to the given server(s) :)

magicmatatjahu · 2022-03-21T11:00:37Z

@Souvikns But atm we don't have implemented operations and other needed models so please skip methods (create only signature) related to them, ok?

Souvikns · 2022-03-21T11:28:17Z

@magicmatatjahu should I be waiting for this #499 since I think I need to use the collection class for the servers class?

magicmatatjahu · 2022-03-21T11:32:47Z

@Souvikns As I know Sergio now is making review, so yes, please wait :)

smoya · 2022-03-21T11:42:33Z

@Souvikns As I know Sergio now is making review, so yes, please wait :)

It is done, and there is no blocker for merging (there is one thing we can change later after merging). So it's just a fact of merging it now 🚀

magicmatatjahu · 2022-03-21T12:26:18Z

Merged :)

…rver

magicmatatjahu

Good, but some things don't work at all and need to be improved and some methods removed because they are unnecessary. I know, ParserAPI says something different, but ParserAPI needs to be changed in several places.

magicmatatjahu · 2022-03-24T10:41:23Z

src/models/v2/server.ts

+ id(): string {
+ return this.json('id');
+ }


We had discussion in Slack that id should not be a part of _json :) I also gave you example how it should work - https:/asyncapi/parser-js/blob/next-major/src/models/v2/mixins/extensions.ts#L11

Suggested change

id(): string {

return this.json('id');

}

constructor(

private readonly _id: string,

_json: Record<string, any>,

) {

super(_json);

}

id(): string {

return this._id;

}

I did this because I modeled the utils function to convert object to array something like this

export function createArrayFromMap(json: Record<string,any>){ const ArrayObject = []; for (const [key, value] of Object.entries(json)) { value['id'] = key; ArrayObject.push(value); }; return ArrayObject; }

so I thought maybe I don't need the add a separate id variable can just get it to format it. Main reason was so that the v2 and v3 have minimal changes, because to add seperate _id parameter now we have to pass something like new Servers(id, this._json)

v3 changes may be much bigger than they are now (to be honest we don't have anything approved so v3=v2 now), so please don't look at this proposal from Fran as something final.

About id. Have in mind that if someone will run json() should get the value that is defined for the object in the specification (not including traits because we have to combine them) without any additional fields that are not defined in the object, so what you want to get with adding the id field is nothing more than changing the original json. I know that adding an id as an argument to a constructor can be weird, but in this case the id is outside the object, and in v3 it can be part of the object and it will be easier to handle.

src/models/v2/server.ts

magicmatatjahu · 2022-03-24T10:45:28Z

src/models/v2/server.ts

+ hasName(): boolean {
+ return !!this.json('name');
+ }
+
+ name(): string | undefined {
+ return this.json('name');
+ }


@smoya what is the name method in the server - https:/asyncapi/parser-api/blob/master/docs/v1.md#server?

The name is the key in the map of servers. But I think we can change it to ID to be consistent?

what is id then?

the key in the map of servers

ok, so name and hasName is out @Souvikns

So then what is id? I was thinking that id is the key

I think it makes sense to use id() for all objects like Channel, Server, Operation, etc, instead of path() for Channels, name for Servers, etc
If we add a ID method for all of those models, no matter if we later change the source of them between versions.
I know the parser-api doesn’t reflect that but we learnt a lot during the last few months :)

src/models/v2/asyncapi.ts

src/models/v3/asyncapi.ts

src/models/v3/server.ts

Souvikns · 2022-03-24T11:03:18Z

I am getting code smell warnings from sonarcloud for semicolons what should I do?

magicmatatjahu · 2022-03-24T11:19:10Z

I am getting code smell warnings from sonarcloud for semicolons what should I do?

as I see, it's from my PR (weird that sonarcloud didn't check it on my PR). I will remove smells from my next PR, no worries.

magicmatatjahu

Overall good, but I see that we have redundant unit tests and also in v2 version we have unnecessary methods. I create suggestions :)

src/models/v2/server.ts

test/models/v2/server.spec.ts

magicmatatjahu · 2022-03-25T11:36:57Z

@Souvikns Good, last thing because I see that sonarcloud fails on duplicated code - please create .sonarcloud.properties file in the root with content:

# Disable specific duplicate code since it would introduce more complexity to reduce it.
sonar.cpd.exclusions=src/models/**/*.ts

and we can merge :)

sonarcloud · 2022-03-25T11:39:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
27 Code Smells

No Coverage information
0.0% Duplication

magicmatatjahu

LGTM! Thanks for contribution! :)

magicmatatjahu · 2022-03-25T12:04:56Z

/rtm

feat: add models for server object

763da46

github-actions bot reviewed Mar 16, 2022

View reviewed changes

Souvikns added 2 commits March 17, 2022 17:21

feat: add v3 server object

6a98d2a

feat: add server related function to asyncapi document

2099076

Souvikns marked this pull request as ready for review March 17, 2022 12:33

Souvikns requested review from fmvilas, magicmatatjahu, jonaslagoni, derberg and asyncapi-bot-eve as code owners March 17, 2022 12:33

magicmatatjahu requested changes Mar 17, 2022

View reviewed changes

refactor: make requested changes

ecaa181

magicmatatjahu requested changes Mar 18, 2022

View reviewed changes

src/models/server.ts Outdated Show resolved Hide resolved

src/models/asyncapi.ts Outdated Show resolved Hide resolved

Souvikns added 2 commits March 18, 2022 16:59

refactor: add suggested changes

6523013

refactor: update asyncapi.ts

20dea91

smoya mentioned this pull request Mar 18, 2022

Migrate project to TypeScript #482

Closed

Souvikns added 2 commits March 21, 2022 09:28

chore: clean up

d4ed90b

Merge branch 'next/model-server' of https:/Souvikns/parse…

68b0982

…r-js into next/model-server

Souvikns added 4 commits March 21, 2022 18:08

Merge remote-tracking branch 'upstream/next-major' into next/model-se…

2330aba

…rver

feat: add server object based on server-api

798db97

feat: add tests

101cad0

refactor: add servers function to asyncapi document

a9fa78c

Souvikns requested a review from magicmatatjahu March 24, 2022 08:46

magicmatatjahu requested changes Mar 24, 2022

View reviewed changes

Souvikns added 2 commits March 24, 2022 19:08

refactor: requested changes

5033ba1

refactor: remove .hasName function as per server-api

2371265

Souvikns requested a review from magicmatatjahu March 25, 2022 08:55

refactor: add id field as per server-api

95c130a

magicmatatjahu requested changes Mar 25, 2022

View reviewed changes

refactor: made suggested changes

356d7b0

feat: add sonarcloud properties

d17d219

magicmatatjahu approved these changes Mar 25, 2022

View reviewed changes

magicmatatjahu changed the title ~~feat: add models for server object~~ refactor: add models for server object Mar 25, 2022

asyncapi-bot added the ready-to-merge label Mar 25, 2022

asyncapi-bot merged commit 875354f into asyncapi:next-major Mar 25, 2022

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

refactor: add models for server object (asyncapi#497)

d749906

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

refactor: add models for server object (#497)

69a0aef

- id(): string {
- return this.json('id');
- }
+ constructor(
+ private readonly _id: string,
+ _json: Record<string, any>,
+ ) {
+ super(_json);
+ }
+ id(): string {
+ return this._id;
+ }

refactor: add models for server object #497

refactor: add models for server object #497

Conversation

Souvikns commented Mar 16, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

Souvikns commented Mar 17, 2022

magicmatatjahu left a comment

Choose a reason for hiding this comment

magicmatatjahu Mar 17, 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

smoya Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

smoya commented Mar 18, 2022

magicmatatjahu commented Mar 18, 2022

Souvikns commented Mar 21, 2022

magicmatatjahu commented Mar 21, 2022

Souvikns commented Mar 21, 2022

The Doubt I have

magicmatatjahu commented Mar 21, 2022

magicmatatjahu commented Mar 21, 2022

Souvikns commented Mar 21, 2022

magicmatatjahu commented Mar 21, 2022

smoya commented Mar 21, 2022

magicmatatjahu commented Mar 21, 2022

magicmatatjahu 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

smoya Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

magicmatatjahu Mar 24, 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

smoya Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Souvikns commented Mar 24, 2022

magicmatatjahu commented Mar 24, 2022 • edited Loading

magicmatatjahu left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Mar 25, 2022

sonarcloud bot commented Mar 25, 2022

magicmatatjahu left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Mar 25, 2022

magicmatatjahu Mar 17, 2022 •

edited

Loading

smoya Mar 18, 2022 •

edited

Loading

smoya Mar 24, 2022 •

edited

Loading

magicmatatjahu Mar 24, 2022 •

edited

Loading

smoya Mar 24, 2022 •

edited

Loading

magicmatatjahu commented Mar 24, 2022 •

edited

Loading