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

feat: parseFromUrl does not resolve relative references (#504) #572

Conversation

aeworxet
Copy link
Contributor

@aeworxet aeworxet commented Jul 19, 2022

  • getBaseUrl() was made a separate util to make code reuse possible.
  • URL validation in it is not performed because node-fetch performs its own validation at fetch time, so no repetition of this task is made. The function only ensures that url has type of string and lets node-fetch deal with the rest.
  • Condition (typeof window !== 'undefined' && !options.hasOwnProperty('path')) was added for the case if getBaseUrl() gets called directly from inside of a browser (not from parseFromUrl(), but from <script>) AND, amongst all, object options doesn't already have in it its own property path with value. It is assumed in this case that getBaseUrl() was not executed yet, so it's executed.
    If getBaseUrl() gets called from inside of a browser AND object options HAS in it its own property path with a value - OK then, somehow it got that value, so getBaseUrl() does not get executed. This construction was inspired by test 'parse from string' in ./test/sample_browser/index.html.
    It is also assumed that, at the end of the day, user could have provided his own version of options.path - injected it at some point, for example.
  • </> in <anonymous-schema-x> were considered custom HTML tags by browser's parser and didn't appear on screen

    making automated tests that assert visible elements' content to fail. So they are changed to HTML entities &lt;/&gt; before insertion into the DOM and are replaced back to </> by browser's parser at the insertion time, thus allowing automated tests querying elements' content to pass.
  • Assertion tests are made big intentionally, so they test big chunks of functionality at once, and if the test fails, one, using difftool, can quickly peek what was parsed wrong and get an idea of where to look.
  • Updated puppeteer to latest version (v17.0.0 at the moment of writing) just to make it more up to date.

Fixes #344
Fixes #504

@aeworxet aeworxet force-pushed the parseFromUrl-does-not-resolve-relative-references-#504 branch from 3288ba9 to 1344ba1 Compare July 20, 2022 14:13
@sonarqubecloud
Copy link

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 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aeworxet aeworxet marked this pull request as draft July 22, 2022 08:04
@aeworxet aeworxet force-pushed the parseFromUrl-does-not-resolve-relative-references-#504 branch from 1344ba1 to 1b38074 Compare August 31, 2022 10:06
@aeworxet aeworxet marked this pull request as ready for review August 31, 2022 10:09
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@aeworxet thanks for the PR, there is one code smell to fix, please have a look https://sonarcloud.io/project/issues?id=asyncapi_parser-js&pullRequest=572&resolved=false&types=CODE_SMELL

@fmvilas @jonaslagoni @magicmatatjahu
hey folks, I'm not gonna jump into review of this one. I synced with @aeworxet offline several times to discuss possible solution, and yeah, I'm not the best person to approve what I already consulted and like 😆

so yeah, please review, especially @jonaslagoni that reported the request.

tl;dr it is about having similar solution for refs that we already have for parser, when parsing AsyncAPI file from the file system, to set base path to refs. In case of this PR it is about setting base path for refs but when parsing AsyncAPI file from URL or in a browser

@aeworxet aeworxet force-pushed the parseFromUrl-does-not-resolve-relative-references-#504 branch from 1b38074 to 3f11dc9 Compare September 1, 2022 12:20
@aeworxet
Copy link
Contributor Author

aeworxet commented Sep 1, 2022

@derberg
Fixed.

@tapan3
Copy link

tapan3 commented Sep 8, 2022

We are facing same issue, so curious to know when this PR could be merged and available to use?

@sonarqubecloud
Copy link

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

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jonaslagoni
Copy link
Member

@derberg are we waiting for other people to review it as well or can we merge it? 🤔

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@jonaslagoni no, I was most interested with your input as you did report the related issue.

we just need to update PR description properly so GitHub closes related issues.

So, you think we can close #344 with this one too?

@jonaslagoni
Copy link
Member

@jonaslagoni no, I was most interested with your input as you did report the related issue.

we just need to update PR description properly so GitHub closes related issues.

So, you think we can close #344 with this one too?

You right, yea, I think we can, at least that's what I am gonna do 👍

@jonaslagoni
Copy link
Member

@aeworxet just added the following to automatically close the issues once released 🙂

Fixes https://github.com/asyncapi/parser-js/issues/344
Fixes https://github.com/asyncapi/parser-js/issues/504

@jonaslagoni jonaslagoni merged commit d7ea01a into asyncapi:master Sep 14, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aeworxet aeworxet deleted the parseFromUrl-does-not-resolve-relative-references-#504 branch September 15, 2022 06:01
@derberg
Copy link
Member

derberg commented Sep 15, 2022

@all-contributors please add @aeworxet for code

@allcontributors
Copy link
Contributor

@derberg

I've put up a pull request to add @aeworxet! 🎉

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.0.0-next-major.18 🎉

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
5 participants