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: generating fromTemplate with refs in spec #945

Merged

Conversation

Laupetin
Copy link
Contributor

@Laupetin Laupetin commented Dec 1, 2023

Description

Updates the code for generation via template to add the path of the specification file to the options passed to the generator.
This makes the generator able to resolve paths specified in $ref properties.

Related issue(s)
Fixes #944

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@Laupetin Laupetin force-pushed the fix/generate-template-from-spec-with-ref branch from d2ec6e4 to 912b417 Compare December 1, 2023 11:37
Copy link

sonarcloud bot commented Dec 1, 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 Dec 18, 2023

@Laupetin hey, thanks for the PR.

I just had a look at related issue and started digging a bit as it was confusing why it works in Generator CLI (next time please specify version that you use, it is super important) and do not work in AsyncAPI CLI.

Please have a look at asyncapi/parser-js#797 (comment)
I have a feeling this PR workarounds this underlying problem

@Laupetin
Copy link
Contributor Author

@derberg Hi, thanks for checking it out. Sorry for the delayed answer.

I can reproduce this issue with @asyncapi/cli/1.2.35 and it works with @asyncapi/generator 1.16.0.

From my understanding the root of this issue is that the CLI first loads the spec from the file to a string in-memory.
Then it calls the parser to load the spec fromString (generateFromString).

The parser therefore does not know the directory in which the spec is located in.
It then fails to resolve relative paths.

What does work with the CLI is having an absolute path inside the root spec file.
Taking the example from the linked issue and assuming the spec file is located in /home/user/projects/sample/index.yaml:

# index.yaml
asyncapi: 2.5.0
info:
  title: Example
  version: 0.1.0
channels:
  user/signedup:
    $ref: /home/user/projects/sample/subfolder/user-signedup.yaml

^ This would work. It is not very practical however when having the spec in a git repository that could be checked out with different paths as well.

I could not manage to get anything working with the fix inside the linked comment.
It is possible that i misunderstood though.
I tried file:subfolder/user-signedup.yaml and file://subfolder/user-signedup.yaml, both did not seem to work.

@Laupetin Laupetin force-pushed the fix/generate-template-from-spec-with-ref branch from 912b417 to f5247f3 Compare January 15, 2024 14:52
Copy link

sonarcloud bot commented Jan 15, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Mar 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@peter-rr
Copy link
Member

/update

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to reproduce the bug related #944 and seems to be solved (output is correctly generated) working under the current CLI version 1.14.1 👍

@Laupetin Could you confirm you are not experiencing the issue anymore? In that case we may proceed to close this PR and also the bug related. Thanks! @Souvikns @Shurtu-gal @Amzani

@Laupetin
Copy link
Contributor Author

@peter-rr hey, thanks for checking it out. You are right, the exact case described in the issue seems to now generate correct output.
When testing the new version on the documentation of our project i noticed however that this only applies if the index.yaml of the examplary setup is in your current workdirectory.

If the paths instead are docs/index.yaml and docs/subfolder/user-signedup.yaml then it will still fail to build (the resolved directory seems to be based on the workdirectory and not the directory containing the index.yaml).
Trying using the generator instead of the cli as described in the issue does seem to work though.

@Laupetin Laupetin force-pushed the fix/generate-template-from-spec-with-ref branch from f5d4c7c to d081928 Compare May 17, 2024 14:58
@Laupetin
Copy link
Contributor Author

I rebased the branch on the current master branch. The following scenario does not work with the master branch but does with the changes of this PR (the index.yaml file is not in the current working directory):

# docs/index.yaml
asyncapi: 2.5.0
info:
  title: Example
  version: 0.1.0
channels:
  user/signedup:
    $ref: subfolder/user-signedup.yaml
# docs/subfolder/user-signedup.yaml
subscribe:
  message:
    description: An event describing that a user just signed up.
    payload:
      type: object
      additionalProperties: false
      properties:
        fullName:
          type: string
        email:
          type: string
          format: email
        age:
          type: integer
          minimum: 18

@peter-rr
Copy link
Member

/u

@peter-rr
Copy link
Member

peter-rr commented Jun 4, 2024

/update

@peter-rr
Copy link
Member

/update

Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
Everything working fine with fix applied.

@peter-rr
Copy link
Member

/ptal

@asyncapi-bot
Copy link
Contributor

@Amzani @derberg @magicmatatjahu @Souvikns @Shurtu-gal Please take a look at this PR. Thanks! 👋

@peter-rr
Copy link
Member

/update

Copy link

sonarcloud bot commented Jun 20, 2024

@Shurtu-gal
Copy link
Collaborator

/u

@Shurtu-gal
Copy link
Collaborator

Great catch @Laupetin 🚀, and thanks @peter-rr for looking into this as well. Passing path makes sense acc. to me.

@peter-rr
Copy link
Member

/u

@peter-rr
Copy link
Member

@Souvikns @Shurtu-gal @Amzani
Seems like the issue related to SonarCloud workflow still persists: #1470

FYI: I also opened this issue as a potential improvement to avoid this kind of scenarios: asyncapi/.github#306

@derberg
Copy link
Member

derberg commented Jul 16, 2024

@peter-rr for now we had to disable sonar cloud - to many problems comparing to the value it brings.

hope we can get alternative solution you reported done quickly

/rtm

@asyncapi-bot asyncapi-bot merged commit c1e1b39 into asyncapi:master Jul 16, 2024
10 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Laupetin Laupetin deleted the fix/generate-template-from-spec-with-ref branch July 16, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CLI cannot generate fromTemplate when specification contains file path $refs
5 participants