-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Trim trailing '/' from basePathWithoutHost #967
Trim trailing '/' from basePathWithoutHost #967
Conversation
Given that the normal use is {{{basePathWithoutHost}}}{{{path}}}.
I did some experiments. A lot of templates are concatenating openapi-generator/modules/openapi-generator/src/main/resources/lumen/routes.mustache Lines 16 to 22 in 26591f5
See This is not a problem, because in theory From the spec (source):
Swagger-Editor online or the KaiZen-Editor are reporting errors in a case like this: 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 But swagger-parser is not reporting any error, letting the user generate code with OpenAPI-Generator. The example I have presented before, now generates wrong code (with this PR): /**
* get pingGet
* Summary:
* Notes:
*/
Route::get('/apiping/me', 'DefaultController@pingGet'); Should be In my opinion this PR is correct, and should be merged. We need to add an additional validation (not just the one from swagger-parser) to prevent people having wrong Specification as input. We need to ensure that each path starts with |
I have opened #997 to add the discussed additional validation. |
FYI @OpenAPITools/generator-core-team This PR is not targeted at any particular generator, but will fix rust-server and (likely) a bunch of others as well. Any chance of getting this merged at some stage? |
basePath = basePath.substring(0, basePath.length() - 1); | ||
} | ||
|
||
basePathWithoutHost = contextPath.replaceAll("/$", ""); // for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjgill for contextPath
, I think you will need to do the same by removing the trailing "/". I remember a few generators are still using contextPath
in the templates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjgill wouldn't this need to remove all trailing slashes, not just the last one?
For instance, although technically a valid URL, this would cause issues:
http://localhost:8000/api/////
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this PR is to work around recent changes to swagger-parser that mean that the many generators that do {{{basePathWithoutHost}}}{{{path}}}
stop introducing a '//
' between the two in some cases.
As such, I think I agree that we could apply this fix to contextPath
as well without much fuss. I'll add a commit to that effect shortly.
I'd be more nervous about extending this to remove all trailing slashes. Whilst very weird, this is technically valid behaviour that we've lived with to date.
a5b6486 (removing trailing slash from See #850 (comment) for reference. I think the samples should be regenerated (and the java files containing the change need to be deleted before generation) |
* Trim trailing '/' from basePathWithoutHost Given that the normal use is {{{basePathWithoutHost}}}{{{path}}}. * Trim trailing '/' from contextPath
@bjgill thanks again for the PR, which is included in the v3.3.0 minor release: https://twitter.com/oas_generator/status/1046941449609068544 |
* Trim trailing '/' from basePathWithoutHost Given that the normal use is {{{basePathWithoutHost}}}{{{path}}}. * Trim trailing '/' from contextPath
PR checklist
./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\
.master
,3.3.x
,4.0.x
. Default:master
.Description of the PR
basePath
has its trailing/
trimmed if present. This PR does the same thing forbasePathWithoutHost
as well. This fixes the common use of{{{basePathWithoutHost}}}{{{path}}}
in the presence of trailing/
.This is now necessary as a result of #951, which causes
basePathWithoutHost
to otherwise default to/
. This breaksrust-server
(and probably other generators as well), which assumes that it can safely perform a string concatenation ofbasePathWithoutHost
andpath
.