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

[all] Add leading slash in path if missing #1034

Merged
merged 2 commits into from
Sep 16, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented Sep 14, 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: @OpenAPITools/generator-core-team @OpenAPITools/openapi-generator-collaborators (change in core)

Description of the PR

From #967 (comment) and #997.

This PR adds the leading / if it is missing in the path definition.


More details:

This spec is not valid:

openapi: '3.0.1'
info:
  title: ping test
  version: '1.0'
servers:
  - url: 'http://localhost:8000/api'
paths:
  ping/me:
    get:
      operationId: pingGet
      responses:
        '201':
          description: OK

Because the path ping/me do not conform to this rule:

The field name MUST begin with a slash.

See the OpenAPI Specification 3.0.1.

Some editors like Swagger online editor or the KaiZen-Editor are reporting errors, but Swagger-Parser does not.

Some templates uses {{{basePathWithoutHost}}}{{{path}}}.

  • Before this PR it will generate http://localhost:8000/apiping/me (incorrect)
  • After this PR it will generate http://localhost:8000/api/ping/me (correct)

@wing328
Copy link
Member

wing328 commented Sep 14, 2018

@jmini I think you will need to update the unit tests as well

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running TestSuite
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Tests run: 438, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 11.76 sec <<< FAILURE! - in TestSuite
testConsistentParameterNameAfterUniquenessRename(org.openapitools.codegen.DefaultCodegenTest)  Time elapsed: 0.025 sec  <<< FAILURE!
java.lang.AssertionError: expected [p/] but found [/p/]
	at org.openapitools.codegen.DefaultCodegenTest.testConsistentParameterNameAfterUniquenessRename(DefaultCodegenTest.java:228)
testProcessPaths(org.openapitools.codegen.DefaultGeneratorTest)  Time elapsed: 0.024 sec  <<< FAILURE!
java.lang.AssertionError: expected [path1/] but found [/path1/]
	at org.openapitools.codegen.DefaultGeneratorTest.testProcessPaths(DefaultGeneratorTest.java:41)
Results :
Failed tests:
  DefaultCodegenTest.testConsistentParameterNameAfterUniquenessRename:228 expected [p/] but found [/p/]
  DefaultGeneratorTest.testProcessPaths:41 expected [path1/] but found [/path1/]

Ref: https://travis-ci.org/OpenAPITools/openapi-generator/builds/428486439

@jmini
Copy link
Member Author

jmini commented Sep 16, 2018

@wing328 I have updated the tests, sorry to have missed it the first time

@wing328
Copy link
Member

wing328 commented Sep 16, 2018

@jmini no worry. The fix looks good to me 👍

@wing328 wing328 merged commit 27e343f into OpenAPITools:master Sep 16, 2018
@wing328 wing328 added this to the 3.3.0 milestone Sep 16, 2018
jaumard pushed a commit to jaumard/openapi-generator that referenced this pull request Sep 21, 2018
* [all] Add leading slash in path if missing

* Fix unit Tests
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* [all] Add leading slash in path if missing

* Fix unit Tests
@jimschubert jimschubert mentioned this pull request May 1, 2019
4 tasks
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.

None yet

2 participants