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

v3 introduces some regression if it comes to parameters description #979

Open
derberg opened this issue Oct 10, 2023 · 13 comments
Open

v3 introduces some regression if it comes to parameters description #979

derberg opened this issue Oct 10, 2023 · 13 comments
Labels
❔ Question A question about the spec or processes

Comments

@derberg
Copy link
Member

derberg commented Oct 10, 2023

In short, no more JSON schema for parameters in v3 -> https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#user-content-parameter-object

But I just discovered one example that actually uses pattern keyword from JSON schema -> https://github.com/asyncapi/spec/blob/master/examples/rpc-client.yml#L21

  • Now, it is not something that I see in a production use cases, just one of our examples
  • Also, affected users might be not many

I think that the impact is small, will not affect adoption and it is completely fine if we add it v3.1

But I want to make sure other codeowners don't see a problem

cc codeowners @fmvilas @dalelane @smoya @char0n

fyi @jonaslagoni

@derberg derberg added the ❔ Question A question about the spec or processes label Oct 10, 2023
@fmvilas
Copy link
Member

fmvilas commented Oct 25, 2023

Yeah, can be added in v3.1.

@smoya
Copy link
Member

smoya commented Nov 2, 2023

First, i want to say thanks @derberg for catching this 🙌🙌.

Now, it is not something that I see in a production use cases

I'm completely not sure about this statement to be true.
I can share a valid use case where SRE department could share just one asyncapi.yaml file with each team who owns channels (lets say rabbitmq or kafka topics) but not documenting all of them one by one but rather using a pattern in the address. I.e. ^team_a\\..+$ or similar pattern.

Having said that, I'm not pretty confident about this causing a low impact in case this feature is missing in v3.0.0.

Im not blocking but just want to know from you if you have sources to confirm this is a really edge case.

@jonaslagoni
Copy link
Member

We have seen similar raised question on slack as well, where people where no longer able to define that the parameter for example should be an integer.

@fmvilas
Copy link
Member

fmvilas commented Nov 3, 2023

It's low impact because it's on 2.6.0. I mean, it's not that some people can't continue using AsyncAPI, it's just that they won't be able to adopt 3.0.0, which is ok. They can adopt 3.1.0, which can be released as soon as January 2024.

@smoya
Copy link
Member

smoya commented Nov 3, 2023

It's low impact because it's on 2.6.0. I mean, it's not that some people can't continue using AsyncAPI, it's just that they won't be able to adopt 3.0.0, which is ok. They can adopt 3.1.0, which can be released as soon as January 2024.

Yes, that's clear enough I would say the only factor that makes me 👍 is that we are gonna add it in the very next minor version.
I was asking, if by any chance, we know real use cases from companies that would be affected. Just to know the impact a bit better.

Another topic is the generator-js. It would be awesome to throw some warnings when a parameter from v2 is not supported in v3. However, I do not see we do that atm, we do not log any line or return some diagnostics but rather the transformed doc. Can we maybe add somewhere some logic that adds a commented line in the final document for each unsupported parameter saying this will be or not supported soon? cc @jonaslagoni

@jonaslagoni
Copy link
Member

Another topic is the generator-js. It would be awesome to throw some warnings when a parameter from v2 is not supported in v3. However, I do not see we do that atm, we do not log any line or return some diagnostics but rather the transformed doc. Can we maybe add somewhere some logic that adds a commented line in the final document for each unsupported parameter saying this will be or not supported soon? cc @jonaslagoni

Do you mean the converter? If so, we already do give the user information about this in the form of a warning 🙂

@derberg
Copy link
Member Author

derberg commented Nov 6, 2023

Now, it is not something that I see in a production use cases, just one of our examples

@smoya I mean't I haven't seen any production-used asyncapi document that would use pattern. I just noticed we have it in one of our examples. This is why I agree to not push to solve this regression in 3.0

Can we maybe add somewhere some logic that adds a commented line in the final document for each unsupported parameter saying this will be or not supported soon? cc @jonaslagoni

@smoya what you mention is addressed in asyncapi/converter-js#192 by @jonaslagoni

@jonaslagoni did you remember to add it to migration guide?

@jonaslagoni
Copy link
Member

@jonaslagoni did you remember to add it to migration guide?

@derberg doing it today yea 👍

@GreenRover
Copy link
Collaborator

+1

Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label May 22, 2024
@ly29
Copy link

ly29 commented May 24, 2024

Just to chime in that we use patters for various parameters that are more open in our channel (topic) structure. No being able to document them is regression. 2.6.0 works fine but yes it is used in production.

@github-actions github-actions bot removed the stale label May 25, 2024
@SebastienGllmt
Copy link

Even if the parameter are all strings in practice for codegen simplicity, it would be good to at least have some kind of docHint field where you can specify the full type

example

parameters:
    streetlightId:
        docHint:
            type: integer

This would allow you to keep enforcing only strings for the parameters, but allow more complex types in tools like the AsyncApi Studio

@viettappcard
Copy link

Hey, chiming in with what @SebastienGllmt mentioned. I've already posted in the Slack as well but essentially:

Let's say I have a channel route /device/{deviceId}/send where deviceId is always an integer. I want to express in the documentation that a deviceId can only be an integer, but with AsyncAPI 3.0.0 restricting the parameter object, I can't.

  1. parameter must be of type string
  2. pattern would be useful here but it's not available

Unfortunately, I have a real person who read the generated html docs and they are — rightfully — confused on what is the type for deviceId when what they sees is a prominent azure-green colored String instead of the expected Integer.

components:
  parameters:
    deviceId:
      description: The ID of the device. must be an integer.

PS: I'm advocating for AsyncAPI usage in the company, they're a higher up, and it's a little awkward to come back and say "hey hey, looks like it's just a limitation of AsyncAPI in this version, hopefully it'll be fixed later" (but like in hopefully nicer than I can say here 😄 )

screenshot_2024-09-28_at_15 14 52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❔ Question A question about the spec or processes
Projects
None yet
Development

No branches or pull requests

8 participants