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

Fix Union Types Import Issue #6789

Merged
merged 44 commits into from
Aug 20, 2020
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
54a529c
First approach for discussion
FrankEssenberger Jun 26, 2020
234d2bd
typo
FrankEssenberger Jun 26, 2020
74f3286
add addiotional method
FrankEssenberger Jul 1, 2020
598ec7b
polish a bit
FrankEssenberger Jul 1, 2020
57e9f2e
remove call of super method
FrankEssenberger Jul 1, 2020
a5f5021
fix javadoc error
FrankEssenberger Jul 1, 2020
5f12d9a
Merge branch 'master' of https://github.com/OpenAPITools/openapi-gene…
FrankEssenberger Jul 2, 2020
53d6b3c
com.google.common.collect.
FrankEssenberger Jul 3, 2020
d0f83e3
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 3, 2020
8f4d3b7
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 7, 2020
2529f40
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 10, 2020
cbb8701
merge master regenerate samples
FrankEssenberger Jul 10, 2020
b3fc17e
sort imports alphabetically
FrankEssenberger Jul 10, 2020
bcfdc66
sort imports alphabetically with right key
FrankEssenberger Jul 10, 2020
51b911c
typo
FrankEssenberger Jul 10, 2020
c8f1139
add type previous imports are still there.
FrankEssenberger Jul 11, 2020
eec9818
add type previous imports are still there.
FrankEssenberger Jul 12, 2020
d52396d
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 12, 2020
23de755
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 20, 2020
e1c0882
remove new test to see if they are the problem.
FrankEssenberger Jul 20, 2020
f34fe62
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 27, 2020
d75a96a
merge master add tests back in
FrankEssenberger Jul 27, 2020
f66c514
align changes which should not lead to failing test but you never know.
FrankEssenberger Jul 27, 2020
a8ecd4e
remove formatting changes
FrankEssenberger Jul 27, 2020
460a0f5
dummy change
FrankEssenberger Jul 27, 2020
f7a9516
revert spaces
FrankEssenberger Jul 27, 2020
08042bf
revert spaces
FrankEssenberger Jul 27, 2020
7096692
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Jul 28, 2020
246614b
revert functional changes
FrankEssenberger Jul 28, 2020
2bbd922
comment out test
FrankEssenberger Jul 28, 2020
56eff35
remove model
FrankEssenberger Jul 28, 2020
16f897f
remove interface method
FrankEssenberger Jul 28, 2020
83b5e6f
remove test class completely
FrankEssenberger Jul 28, 2020
dc0f575
put test class back - test body commented out
FrankEssenberger Aug 3, 2020
de585d5
rename test methods
FrankEssenberger Aug 3, 2020
40dce44
put back logic and tests
FrankEssenberger Aug 3, 2020
7cc0b16
remove generated APIs
FrankEssenberger Aug 3, 2020
1aff13b
remark amakhrov
FrankEssenberger Aug 4, 2020
88ded7d
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Aug 4, 2020
1fb99be
check in one generated file to test
FrankEssenberger Aug 4, 2020
24dc8ce
adjust call super
FrankEssenberger Aug 4, 2020
c83d6b4
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Aug 4, 2020
5f376de
Merge remote-tracking branch 'upstream/master'
FrankEssenberger Aug 11, 2020
d076160
add comment use set.
FrankEssenberger Aug 13, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ public interface CodegenConfig {

String toModelImport(String name);

Map<String,String> toModelImportMap(String name);

String toApiImport(String name);

void addOperationToGroup(String tag, String resourcePath, Operation operation, CodegenOperation co, Map<String, List<CodegenOperation>> operations);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.base.CaseFormat;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.samskivert.mustache.Mustache;
import com.samskivert.mustache.Mustache.Compiler;
import com.samskivert.mustache.Mustache.Lambda;
Expand Down Expand Up @@ -1367,6 +1368,16 @@ public String toModelImport(String name) {
}
}

