refactor: implement schemas and all* methods in AsyncAPIDocument class #612
Conversation
magicmatatjahu
requested review from
fmvilas,
jonaslagoni,
derberg and
asyncapi-bot-eve
as code owners
src/models/v2/asyncapi.ts
Outdated
| schemas.add(schema); | ||
| } | ||
| } | ||
| // return only schemas used in channels ("active" schemas) |
There was a problem hiding this comment.
This is raising a hidden behaviour I didn't think when designing the API. Are we sure this is the behavior all users will need? Once i have the "active" schemas, how do I know the ones are not active?
I think we should rather return all schemas and add a filter in the Schemas collection such as FilterByActive() and FilterByInactive or FilterByInUse()/FilterByNotInUse() or whatever.
There was a problem hiding this comment.
If someone will need also inactive schemas can write:
const schemas: Set<SchemaInterface> = new Set();
function callback(schema: SchemaInterface) {
if (!schemas.has(schema.json())) {
schemas.add(schema);
}
}
traverseAsyncApiDocument(this, callback);
const schemas = new Schemas(Array.from(schemas));There was a problem hiding this comment.
Sure they can but I don't think we should take this decision on return only the ones used by channels but rather all of them. Then, thanks to filters, they could filter after all.
There was a problem hiding this comment.
🤷🏼 I don't know if we should take these methods.
There was a problem hiding this comment.
We had a chat on Slack about it. As it ended up making sense, I'm gonna create those filters not only for Schemas but also for Channels, Operations, Messages, and Servers, also a PR to parser-api reflecting the change.
Can you @magicmatatjahu then make this method to return all schemas?
There was a problem hiding this comment.
@smoya After your PR, ok? 😄
There was a problem hiding this comment.
@magicmatatjahu would you have some time to review the PR mentioned in my previous component, so we can unblock this? 🙏
There was a problem hiding this comment.
I think you can adapt the code of this PR to consider #619 is gonna happen so we can merge this right? It is not really blocked in fact.
magicmatatjahu
force-pushed
the
next/schemas-function
branch
from
2ce550f
to
b367745
Compare
magicmatatjahu
changed the title
|
Kudos, SonarCloud Quality Gate passed!
|
|
@smoya I pushed latest changes with |
| const operations: OperationInterface[] = []; | ||
| this.channels().forEach(channel => operations.push(...channel.operations().all())); | ||
| return new Operations(operations); | ||
| return new Operations([]); |
There was a problem hiding this comment.
We don't have operations in components in v2, so we should return empty collection.
| @@ -91,7 +96,60 @@ export class AsyncAPIDocument extends BaseModel<v2.AsyncAPIObject> implements As | |||
| return this.createModel(Components, this._json.components || {}, { pointer: '/components' }); | |||
| } | |||
|
|
|||
| allServers(): ServersInterface { | |||
There was a problem hiding this comment.
This comment applies to all new all* methods.
If I understand correctly, the following servers will be considered equal and only one will be returned:
servers:
myServer: {}
components:
servers:
anotherServer: {}If I'm right, I think the behavior is wrong and the key (stored under metadata.id i think) should be considered as well when comparing.
There was a problem hiding this comment.
In your example you will have two servers, servers.myServer and components.servers.anotherServer. We only compare the data of given object and we base on that filtering of these same "references".
There was a problem hiding this comment.
So we compare the references and not the value of the json object?
There was a problem hiding this comment.
yep, these same references == these same JSON objects
There was a problem hiding this comment.
You can check tests for allSchemas or allMessages :)
There was a problem hiding this comment.
Then, all good! Thanks for the explanation.
|
/rtm |
Description
Implement
schemas,allChannels,allServers,allOperations,allMessages,allSchemasmethods in the AsyncAPIDocument class.cc @smoya
Related issue(s)
Part of #481
Part of #482