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

fix: disclose limitation about parameter schema reference #192

Merged
merged 11 commits into from
Nov 7, 2023

Conversation

jonaslagoni
Copy link
Member

Description
If a parameter's schema is defined with a reference in v2, it would completely be removed in v3, because we cant manipulate references correctly yet, see #90

@jonaslagoni jonaslagoni changed the title chore: disclose limitation about parameter schema reference fix: disclose limitation about parameter schema reference Oct 9, 2023
@jonaslagoni jonaslagoni requested a review from derberg October 9, 2023 16:00
@derberg
Copy link
Member

derberg commented Oct 9, 2023

just noticed problem is not only with parameters that have schema as reference

look at https://github.com/asyncapi/spec/blob/master/examples/rpc-client.yml

  '{queue}':
    parameters:
      queue:
        schema:
          type: string
          pattern: '^amq\\.gen\\-.+$'

v3 conversion makes it queue: {}

at the reason is pattern which we do not have in https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#user-content-parameter-object and imho it means we should add it Parameter Object for 3.0.0

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Oct 9, 2023

just noticed problem is not only with parameters that have schema as reference

look at https://github.com/asyncapi/spec/blob/master/examples/rpc-client.yml

  '{queue}':
    parameters:
      queue:
        schema:
          type: string
          pattern: '^amq\\.gen\\-.+$'

v3 conversion makes it queue: {}

at the reason is pattern which we do not have in https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#user-content-parameter-object and imho it means we should add it Parameter Object for 3.0.0

Can only convert to what the spec allows 😄

But yea, agree it's probably a good case for a feature request. For 3.0.0, no, 3.1.0 sure.

@jonaslagoni
Copy link
Member Author

@derberg I still think this should be merged as is, as the other issue you raised does not really affect it 🙂

@derberg
Copy link
Member

derberg commented Oct 10, 2023

@jonaslagoni yeah, I agree that pattern is not critical for 3.0 as lack of it will not affect adoption. But it is a known regression that we should document in release notes imho, and probably migration guide, to warn folks that use it. Probably can be additional sentence in section where you talk about parameters in these docs?

Anyway I opened an issue in spec repo to make sure other codeowners agree

regarding this PR, I still see no change and afaik even though error is spotted, conversion continues, so user still gets empty parameter. We should be super clear in the console info, that was not successfully converted AND resulted in an empty parameter object that user needs to update manually

and yeah, we need console.error for pattern

@jonaslagoni
Copy link
Member Author

@derberg I altered the converter to warn the user whenever they use properties in the schema that won't be taken over into the new parameter object, not just for pattern.

I don't think we need to explain anything in the docs though, because it is not really a missing feature 🤔 The regression should be pretty apparent with the warning I added and the migration guide on the website and release notes 🤷

Copy link
Member

derberg commented Oct 11, 2023

but I had a quick look at asyncapi/website#2008 and there is no mention of parameters and migration related to it, or are my 👁️ wrong

@jonaslagoni
Copy link
Member Author

Nope you right, I missed it 😆

@jonaslagoni
Copy link
Member Author

I will add it to the migration guide 👍

src/third-version.ts Outdated Show resolved Hide resolved
src/third-version.ts Outdated Show resolved Hide resolved
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@jonaslagoni jonaslagoni requested a review from derberg October 12, 2023 16:24
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Copy link

sonarqubecloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member

derberg commented Nov 7, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit 6e66aa3 into asyncapi:master Nov 7, 2023
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants