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 2 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 @@ -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,65 @@ 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("\\|"));
return names.stream()
.map(withSpace->withSpace.replaceAll(" ",""))
.map(noSpace->super.toModelImport(noSpace))
.collect(Collectors.joining("|"));
}
return super.toModelImport(name);
}

protected String toModelImportForUnionTypes(String name, Function<String,String> toModelImportSingle){
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
if(name.contains("|")){
List<String> names = Arrays.asList(name.split("\\|"));
return names.stream()
.map(withSpace->withSpace.replaceAll(" ",""))
.map(noSpace->toModelImportSingle.apply(noSpace))
.collect(Collectors.joining("|"));
}
return toModelImportSingle.apply(name);
}

@Override
public Map<String, Object> postProcessOperationsWithModels(final Map<String, Object> objs, final List<Object> allModels) {
Map<String, Object> objsUnderProcess= super.postProcessOperationsWithModels(objs, allModels);

if(Boolean.valueOf(objsUnderProcess.get("hasImport").toString())){
objsUnderProcess.put("imports", splitUniontypeImports((List<Map<String,String>>) objsUnderProcess.get("imports")));
}
return objsUnderProcess;
}

protected List<Object> splitUniontypeImports(List<Map<String,String>> imports){
Copy link
Member

Choose a reason for hiding this comment

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

@amakhrov @wing328 do you think such a complicated logic is necessary, or could this be solved elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks quite similar to the existing logic in TypeScriptAngularClientCodegen (see postProcessAllModels and parseImports)

Maybe we should lift the logic from AngularClient to AbstractTypescriptClient instead of pretty much repeating it?

The root problem, as I see it, is that toModelImport takes a string as an argument rather than a model and returns a string instead of a list. It makes it very limited in terms of handling composite models. But changing that would require a change in Generator and CodegenConfig interfaces. And generally means more work, of course.

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 totally agree on your analysis regarding the signature of the toModelImport(). I did not want to change that. I have also missed the implementation already present in the AngularClient. I will try to move everything up to the parent class.

List<Object> splittedImports = Lists.newArrayList();
imports.forEach(
(importEntry)->{
if(importEntry.get("import").contains("|")){
String[] importsForEntry = importEntry.get("import").split("\\|");
String[] classNamesForEntry = importEntry.get("classname").replaceAll(" ","").split("\\|");
if(importsForEntry.length != classNamesForEntry.length){
throw new RuntimeException(String.format("Size of imports and class names do not match. Imports: %s ClassNames: %s",importsForEntry.toString(),classNamesForEntry.toString()));
}
for(int i=0; i < importsForEntry.length;i++){
Map<String,String> splittedImportEntry= Maps.newHashMap();
splittedImportEntry.put("import",importsForEntry[i]);
splittedImportEntry.put("classname",classNamesForEntry[i]);
if(!imports.contains(splittedImportEntry)) {
splittedImports.add(splittedImportEntry);
}
}
}else {
splittedImports.add(importEntry);
}
}
);
return splittedImports;
}
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
Expand Up @@ -27,6 +27,7 @@

import java.io.File;
import java.util.*;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -176,7 +177,7 @@ public void processOpts() {
apiTemplateFiles.put("apiInterface.mustache", "Interface.ts");
}
}

if (additionalProperties.containsKey(USE_SINGLE_REQUEST_PARAMETER)) {
this.setUseSingleRequestParameter(convertPropertyToBoolean(USE_SINGLE_REQUEST_PARAMETER));
}
Expand All @@ -194,7 +195,7 @@ public void processOpts() {
));
}

if (ngVersion.atLeast("9.0.0")) {
if (ngVersion.atLeast("9.0.0")) {
additionalProperties.put(ENFORCE_GENERIC_MODULE_WITH_PROVIDERS, true);
} else {
additionalProperties.put(ENFORCE_GENERIC_MODULE_WITH_PROVIDERS, false);
Expand Down Expand Up @@ -333,7 +334,7 @@ public boolean getQueryParamObjectFormatKey() {
public boolean isDataTypeFile(final String dataType) {
return dataType != null && dataType.equals("Blob");
}

@Override
public String getTypeDeclaration(Schema p) {
if (ModelUtils.isFileSchema(p)) {
Expand All @@ -359,6 +360,7 @@ public void postProcessParameter(CodegenParameter parameter) {

@Override
public Map<String, Object> postProcessOperationsWithModels(Map<String, Object> operations, List<Object> allModels) {
operations = super.postProcessOperationsWithModels(operations, allModels);
Map<String, Object> objs = (Map<String, Object>) operations.get("operations");

// Add filename information for api imports
Expand Down Expand Up @@ -546,10 +548,13 @@ public String toModelFilename(String name) {

@Override
public String toModelImport(String name) {
if (importMapping.containsKey(name)) {
return importMapping.get(name);
}
return modelPackage() + "/" + toModelFilename(name).substring(DEFAULT_IMPORT_PREFIX.length());
Function<String,String> toModelImportAngular = (s) -> {
if (importMapping.containsKey(s)) {
return importMapping.get(s);
}
return modelPackage() + "/" + toModelFilename(s).substring(DEFAULT_IMPORT_PREFIX.length());
};
return toModelImportForUnionTypes(name,toModelImportAngular);
}

public String getNpmRepository() {
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.media.Schema;
import io.swagger.v3.parser.util.SchemaTypeUtil;
import org.apache.commons.lang3.StringUtils;
Expand All @@ -26,7 +28,6 @@
import org.openapitools.codegen.CodegenOperation;
import org.openapitools.codegen.SupportingFile;
import org.openapitools.codegen.meta.features.DocumentationFeature;
import org.openapitools.codegen.utils.ModelUtils;

import java.util.*;

Expand Down Expand Up @@ -148,6 +149,11 @@ public Map<String, Object> postProcessOperationsWithModels(Map<String, Object> o
return objs;
}

@Override
public String toModelImport(final String name) {
return toModelImportForUnionTypes(name,super::toModelImport);
}

@Override
public Map<String, Object> postProcessAllModels(Map<String, Object> objs) {
Map<String, Object> result = super.postProcessAllModels(objs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import java.io.File;
import java.util.*;
import java.util.function.Function;

import static org.openapitools.codegen.utils.StringUtils.camelize;

Expand Down Expand Up @@ -158,11 +159,13 @@ public String toModelFilename(String name) {

@Override
public String toModelImport(String name) {
if (importMapping.containsKey(name)) {
return importMapping.get(name);
}

return modelPackage() + "/" + camelize(toModelName(name), true);
Function<String,String> toModelImportNode = s -> {
if (importMapping.containsKey(s)) {
return importMapping.get(s);
}
return modelPackage() + "/" + camelize(toModelName(s), true);
};
return toModelImportForUnionTypes(name,toModelImportNode);
}

@Override
Expand Down Expand Up @@ -197,6 +200,7 @@ private List<Map<String, String>> toTsImports(CodegenModel cm, Set<String> impor

@Override
public Map<String, Object> postProcessOperationsWithModels(Map<String, Object> operations, List<Object> allModels) {
operations = super.postProcessOperationsWithModels(operations, allModels);
Map<String, Object> objs = (Map<String, Object>) operations.get("operations");

// The api.mustache template requires all of the auth methods for the whole api
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
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 splitImport() throws IOException {
CodegenConfigurator config =
new CodegenConfigurator()
.setInputSpec("split-import.json")
.setModelPackage("model")
.setApiPackage("api")
.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");

}

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);
}
}
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"
}
}
}
}
}
}
}