Skip to content

Conversation

@A-Aravindhan
Copy link
Contributor

No description provided.

Server with relative location are not converted to an absolute url
@A-Aravindhan
Copy link
Contributor Author

Fix for #742

@A-Aravindhan A-Aravindhan reopened this Jul 6, 2018
Copy link
Contributor

@jmini jmini left a comment

Choose a reason for hiding this comment

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

Thank you a lot for this PR. This is a great start.


Line 351 in modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/OpenAPIDeserializer.java

Current (your PR did not change it):

 Server server = getServer((ObjectNode) item, location, result);

Should be:

Server server = getServer((ObjectNode) item, location, result, path);

To use the new method you have introduced.


Other point (I do not know if it matters for this project. You are working with tabs and the code is formatted with spaces.

String value = getString("url", obj, true, location, result);
if(StringUtils.isNotBlank(value)) {
File file = new File(value);
if(!isValidURL(value) && !file.isAbsolute() && path != null){
Copy link
Contributor

Choose a reason for hiding this comment

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

The check && !file.isAbsolute() is not necessary if you consider a server definition like this:

      {
        "description": "Server relative to root path",
        "url": "/api/v2"
      }

If you remove the check, then you also can remove File file = new File(value);

jmini added a commit to jmini/swagger-parser that referenced this pull request Jul 11, 2018
@jmini
Copy link
Contributor

jmini commented Jul 11, 2018

@A-Aravindhan thank you a lot for this PR to fix my issue #742

During the review, I discovered some points that I have addressed with: jmini@21c0b5e (on top of your commit A-Aravindhan@935c015). I did not modify indentation (spaces vs tabs).


I also think that it is important to add a unit test with this PR.

I have extended my example specification in #742. I then added a new test case to NetworkReferenceTest

See my commit: jmini@209994a

Without your fix the test is red, with your fix it is green.

Can you please integrate these commits (or similar ones) in this Pull Request?

@gracekarina
Copy link
Contributor

Hi, @A-Aravindhan @jmini to be able to merge this PR, can you please integrate your changes into one PR? Thanks!

@jmini
Copy link
Contributor

jmini commented Jul 16, 2018

I am not maintainer of this project, I can not update it: I do not have write permissions on A-Aravindhan:v2.0.1 branch.


@gracekarina:

Performing a quick API call on this PR, I see that you have the permission: maintainer_can_modify: true

You need to add both forks as remote (assuming you work with ssh on GitHub). This should do it:

$ git remote add jmini git@github.com:jmini/swagger-parser.git
$ git remote add A-Aravindhan git@github.com:A-Aravindhan/swagger-parser.git
$ git fetch jmini pr754
$ git push A-Aravindhan jmini/pr754:v2.0.1

Let me know if it does not work.
Thank you in advance.

@jmini
Copy link
Contributor

jmini commented Jul 19, 2018

@gracekarina : I have opened a replacement PR that contains all the changes discussed here => #773

@gracekarina gracekarina merged commit 935c015 into swagger-api:2.0 Jul 19, 2018
@gracekarina
Copy link
Contributor

Hi, I just merged the #773 PR. Thanks for your contributions.

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

Successfully merging this pull request may close these issues.

3 participants