Skip to content

fix relative ref with http ref bug#806

Merged
gracekarina merged 7 commits intoswagger-api:2.0from
fuj1g0n:feature/relative-ref-with-http-ref-bugfix
Aug 22, 2018
Merged

fix relative ref with http ref bug#806
gracekarina merged 7 commits intoswagger-api:2.0from
fuj1g0n:feature/relative-ref-with-http-ref-bugfix

Conversation

@fuj1g0n
Copy link
Copy Markdown

@fuj1g0n fuj1g0n commented Aug 20, 2018

This patch fixes #805.

}

@Test
public void testRelativeRefIncludingUrlRef(@Injectable final Schema mockedModel) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This requires a server call to https://raw.githubusercontent.com/. Please mock it in the unit tests.

@jmini
Copy link
Copy Markdown
Contributor

jmini commented Aug 21, 2018

The change looks good to me, but your new unit test requires a server call to https://raw.githubusercontent.com/. Please mock it in the unit tests.

Add a new parameter: @Mocked RemoteUrl remoteUrl and the expectations at the beginning of your method:

final String url = "https://raw.githubusercontent.com/swagger-api/swagger-parser/v2.0.2/modules/swagger-parser-v3/src/test/resources/relative/globals.yaml";
final String expectedResult = "components:\n" + 
        "  schemas:\n" + 
        "    link-object:\n" + 
        "      type: object\n" + 
        "      additionalProperties:\n" + 
        "        \"$ref\": \"#/components/schemas/rel-data\"\n" + 
        "    rel-data:\n" + 
        "      type: object\n" + 
        "      required:\n" + 
        "      - href\n" + 
        "      properties:\n" + 
        "        href:\n" + 
        "          type: string\n" + 
        "        note:\n" + 
        "          type: string\n" + 
        "    result:\n" + 
        "      type: object\n" + 
        "      properties:\n" + 
        "        name:\n" + 
        "          type: string\n" + 
        "        _links:\n" + 
        "          \"$ref\": \"#/components/schemas/link-object\"\n" + 
        "";
List<AuthorizationValue> auths = null;

new Expectations() {{
    RemoteUrl.urlToString(url, auths);
    times = 1;
    result = expectedResult;
}};

Second question:
Is is use that the old behavior adding ./ (I guess that this was for relative url) is still present for relative url?

@fuj1g0n
Copy link
Copy Markdown
Author

fuj1g0n commented Aug 22, 2018

Thanks for the review, and fine example to support this PR.
I reflected your review point.

@fuj1g0n
Copy link
Copy Markdown
Author

fuj1g0n commented Aug 22, 2018

I added a test to confirm that "./" is still attached to relative local ref file.

@jmini
Copy link
Copy Markdown
Contributor

jmini commented Aug 22, 2018

I am not owner of this project, but this PR looks good to me.

Thank you a lot for this contribution

@gracekarina gracekarina self-assigned this Aug 22, 2018
jmini added a commit to OpenAPITools/swagger-parser that referenced this pull request Aug 22, 2018
@gracekarina gracekarina merged commit ed719b0 into swagger-api:2.0 Aug 22, 2018
@gracekarina
Copy link
Copy Markdown
Contributor

Thanks for this PR @fujigon

@fuj1g0n fuj1g0n deleted the feature/relative-ref-with-http-ref-bugfix branch August 23, 2018 06:07
@douglasbgray
Copy link
Copy Markdown

@gracekarina I ran into this issue in the 2.0.2 release and was about to file an issue, when I see it has been fixed here. Do you have a timeline for the 2.0.3 release with this fix?

Thanks!

@gracekarina
Copy link
Copy Markdown
Contributor

hi @douglasbgray the current plan is to release in a couple of weeks.

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.

4 participants