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

[AsyncAPI] Identify abandoned channels when specifying multiple files #1114

Closed
jonaslagoni opened this issue Apr 22, 2020 · 13 comments
Closed

Comments

@jonaslagoni
Copy link
Collaborator

jonaslagoni commented Apr 22, 2020

User story.
As a developer I would like to check all my AsyncAPI documents for wrongfully specified or abandoned channels.

Is your feature request related to a problem?
When you have multiple AsyncAPI documents it becomes hard to manually figure out if a channel was wrongfully specified or abandoned.

Describe the solution you'd like
Given the following two AsyncAPI documents

...
channels:
  smartylighting/streetlights/1/0/event/{streetlightId}/Iighting/measured:
    parameters:
      streetlightId:
        $ref: './components/parameters.yml#/streetlightId'
    subscribe:
      message:
        $ref: './components/messages.yml#/lightMeasured'
...
channels:
  smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured:
    parameters:
      streetlightId:
        $ref: './components/parameters.yml#/streetlightId'
    publish:
      message:
        $ref: './components/messages.yml#/lightMeasured'

The 🦅 might see the uppercase 'i' in the one channel and lowercase 'L' in the other however in different fonts they become one and the same:
smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured
smartylighting/streetlights/1/0/event/{streetlightId}/Iighting/measured

I would like to see an error or warning saying that those two channels are abandoned.

Additional context
Quickly scanned your ruleset documentation but didn't find any information on how to specify multiple document rulesets, is it supported in Spectral?

Furthermore this multi document rulesets could be applied to matching channels with different parameter types, payloads etc.

Edit
Just to clarify further, when using this rulecheck all of one's AsyncAPI documents would need to be provided for the verification to be complete, so it shouldn't be a standard ruleset which is always checked. Wouldn't mind implementing this myself, I just need to figure out how and if multiple document rulesets can be done.

@nulltoken
Copy link
Contributor

nulltoken commented Apr 23, 2020

As a developer I would like to check all my AsyncAPI documents for wrongfully specified or abandoned channels.

This would be completely in line with #1103.

The core idea would be to check if https://github.com/stoplightio/spectral/blob/develop/src/functions/unreferencedReusableObject.ts can be used as is for this kind of purpose or would need to be extended.

@nulltoken
Copy link
Contributor

Quickly scanned your ruleset documentation but didn't find any information on how to specify multiple document rulesets, is it supported in Spectral?

@jonaslagoni I'm sorry but I fail to understand how this is directly related to the "identify abandoned channels" issue.

Would this not be directly related, but rather a different topic, could you please create an issue of its own for it?

@jonaslagoni
Copy link
Collaborator Author

jonaslagoni commented Apr 23, 2020

@nulltoken to identify abandoned channels you would need to cross reference all your AsyncAPI documents and find channels which are only describe once as either subscribe or a publish operation. This is where the question of whether Spectral supports multiple document rulesets comes in, because without that this feature would not be possible 😄

Just to clarify a bit further on the use-case. Imagine having multiple internal services (service A, B and C, which each have an AsyncAPI document defining their behavior) which are interconnected i.e. A relies on channels from both B and C, B relies on channels from A and C and C relies on channels from A and B. So really interconnected. A developer then introduces a wrong channel for A, it should have been a channel C publishes to, however the developer spells the channel wrong.

I.e. service A subscribes to channel:
smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured
and service C publishes on channel:
smartylighting/streetlights/1/0/event/{streetlightId}/Iighting/measured
This feature would then define both channels as abandoned because C publishes to a channel which nobody subscribes to and A subscribes to a channel nobody publishes to.

With more and more services and the more interconnected they are, the harder it is to figure out where an error is located, but with this feature this exact problem could be easily solved.

@nulltoken
Copy link
Contributor

@jonaslagoni Much clearer! Thanks for the explanation.

This feature would then define both channels as abandoned because C publishes to a channel which nobody subscribes to and A subscribes to a channel nobody publishes to.

That sentence also helped me get a better understanding of the initial request.

It's not a matter of identifying "orphaned" channels (ie. channels that no one refers to, which would be that complex), but useless associations (channels that are actually referenced but with no usage whatsoever from a functional standpoint).

@philsturgeon That sounds like a great proposal, don't you think?

@jonaslagoni Could you please come up with a multi document based repro case that would represent the issue to be solved? This would be very helpful for a potential future PR bringing that rule.

@nulltoken
Copy link
Contributor

This is where the question of whether Spectral supports multiple document rulesets comes in, because without that this feature would not be possible

Provided the documents refer to one another through a $ref, that won't be an issue.

