Skip to content

Remove servers urls with trailing slash #7940

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jan 25, 2021
Merged

Remove servers urls with trailing slash #7940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b90f812
remove trailing slash
ksvirkou-hubspot Nov 14, 2020
94444eb
update sample app
ksvirkou-hubspot Nov 14, 2020
0c767a2
added tests
ksvirkou-hubspot Dec 24, 2020
a0e0a03
bug fix
ksvirkou-hubspot Dec 24, 2020
d8d322a
Adds test in DefaultGeneratorTest
spacether Jan 10, 2021
1fb0199
Reverts python files
spacether Jan 10, 2021
6be24b2
Does not modify a value of /
spacether Jan 11, 2021
69ff970
Stops skipping / use case
spacether Jan 13, 2021
79f34dd
update samples
wing328 Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,9 @@ void configureGeneratorProperties() {
// TODO: Allow user to define _which_ servers object in the array to target.
// Configures contextPath/basePath according to api document's servers
URL url = URLPathUtils.getServerURL(openAPI, config.serverVariableOverrides());
contextPath = config.escapeText(url.getPath()).replaceAll("/$", ""); // for backward compatibility
contextPath = removeTrailingSlash(config.escapeText(url.getPath())); // for backward compatibility
basePathWithoutHost = contextPath;
basePath = config.escapeText(URLPathUtils.getHost(openAPI, config.serverVariableOverrides())).replaceAll("/$", "");
basePath = removeTrailingSlash(config.escapeText(URLPathUtils.getHost(openAPI, config.serverVariableOverrides())));
}

private void configureOpenAPIInfo() {
Expand Down Expand Up @@ -552,7 +552,7 @@ void generateModels(List<File> files, List<Object> allModels, List<String> unuse
}

@SuppressWarnings("unchecked")
private void generateApis(List<File> files, List<Object> allOperations, List<Object> allModels) {
void generateApis(List<File> files, List<Object> allOperations, List<Object> allModels) {
if (!generateApis) {
// TODO: Process these anyway and present info via dryRun?
LOGGER.info("Skipping generation of APIs.");
Expand Down Expand Up @@ -580,7 +580,7 @@ private void generateApis(List<File> files, List<Object> allOperations, List<Obj
Map<String, Object> operation = processOperations(config, tag, ops, allModels);
URL url = URLPathUtils.getServerURL(openAPI, config.serverVariableOverrides());
operation.put("basePath", basePath);
operation.put("basePathWithoutHost", config.encodePath(url.getPath()).replaceAll("/$", ""));
operation.put("basePathWithoutHost", removeTrailingSlash(config.encodePath(url.getPath())));
operation.put("contextPath", contextPath);
operation.put("baseName", tag);
operation.put("apiPackage", config.apiPackage());
Expand Down Expand Up @@ -756,7 +756,7 @@ private void generateSupportingFiles(List<File> files, Map<String, Object> bundl
}

@SuppressWarnings("unchecked")
private Map<String, Object> buildSupportFileBundle(List<Object> allOperations, List<Object> allModels) {
Map<String, Object> buildSupportFileBundle(List<Object> allOperations, List<Object> allModels) {

Map<String, Object> bundle = new HashMap<>(config.additionalProperties());
bundle.put("apiPackage", config.apiPackage());
Expand Down Expand Up @@ -806,6 +806,7 @@ private Map<String, Object> buildSupportFileBundle(List<Object> allOperations, L

List<CodegenServer> servers = config.fromServers(openAPI.getServers());
if (servers != null && !servers.isEmpty()) {
servers.forEach(server -> server.url = removeTrailingSlash(server.url));
bundle.put("servers", servers);
bundle.put("hasServers", true);
}
Expand Down Expand Up @@ -1483,4 +1484,8 @@ private void generateFilesMetadata(List<File> files) {
}
}

private String removeTrailingSlash(String value) {
Copy link
Contributor

@spacether spacether Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should pass in a boolean doNotModifyOnlySlash and when it is true, for "/" leave it as-is?
That we we could use it to preserve the current behavior here for rust jersey-2 etc.

return StringUtils.removeEnd(value, "/");
}

}
29 changes: 29 additions & 0 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
import io.swagger.v3.oas.models.parameters.RequestBody;
import io.swagger.v3.oas.models.responses.ApiResponse;
import io.swagger.v3.oas.models.responses.ApiResponses;
import org.apache.commons.lang3.StringUtils;
import org.openapitools.codegen.config.CodegenConfigurator;
import org.openapitools.codegen.config.GlobalSettings;
import org.openapitools.codegen.meta.GeneratorMetadata;
import org.openapitools.codegen.meta.Stability;
import org.openapitools.codegen.utils.ModelUtils;
import org.testng.Assert;
import org.testng.annotations.Test;
Expand Down Expand Up @@ -636,5 +639,31 @@ public void testCustomNonLibraryTemplates() throws IOException {
templates.toFile().delete();
}
}

@Test
public void testHandlesTrailingSlashInServers() {
OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/issue_7533.yaml");
ClientOptInput opts = new ClientOptInput();
opts.openAPI(openAPI);
DefaultCodegen config = new DefaultCodegen();
config.setStrictSpecBehavior(false);
opts.config(config);
final DefaultGenerator generator = new DefaultGenerator();
generator.opts(opts);
generator.configureGeneratorProperties();

List<File> files = new ArrayList<>();
List<String> filteredSchemas = ModelUtils.getSchemasUsedOnlyInFormParam(openAPI);
List<Object> allModels = new ArrayList<>();
generator.generateModels(files, allModels, filteredSchemas);
List<Object> allOperations = new ArrayList<>();
generator.generateApis(files, allOperations, allModels);

Map<String, Object> bundle = generator.buildSupportFileBundle(allOperations, allModels);
LinkedList<CodegenServer> servers = (LinkedList<CodegenServer>) bundle.get("servers");
Assert.assertEquals(servers.get(0).url, "");
Assert.assertEquals(servers.get(1).url, "http://trailingshlash.io:80/v1");
Assert.assertEquals(servers.get(2).url, "http://notrailingslash.io:80/v2");
}
}

17 changes: 17 additions & 0 deletions modules/openapi-generator/src/test/resources/3_0/issue_7533.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
openapi: 3.0.1
info:
title: OpenAPI Petstore
description: "sample spec"
license:
name: Apache-2.0
url: https://www.apache.org/licenses/LICENSE-2.0.html
version: 1.0.0
servers:
- url: /
Copy link
Contributor

@spacether spacether Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Per @jimschubert
Server with no url base specified is meant to be the same server where the spec doc was downloaded from.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#server-object:
REQUIRED. A URL to the target host. 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. Variable substitutions will be made when a variable is named in {brackets}.
So we should only do this stripping if the length is > 1

Copy link
Contributor

@spacether spacether Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm when we keep this value as '/' rust tests fail because they assume that we keep stripping the / from the end of the server url. Probably better to keep removing it and have future PRs detect the "" use case if they need to. That way this is non-breaking.

- url: http://trailingshlash.io:80/v1/
- url: http://notrailingslash.io:80/v2
tags: []
paths: {}
components:
schemas: {}
securitySchemes: {}
2 changes: 1 addition & 1 deletion ...ava/jersey2-java8-special-characters/src/main/java/org/openapitools/client/ApiClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public class ApiClient extends JavaTimeFormatter {

protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>(Arrays.asList(
new ServerConfiguration(
"/",
"",
"No description provided",
new HashMap<String, ServerVariable>()
)
Expand Down