-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
refactor: add filterByInUse()
and filterByNotInUse()
to collections
#619
Conversation
d6452aa
to
5cf5e6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand, even if you access the model, say you access it from components, it will still say it's in use? (if it is of course)
At least in my mind, something being used means is declared outside components or rather inside but referenced in the doc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright 👍
src/models/collection.ts
Outdated
filterByInUse(): T[] { | ||
return this.filterBy((item: T): boolean => { | ||
// In the case of using $ref to reference any item from components, it will be already resolved since references are resolved before parsing, and will use another pointer rather than /components/... | ||
return !item.meta().pointer.startsWith('/components'); | ||
}); | ||
} | ||
|
||
filterByNotInUse(): T[] { | ||
return this.filterBy((item: T): boolean => { | ||
// In the case of using $ref to reference any item from components, it will be already resolved since references are resolved before parsing, and will use another pointer rather than /components/... | ||
return item.meta().pointer.startsWith('/components'); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't problem with these methods on Collection
level, but they have only logic sense in Operations
, Servers
, Channels
, Messages
and Schemas
collections, so I would opt to create from these methods the mixins (helper functions) like others, for tags etc. https://github.com/asyncapi/parser-js/blob/next-major/src/models/v2/mixins.ts and apply them only for mentioned collections. Also, we should remember that your logic based on checking pointer
is only sustainable for schemas()
and messages()
methods in AsyncAPIDocument level, of course atm - rest collections will be affected in this way that only one method will return array with all items, second one empty (I don't know if method's comment points to this situation, is it?). Also related to last sentence, probably only Schemas
and Messages
collection should have these methods, for channels, operations, servers the method filterByNotInUse
will return always empty array. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't problem with these methods on Collection level, but they have only logic sense in Operations, Servers, Channels, Messages and Schemas collections, so I would opt to create from these methods the mixins (helper functions) like others, for tags etc. https://github.com/asyncapi/parser-js/blob/next-major/src/models/v2/mixins.ts and apply them only for mentioned collections.
This is a really good point. I applied all the changes regarding this so now those filters are in a mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should remember that your logic based on checking pointer is only sustainable for schemas() and messages() methods in AsyncAPIDocument level, of course atm - rest collections will be affected in this way that only one method will return array with all items, second one empty (I don't know if method's comment points to this situation, is it?). Also related to last sentence, probably only Schemas and Messages collection should have these methods, for channels, operations, servers the method filterByNotInUse will return always empty array. WDYT?
I think the current shape of the AsyncAPI document are strongly biasing us during this discussion, but we should remember the parser-api
has nothing to do with the shape of the document but rather with intents. It should be resilient enough to adapt breaking changes on that shape.
I also missed a point here that is very important. I believe all methods on the root document such as root.channels()
, root.operations()
, root.messages()
, etc return all objects from around the document (including those in components). On the contrary, as the perspective is always from the Application, those methods located in other places rather than root will return objects associated with such collection, for example root.operations()[0].channels()
will return channels associated to that particular operation.
See the following examples:
- Channels from root:
root.channels().all()
(method that already exists) -> returns channels across the whole documentroot.channels().filterByInUse()
-> returns channels used by the applicationroot.channels().filterByNotInUse()
-> returns channels not used by the application
- Channels from components:
components.channels().all()
(method that already exists) -> returns all channels defined as components.components.channels().filterByInUse()
-> returns channels used by the applicationroot.channels().filterByNotInUse()
-> returns channels not used by the application
- Servers from channels defined in the application (not as components):
root.channels()[0].servers().all()
(method that already exists) -> returns servers associated to a particular channel.root.channels()[0].servers().filterByInUse()
-> this will return the same asall()
in this case.root.channels()[0].servers().filterByNotInUse()
-> this will return an empty array.
In terms of implementation, we could use (correct me if I'm wrong @magicmatatjahu) the same strategy as you did here comparing the JSON payload to determine if an object is isolated only in components or used by the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the examples from my previous comment based and trying to answer an use case @magicmatatjahu shared with me via Slack:
the method channel.severs().all() what should it return at a glance? In this case all() means return servers from channel.severs() not all from document. Meaning of all from your perspective has only sense in root level
only that channel can have references to the servers that are used (in v3), relative to the document but the channel itself can only be defined in components::
servers:
server:
$ref: '#/components/servers/someServer'
components:
servers:
someServer: ...
channels:
channel:
servers:
$ref: '#/components/servers/someServer'
so someServer is used, so these filterByNotInUse and filterByInUse from which perspective looking on objects?
I mean from calling method channel.severs().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this over the weekend and still opt for that methods channels()
, messages()
, servers()
etc which are on the root should always return "used" elements without specifying whether something is used or not, i.e. concatenation with elements defined in components but which are not used. Let's not complicate the lives of normal users.
At the moment I see only one use case where someone would like to retrieve suddenly all the elements of a given type - Modelina which needs all schemas for model generation, but I do not think we need to add the given methods in each collection, because it is not necessary. Also in the Studio itself, even the active/inactive element will not be needed, because the left navigation should show used elements and not the collection of available elements - there should be a separate components
section to show reusable objects to reference.
I think we should "freeze" this feature, create a separate issue describing what we want to add and see if people need it and that's it. That feature has a lot of weird edge cases though like the one above what Sergio pasted from our Slack conversation. If it will be breaking change (and it will be) then we'll release v3 of ParserJS. As I wrote, except for Modelina I don't know of a case of any tool needing such logic - but this use case can be written in one line to retrieve all the schemas.
In addition, I spoke with Sergio on Friday and proposed the all
method on the root of the document, which would take as an argument the name of a given object like schema
and based on that we would retrieve all elements, those active and those of the components that are not used, for example:
all(type: 'schema' | 'message' | ...etc) {...}
For me this is much better and easier to implement, + we won't have to do breaking change in future.
For the argument we should not spend time maintaining this method to have support for all types
, I answer: how many new objects have we added to the specification since the 2.0.0 release (that is, some 4-5 years ago)?
😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure what is the use case, also I'm not involved in new API discussion. Only at the beginning. I don't like it much 🙂 so I choose to silently accept and then evaluate by using RC as user in code gen.
Anyway, if you have use case for these methods, add them, if not don't. Don't rush. Sorry for being useless anyway 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agree with @magicmatatjahu. Let's freeze this feature and see if it's actually needed. A root all
method should do the trick for now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A root all method should do the trick for now 👍
I would suggest we rather add one method per each collection we want to grab all objects, declaring each intent individually:
allChannels()
allMessages()
allServers()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I dont care whether it's allChannels
, all(type: 'channels')
or channels().all()
🤷 Just stay consistent that is all I can add 👍 Let's see it in action and how it feels like, so just pick one 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a followup issue in parser-api
repo: asyncapi/parser-api#76
I'm closing this PR in favour of the new suggested functionality.
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I created a followup issue in parser-api repo: asyncapi/parser-api#76 I'm closing this PR in favour of the new suggested functionality. |
Description
Even though there is an ongoing discussion, this PR adds the
filterByInUse()
andfilterByNotInUse()
filters to collections.We can either wait until we resolve the discussion for merging/removing/modifying this or rather move forward and change later if needed.
For the record, this is also related: #612 (comment)
For simplicity, I added the filter to the
Collection
class. This has a side effect: all collections now have such a filters.If this is a problem (could confuse?), I could create another class that extends from Collection or rather a mixin or similar.
cc @magicmatatjahu @fmvilas
Related issue(s)