/**
* Returns the same content as [[toModelImport]] with key the fully-qualified Model name and value the initial input.
* In case of union types this method has a key for each separate model and import.
* @param name the name of the "Model"
* @return Map of fully-qualified models.
*/
public Map<String,String> toModelImportMap(String name){
return Collections.singletonMap(this.toModelImport(name),name);
}

/**
* Return the fully-qualified "Api" name for import
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1077,24 +1077,8 @@ private Map<String, Object> processOperations(CodegenConfig config, String tag,
allImports.addAll(op.imports);
}

List<Map<String, String>> imports = new ArrayList<>();
Set<String> mappingSet = new TreeSet<>();
for (String nextImport : allImports) {
Map<String, String> im = new LinkedHashMap<>();
String mapping = config.importMapping().get(nextImport);
if (mapping == null) {
mapping = config.toModelImport(nextImport);
}

if (mapping != null && !mappingSet.contains(mapping)) { // ensure import (mapping) is unique
mappingSet.add(mapping);
im.put("import", mapping);
im.put("classname", nextImport);
if (!imports.contains(im)) { // avoid duplicates
imports.add(im);
}
}
}
Map<String,String> mappings = getAllImportsMapppings(allImports);
List<Map<String, String>> imports = toImportsObjects(mappings);

operations.put("imports", imports);

Expand All @@ -1115,6 +1099,40 @@ private Map<String, Object> processOperations(CodegenConfig config, String tag,
return operations;
}

private Map<String,String> getAllImportsMapppings(Set<String> allImports){
Copy link
Member

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)

Copy link
Contributor Author

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.

Map<String,String> result = new HashMap<>();
allImports.forEach(nextImport->{
String mapping = config.importMapping().get(nextImport);
if(mapping!= null){
result.put(mapping,nextImport);
}else{
result.putAll(config.toModelImportMap(nextImport));
}
});
return result;
}

private List<Map<String,String>> toImportsObjects(Map<String,String> mappedImports){
List<Map<String, String>> result = new ArrayList<>();
mappedImports.entrySet().forEach(mapping->{
Map<String, String> im = new LinkedHashMap<>();
im.put("import", mapping.getKey());
im.put("classname", mapping.getValue());
if (!result.contains(im)) { // avoid duplicates
result.add(im);
}
});
Collections.sort(result, new Comparator<Map<String, String>>() {
@Override
public int compare(final Map<String, String> o1, final Map<String, String> o2) {
String s1 = o1.get("classname");
String s2 = o2.get("classname");
return s1.compareTo(s2);
}
});
return result;
}

private Map<String, Object> processModels(CodegenConfig config, Map<String, Schema> definitions) {
Map<String, Object> objs = new HashMap<>();
objs.put("package", config.modelPackage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -221,6 +224,38 @@ public void processOpts() {
}
}

@Override
public String toModelImport( String name){
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
if(name.contains("|")){
List<String> names = Arrays.asList(name.split("\\|"));
LOGGER.warn("The import is a union type. Consider using the toModelImportMap method.");
return names.stream()
.map(withSpace->withSpace.replaceAll(" ",""))
.map(noSpace->super.toModelImport(noSpace))
.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(name.contains("|")){
List<String> names = Arrays.asList(name.replace(" ","").split("\\|"));
Map<String,String> result = Maps.newHashMap();
names.forEach(s->result.put(toModelImport(s),s));
return result;
}
return Collections.singletonMap(toModelImport(name),name);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These two methods (toModelImportMap and toModelImport) look very similar (split by |, remove spaces, process each part). Should one method just delegate to the other? E.g. toModelImport could call toModelImportMap and then collect the resulting map back into a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.



@Override
public void preprocessOpenAPI(OpenAPI openAPI) {

Expand Down
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/"));
}
}
74 changes: 74 additions & 0 deletions modules/openapi-generator/src/test/resources/split-import.json
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"
}
}
}
}
}
}
}