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

Recursively search for types during import type collection in deeply nested schemas #11221

Merged
merged 14 commits into from
Jan 14, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -756,5 +756,10 @@ public LinkedHashMap<String, CodegenMediaType> getContent() {
public void setContent(LinkedHashMap<String, CodegenMediaType> content) {
this.content = content;
}

@Override
public String getBaseType() {
return baseType;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -614,4 +614,9 @@ public CodegenComposedSchemas getComposedSchemas() {

@Override
public void setHasMultipleTypes(boolean hasMultipleTypes) { this.hasMultipleTypes = hasMultipleTypes; }

@Override
public String getBaseType() {
return baseType;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5120,12 +5120,32 @@ private static String generateNextName(String name) {
}
}

protected void addImports(CodegenModel m, IJsonSchemaValidationProperties type) {
addImports(m.imports, type);
}

protected void addImports(Set<String> importsToBeAddedTo, IJsonSchemaValidationProperties type) {
addImports(importsToBeAddedTo, type.getImports(true));
}

protected void addImports(Set<String> importsToBeAddedTo, Set<String> importsToAdd) {
importsToAdd.stream().forEach(i -> addImport(importsToBeAddedTo, i));
}

protected void addImport(CodegenModel m, String type) {
if (type != null && needToImport(type)) {
m.imports.add(type);
addImport(m.imports, type);
}

private void addImport(Set<String> importsToBeAddedTo, String type) {
if (shouldAddImport(type)) {
importsToBeAddedTo.add(type);
}
}

private boolean shouldAddImport(String type) {
return type != null && needToImport(type);
}

/**
* Loop through properties and unalias the reference if $ref (reference) is defined
*
Expand Down Expand Up @@ -5262,17 +5282,10 @@ protected void addVars(IJsonSchemaValidationProperties m, List<CodegenProperty>
* @param property The codegen representation of the OAS schema's property.
*/
protected void addImportsForPropertyType(CodegenModel model, CodegenProperty property) {
// TODO revise the logic to include map
if (property.isContainer) {
addImport(model, typeMapping.get("array"));
}

addImport(model, property.baseType);
CodegenProperty innerCp = property;
while (innerCp != null) {
addImport(model, innerCp.complexType);
innerCp = innerCp.items;
addImport(model.imports, typeMapping.get("array"));
}
addImports(model, property);
}

/**
Expand Down Expand Up @@ -6727,6 +6740,9 @@ protected LinkedHashMap<String, CodegenMediaType> getContent(Content content, Se
CodegenProperty schemaProp = fromProperty(toMediaTypeSchemaName(contentType, mediaTypeSchemaSuffix), mt.getSchema());
CodegenMediaType codegenMt = new CodegenMediaType(schemaProp, ceMap);
cmtContent.put(contentType, codegenMt);
if (schemaProp != null) {
addImports(imports, schemaProp.getImports(false));
Copy link
Contributor

@spacether spacether Jan 13, 2022

Choose a reason for hiding this comment

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

Why are we not accepting container type imports here?
objects can be sent in parameters and properties of those objects can $ref to other models

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I leave this as true, 130 sample files are changed to include List, Set and Map-like imports that were already defined. This is because there seems to be an assumption to include such standard imports in the mustache templates already, and for some reason the duplicates aren't 'known about' in the codegen code. So I opted to optionally refrain from collecting those types. This could be changed later but for now is at least most consisten with what already is happening.

}
}
return cmtContent;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package org.openapitools.codegen;

import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Stream;

import io.swagger.v3.oas.models.media.Schema;
import org.openapitools.codegen.utils.ModelUtils;
Expand Down Expand Up @@ -213,4 +217,51 @@ default void setTypeProperties(Schema p) {
setIsAnyType(true);
}
}

/**
* @return basic type - no generics supported.
*/
default String getBaseType() {
return null;
};

/**
* @return complex type that can contain type parameters - like {@code List<Items>} for Java
*/
default String getComplexType() {
return getBaseType();
};

/**
* Recursively collect all necessary imports to include so that the type may be resolved.
*
* @param includeContainerTypes whether or not to include the container types in the returned imports.
* @return all of the imports
*/
default Set<String> getImports(boolean includeContainerTypes) {
Set<String> imports = new HashSet<>();
if (this.getComposedSchemas() != null) {
CodegenComposedSchemas composed = (CodegenComposedSchemas) this.getComposedSchemas();
List<CodegenProperty> allOfs = composed.getAllOf() == null ? Collections.emptyList() : composed.getAllOf();
List<CodegenProperty> oneOfs = composed.getOneOf() == null ? Collections.emptyList() : composed.getOneOf();
Stream<CodegenProperty> innerTypes = Stream.concat(allOfs.stream(), oneOfs.stream());
innerTypes.flatMap(cp -> cp.getImports(includeContainerTypes).stream()).forEach(s -> imports.add(s));
} else if (includeContainerTypes || !(this.getIsArray() || this.getIsMap())) {
// this is our base case, add imports for referenced schemas
// this can't be broken out as a separate if block because Typescript only generates union types as A | B
// and would need to handle this differently
imports.add(this.getComplexType());
imports.add(this.getBaseType());
spacether marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@spacether spacether Jan 13, 2022

Choose a reason for hiding this comment

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

This code is for our base case, can you move it above our first composedschema if?
We don't need the baseType import, that is used to describe string/int/object etc.
Unless: do you know of code that imports baseType?

if (this.getComplexType() != null) {
  // Base case: referenced components need to be imported
  imports.add(this.getComplexType());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conversely, if I don't add baseType here, I get 130 changed sample files where key imports are removed. Some languages apparently need to import string - like in cpp they import QString and QDateTime. I wasn't sure why these aren't included, but my goal was to be as non-destructive as possible while adding what I needed. So I left the base type import in. You can see that this was brought in directly in the old code here on line 5270 in DefaultCodegen.java

}
if (this.getItems() !=null && this.getIsArray()) {
imports.addAll(this.getItems().getImports(includeContainerTypes));
}
if (this.getAdditionalProperties() != null) {
imports.addAll(this.getAdditionalProperties().getImports(includeContainerTypes));
}
if (this.getVars() != null && !this.getVars().isEmpty()) {
this.getVars().stream().flatMap(v -> v.getImports(includeContainerTypes).stream()).forEach(s -> imports.add(s));
}
return imports;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@

public class DefaultCodegenTest {

@Test
public void testDeeplyNestedAdditionalPropertiesImports() {
final DefaultCodegen codegen = new DefaultCodegen();
final OpenAPI openApi = TestUtils.parseFlattenSpec("src/test/resources/3_0/additional-properties-deeply-nested.yaml");
codegen.setOpenAPI(openApi);
PathItem path = openApi.getPaths().get("/ping");
CodegenOperation operation = codegen.fromOperation("/ping", "post", path.getPost(), path.getServers());
Assert.assertEquals(Sets.intersection(operation.imports, Sets.newHashSet("Person")).size(), 1);
}

@Test
public void testHasBodyParameter() {
final Schema refSchema = new Schema<>().$ref("#/components/schemas/Pet");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.openapitools.codegen.typescript;

import com.google.common.collect.Sets;
import io.swagger.v3.oas.models.OpenAPI;
import io.swagger.v3.oas.models.PathItem;
import io.swagger.v3.oas.models.media.*;
import org.openapitools.codegen.CodegenOperation;
import org.openapitools.codegen.TestUtils;
import org.openapitools.codegen.languages.TypeScriptClientCodegen;
import org.openapitools.codegen.utils.ModelUtils;
Expand Down Expand Up @@ -41,4 +44,14 @@ public void getTypeDeclarationTest() {
Assert.assertEquals(codegen.getTypeDeclaration(parentSchema), "{ [key: string]: Child; }");
}

@Test
public void testComposedSchemasImportTypesIndividually() {
final TypeScriptClientCodegen codegen = new TypeScriptClientCodegen();
final OpenAPI openApi = TestUtils.parseFlattenSpec("src/test/resources/3_0/composed-schemas.yaml");
codegen.setOpenAPI(openApi);
PathItem path = openApi.getPaths().get("/pets");
CodegenOperation operation = codegen.fromOperation("/pets", "patch", path.getPatch(), path.getServers());
Assert.assertEquals(operation.imports, Sets.newHashSet("Cat", "Dog"));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
openapi: 3.0.1
info:
title: Test additional properties with ref
version: '1.0'
servers:
- url: 'http://localhost:8000/'
paths:
/ping:
post:
operationId: pingGet
responses:
default:
description: default response
content:
application/json:
schema:
type: object
additionalProperties:
type: object
additionalProperties:
type: object
additionalProperties:
"$ref": "#/components/schemas/Person"
components:
schemas:
Person:
type: object
properties:
lastName:
type: string
firstName:
type: string
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,5 @@ def __new__(
_instantiation_metadata=_instantiation_metadata,
**kwargs,
)

from petstore_api.model.animal import Animal