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

Update swagger-parser to v2.0.3-OpenAPITools.org-1 #951

Merged
merged 4 commits into from
Sep 4, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented Sep 1, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

With our 3.3.0 release we would like to update Swagger-Parser.

Version 2.0.3-OpenAPITools.org-1 was published and is intended to be used until the official 2.0.3 is published (see #787)

Related PR: last Swagger-Parser update => #696

@jmini jmini mentioned this pull request Sep 1, 2018
@jmini
Copy link
Member Author

jmini commented Sep 2, 2018

For a reason that I ignore, the new swagger-parser version has an impact on samples/server/petstore/rust-server/output/rust-server-test/. Samples updated with d3fd57d

CC: @farcaller @frol, @bjgill

@jmini
Copy link
Member Author

jmini commented Sep 3, 2018

Following change was observed when no host is defined in the OASv2 spec like rust-server-test.yaml

--- with version 2.0.2-OpenAPITools.org-1
+++ with version 2.0.3-OpenAPITools.org-1
openapi: 3.0.1
info:
  title: rust-server-test
  description: This spec is for testing rust-server-specific things
  version: 1.0.0
+servers:
+- url: /
paths:
  /dummy:
    get:
      summary: A dummy endpoint to make the spec valid.
      responses:
        200:
          description: Success
          content: {}
components:
  schemas:
    additionalPropertiesObject:
      type: object
      additionalProperties:
        type: string
      description: An additionalPropertiesObject
      example: foo

A new server containing "/" is defined.

--

Having no host seems to be valid according to this page:

If host is not specified, it is assumed to be the same host where the API documentation is being served.

The new value seems correct to me (OAS3 specification server object)

This URL supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the OpenAPI document is being served.

The change in the generated code might be unexpected now, but I think that we will need to fix it.

@bjgill
Copy link
Contributor

bjgill commented Sep 3, 2018

I guess it's technically correct that defining a base path of / and a path of /foo yields a resolved path of //foo.

And, indeed, from your first link:

If basePath is not specified, it defaults to /, that is, all paths start at the host root.

As I understand it, this implies that rust-server was relying on the previous incorrect behaviour where the base path was defaulted to "".

I suspect there are two possible solutions - either we alter the Rust code to strip the right-most / from the base path or we add a vendor extension to the generator to give us back a base path with the current defaulting.

What's the urgency on this? Is this something you were planning to fix? I could probably have a look sometime later this week.

As an aside, I wonder if this is going to cause problems for other generators as well. Do we have any other generators which concatenate the base path and path, and so would end up with //foo as well?

@jmini
Copy link
Member Author

jmini commented Sep 3, 2018

@bjgill: I have the feeling you are mixing basepath and host.

In OASv2 you need to provide 3 elements:

    host: localhost:8080
    basePath: /v2
    schemes:
      - http

The result is one server entry in an OASv3:

servers:
  - url: http://localhost:8080/v2

In OASv3: https://localhost and / as server values are not the same.

  • https://localhost is absolute (corresponds to host: localhost in OASv2)
  • / is relative to the server, where the spec is read (corresponds to host not defined in OASv2)

When the location is relative:

  • if you parse the spec from a distant server: http://api.example.com:8088/v10/openapi.yaml => host is api.example.com:8088
  • if you parse a local file, the host is unknown, it can not be determined.

In the rust-server-test.yaml used as input to generate the sample host is not defined.

In previous Swagger-Parser version: not having an host resulted in not having a server entry after the conversion from OASv2 to OASv3.

In my opinion swagger-parser has improved the conversion.


Notice that the discussion is about checking if Swagger-Parser is doing a better job now or if they have introduced a bug. This should be the criteria to accept or not the Pull-Request.

What we do based on a given input is up to our project and might need some fixes (can be made in separated PR).

@jmini
Copy link
Member Author

jmini commented Sep 4, 2018

What's the urgency on this? Is this something you were planning to fix? I could probably have a look sometime later this week.

Doing a Swagger-Parser update with a minor release (second digit in the version number) is a good practice.

Other users are waiting for other fixes in the parser.


The discussion was triggered here because, while I was updating the lib, I have noticed some changes in the generated rust sample and I was trying to figure out if this is due to a swagger-parser issue (like a regression).


For me the discussion pointed out that the rust-generator might need to better handle cases like (OASv3):

openapi: 3.0.1
info:
  title: test
  description: Test API
  version: 1.2.3
servers:
  - url: /

Independently of this Swagger-Parser update.


With the new parser version the same case in OASv2 can now be also considered:

swagger: '2.0'
info:
  description: Test API
  version: 1.2.3
  title: test
schemes:
- http

This was not possible before due to an issue in the parser.


Keep in mind that this is a sort of corner case. Most of the spec out there will define a host (OASv2) or an absolute server location (OASv3). Not fixing this case right now is not a big deal at all.


@bjgill:

This is why I hope you agree with this PR that does the Swagger-Update.
A feedback from your side would be appreciated.

@wing328
Copy link
Member

wing328 commented Sep 4, 2018

Keep in mind that this is a sort of corner case. Most of the spec out there will define a host (OASv2) or an absolute server location (OASv3). Not fixing this case right now is not a big deal at all.

I agree with you that we can move forward with this PR

@wing328
Copy link
Member

wing328 commented Sep 4, 2018

Keep in mind that this is a sort of corner case. Most of the spec out there will define a host (OASv2) or an absolute server location (OASv3). Not fixing this case right now is not a big deal at all.

I agree with you that we can move forward with this PR

@jmini jmini merged commit 66022a1 into OpenAPITools:master Sep 4, 2018
@bjgill
Copy link
Contributor

bjgill commented Sep 4, 2018

Was busy writing a comment agreeing, but clearly not fast enough.

I'll have a look at getting rust-server fixed up, then.

bjgill pushed a commit to bjgill/openapi-generator that referenced this pull request Sep 4, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Update swagger-parser to v2.0.3-OpenAPITools.org-1
* Run bin/rust-server-petstore.sh
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.

3 participants