There's a sentence in the doco about this ("By default, Spectral processes each rule on "resolved document" (a file where all $refs have been resolved.").

@philsturgeon
Copy link
Contributor

This sounds like an awesome feature, absolutely @nulltoken ill leave it to you to prioritise this with other spectral workload!

@jonaslagoni
Copy link
Collaborator Author

jonaslagoni commented Apr 23, 2020

@jonaslagoni Could you please come up with a multi document based repro case that would represent the issue to be solved? This would be very helpful for a potential future PR bringing that rule.

@nulltoken i'll set it up tomorrow or saturday.

Provided the documents refer to one another through a $ref, that won't be an issue.

Normally channels cannot be reused with $ref unless you expect the exact same operation (subscribe or publish). So unfortunately that doesn't solve it.

@nulltoken
Copy link
Contributor

Normally channels cannot be reused with $ref unless you expect the exact same operation (subscribe or publish). So unfortunately that doesn't solve it.

@jonaslagoni Ok. This is where a repro case would be very helpful to get a better understanding of the challenges and try and design a solution.

@jonaslagoni
Copy link
Collaborator Author

@nulltoken thought a gist was an easier option, let me know if you want an actual repository instead.
https://gist.github.com/jonaslagoni/bf1802cd3a8b9160db8f18d0c23e8030

@nulltoken
Copy link
Contributor

@jonaslagoni Thanks for that.

File Structure

The A2S representation of the API is made of a single file. However, parts of the definitions can be split into separate files, at the discretion of the user. This is applicable for $ref fields in the specification as follows from the JSON Schema definitions.

In that standpoint (and from my understanding of the spec), the example is made of two completely autonomous specifications and Spectral will currently consider those as separate and completely independendant (and there's actually no way to write a rule working across separate specs).

I now realize that my understanding of your initial context isn't as clear as I thought (and my very thin knowledge of AsyncAPI doesn't help). Those specs seems to represent different actors (eg. consumer vs publisher). And you seem to be "in control" of those two parties. Is this always the case in AsyncAPI world? Do all publishers always know who their subscribers are? Do they always have access to their "consuming" specifications? Isn't that knowledge supposed to be dealt with at the MessageBroker level?

@jonaslagoni
Copy link
Collaborator Author

jonaslagoni commented Apr 29, 2020

Those specs seems to represent different actors (eg. consumer vs publisher). And you seem to be "in control" of those two parties.

Yes exactly.

Is this always the case in AsyncAPI world?

My guess is no. Might need your experience here as well @fmvilas. (from what I know) it depends on the organization/company setup, in our use-case all the AsyncAPI documents for all the services are located in one repository, because we use references throughout the AsyncAPI documents to reuse payload schemas. But I could imagine there would be use-cases where this is not the case and people would not have access to all the documents. Hence my earlier edit of the issue

Just to clarify further, when using this rulecheck all of one's AsyncAPI documents would need to be provided for the verification to be complete, so it shouldn't be a standard ruleset which is always checked.

This issues is based on the notion that if you have a publisher you generally would always want to (at some point) have a subscriber (else why publish it).

Do all publishers always know who their subscribers are? ... Isn't that knowledge supposed to be dealt with at the MessageBroker level?

Generally publishers have no information about who subscribe to the data it just publishes the data. That information should indeed be dealt with at the MessageBroker level as you stated. It is not that the publisher should know who the subscribers are, but give the developer (with access to all the AsyncAPI documents of the interconnected services) a chance to easier troubleshoot and identify if mistakes were made in the designing of the system.

@fmvilas
Copy link

fmvilas commented Apr 29, 2020

Those specs seems to represent different actors (eg. consumer vs publisher). And you seem to be "in control" of those two parties.

AsyncAPI documents represent a single application and how you interact with it. E.g., if you see publish it means you can send messages to this application through the defined servers. If you see subscribe it means you can listen to the messages this application is sending through the defined servers.

Therefore, AsyncAPI doesn't define publishers or subscribers. An application can be both, and most of the times this is the case. You're a publisher on a channel but a subscriber on another one.

Do all publishers always know who their subscribers are?

They shouldn't. Each AsyncAPI file should only describe what a specific application expects. The problem with this approach is that channel definitions are duplicated all over the AsyncAPI files of your event-driven architecture, hence this issue. This is something we want to solve for version 3 but I guess this is going to take some time 😄

@philsturgeon
Copy link
Contributor

Thank you so much for your input everyone! Unfortunately this seems like it's not going to work in the scope of Spectral for now, but instead is probably better handled by Stoplight Platform when it gets more AsyncAPI support in the core (Explorer in particular).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants