-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Fix Union Types Import Issue #6789
Changes from 43 commits
54a529c
234d2bd
74f3286
598ec7b
57e9f2e
a5f5021
5f12d9a
53d6b3c
d0f83e3
8f4d3b7
2529f40
cbb8701
b3fc17e
bcfdc66
51b911c
c8f1139
eec9818
d52396d
23de755
e1c0882
f34fe62
d75a96a
f66c514
a8ecd4e
460a0f5
f7a9516
08042bf
7096692
246614b
2bbd922
56eff35
16f897f
83b5e6f
dc0f575
de585d5
40dce44
7cc0b16
1aff13b
88ded7d
1fb99be
24dc8ce
c83d6b4
5f376de
d076160
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ | |
|
||
package org.openapitools.codegen.languages; | ||
|
||
import com.google.common.collect.Lists; | ||
import com.google.common.collect.Maps; | ||
import io.swagger.v3.oas.models.OpenAPI; | ||
import io.swagger.v3.oas.models.media.ArraySchema; | ||
import io.swagger.v3.oas.models.media.ComposedSchema; | ||
|
@@ -35,6 +37,7 @@ | |
import java.io.File; | ||
import java.text.SimpleDateFormat; | ||
import java.util.*; | ||
import java.util.function.Function; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
|
@@ -224,6 +227,47 @@ public void processOpts() { | |
} | ||
} | ||
|
||
@Override | ||
public String toModelImport( String name){ | ||
FrankEssenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if(isUnionType(name)){ | ||
LOGGER.warn("The import is a union type. Consider using the toModelImportMap method."); | ||
return toModelImportMap(name).values().stream().collect(Collectors.joining("|")); | ||
} | ||
return super.toModelImport(name); | ||
} | ||
|
||
/** | ||
* Maps the fully qualified model import to the initial given name. In case of union types the map will have multiple entries. | ||
* For example for "classA | classB" the map will two entries have ["model.classA","classA"] and ["model.classB","classB"]. | ||
* | ||
* @param name the name of the "Model" | ||
* @return Map between the fully qualified model import and the initial given name. | ||
*/ | ||
@Override | ||
public Map<String,String> toModelImportMap( String name){ | ||
if(isUnionType(name)){ | ||
String[] names = splitUnionType(name); | ||
return toImportMap(names); | ||
} | ||
return toImportMap(name); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two methods (toModelImportMap and toModelImport) look very similar (split by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure good point. I introduced small helper methods with a more descriptive name and used the map method inside the single entry one. |
||
|
||
private boolean isUnionType(String name){ | ||
return name.contains("|"); | ||
} | ||
|
||
private String[] splitUnionType(String name){ | ||
return name.replace(" ","").split("\\|"); | ||
} | ||
|
||
private Map<String,String> toImportMap(String... names){ | ||
Map<String,String> result = Maps.newHashMap(); | ||
for(String name: names){ | ||
result.put(toModelImport(name),name); | ||
} | ||
return result; | ||
} | ||
|
||
@Override | ||
public void preprocessOpenAPI(OpenAPI openAPI) { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
package org.openapitools.codegen.typescript; | ||
|
||
import org.apache.commons.io.FileUtils; | ||
import org.apache.commons.lang3.StringUtils; | ||
import org.openapitools.codegen.DefaultGenerator; | ||
import org.openapitools.codegen.Generator; | ||
import org.openapitools.codegen.config.CodegenConfigurator; | ||
import org.openapitools.codegen.languages.TypeScriptAxiosClientCodegen; | ||
import org.testng.Assert; | ||
import org.testng.annotations.Test; | ||
|
||
import java.io.File; | ||
import java.io.IOException; | ||
import java.util.List; | ||
|
||
public class SharedTypeScriptTest { | ||
@Test | ||
public void typesInImportsAreSplittedTest() throws IOException { | ||
CodegenConfigurator config = | ||
new CodegenConfigurator() | ||
.setInputSpec("src/test/resources/split-import.json") | ||
.setModelPackage("model") | ||
.setApiPackage("api") | ||
.setOutputDir("src/test/resources/typesInImportsAreSplittedTest") | ||
.addAdditionalProperty( | ||
TypeScriptAxiosClientCodegen.SEPARATE_MODELS_AND_API, true); | ||
|
||
config.setGeneratorName("typescript-axios"); | ||
checkAPIFile(getGenerator(config).generate(), "default-api.ts"); | ||
|
||
config.setGeneratorName("typescript-node"); | ||
checkAPIFile(getGenerator(config).generate(), "defaultApi.ts"); | ||
|
||
config.setGeneratorName("typescript-angular"); | ||
checkAPIFile(getGenerator(config).generate(), "default.service.ts"); | ||
|
||
FileUtils.deleteDirectory(new File("src/test/resources/typesInImportsAreSplittedTest")); | ||
} | ||
|
||
private Generator getGenerator(CodegenConfigurator config) { | ||
return new DefaultGenerator().opts(config.toClientOptInput()); | ||
} | ||
|
||
private void checkAPIFile(List<File> files, String apiFileName) throws IOException { | ||
File apiFile = files.stream().filter(file->file.getName().contains(apiFileName)).findFirst().get(); | ||
String apiFileContent = FileUtils.readFileToString(apiFile); | ||
Assert.assertTrue(!apiFileContent.contains("import { OrganizationWrapper | PersonWrapper }")); | ||
Assert.assertEquals(StringUtils.countMatches(apiFileContent,"import { PersonWrapper }"),1); | ||
Assert.assertEquals(StringUtils.countMatches(apiFileContent,"import { OrganizationWrapper }"),1); | ||
} | ||
|
||
@Test | ||
public void oldImportsStillPresentTest() throws IOException { | ||
CodegenConfigurator config = | ||
new CodegenConfigurator() | ||
.setInputSpec("petstore.json") | ||
.setModelPackage("model") | ||
.setApiPackage("api") | ||
.setOutputDir("src/test/resources/oldImportsStillPresentTest/") | ||
.addAdditionalProperty( | ||
TypeScriptAxiosClientCodegen.SEPARATE_MODELS_AND_API, true); | ||
|
||
config.setGeneratorName("typescript-angular"); | ||
final List<File> files = getGenerator(config).generate(); | ||
File pets = files.stream().filter(file->file.getName().contains("pet.ts")).findFirst().get(); | ||
String apiFileContent = FileUtils.readFileToString(pets); | ||
Assert.assertTrue(apiFileContent.contains("import { Category }")); | ||
Assert.assertTrue(apiFileContent.contains("import { Tag }")); | ||
|
||
FileUtils.deleteDirectory(new File("src/test/resources/oldImportsStillPresentTest/")); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
{ | ||
"openapi": "3.0.2", | ||
"info": { | ||
"title": "SAP Graph - Customers API", | ||
"version": "0.1.0" | ||
}, | ||
"paths": { | ||
"/Customer": { | ||
"get": { | ||
"operationId": "getCustomer", | ||
"responses": { | ||
"200": { | ||
"$ref": "#/components/responses/CustomerResponse" | ||
} | ||
} | ||
} | ||
}, | ||
"/Person": { | ||
"get": { | ||
"operationId": "getPerson", | ||
"responses": { | ||
"200": { | ||
"$ref": "#/components/responses/PersonResponse" | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
"components": { | ||
"schemas": { | ||
"OrganizationWrapper": { | ||
"type": "object", | ||
"properties": { | ||
"id": {"type": "string"} | ||
} | ||
}, | ||
"PersonWrapper": { | ||
"type": "object", | ||
"properties": { | ||
"id": {"type": "string"} | ||
} | ||
} | ||
}, | ||
"responses": { | ||
"CustomerResponse": { | ||
"description": "Get Customer", | ||
"content": { | ||
"application/json": { | ||
"schema": { | ||
"oneOf": [ | ||
{ | ||
"$ref": "#/components/schemas/OrganizationWrapper" | ||
}, | ||
{ | ||
"$ref": "#/components/schemas/PersonWrapper" | ||
} | ||
] | ||
} | ||
} | ||
} | ||
}, | ||
"PersonResponse": { | ||
"description": "Get Person", | ||
"content": { | ||
"application/json": { | ||
"schema": { | ||
"$ref": "#/components/schemas/PersonWrapper" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
Can you please add docstrings to explain what these new (private) functions do (e.g. avoid duplicates)?
We want to document all methods in the default codegen/generator class to make code easier to understand for new contributors. (I do not expect you to document all methods missing the docstring. The community can help with separate PRs)
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.
I added some doc and used a Set to better represent that the entries are unique for the other method.