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

eak24 · 2022-01-03T13:42:38Z

recursively search for types during import type collection in deeply nested schemas #11220

Simple change - just recursively adding needed types in the Schema to the imports list.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

wing328 · 2022-01-07T01:29:40Z

@ethan92429 thanks for the PR.

cc @OpenAPITools/generator-core-team

…nested schemas OpenAPITools#11220

This reverts commit 6154f202a0f4db7a706fe3d61b45573581d164d4.

eak24 · 2022-01-07T16:00:33Z

OK - I just finished with the tests, and modularized the import collection code to make code reuse a lot easier. Should be good to go! There is definitely more work to be done in this area in terms of improving code reuse and modularity, but at least I think this is an improvement.

eak24 · 2022-01-09T13:48:15Z

@OpenAPITools/generator-core-team and @wing328 please give this a review when you get a chance! I'm hoping 🤞 that we could get it into master before the end of the week.

modules/openapi-generator/src/main/java/org/openapitools/codegen/ComplexType.java

eak24 · 2022-01-13T17:55:19Z

@spacether I think I've incorporated all your feedback except for the AnyType comment - I'm not sure what the implications would be there. Please advise!

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java

eak24 · 2022-01-13T22:09:09Z

@spacether I think I incorporated all your feedback 😄

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java

spacether · 2022-01-13T23:48:34Z

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java

+        } else if (includeContainerTypes || !(this.getIsArray() || this.getIsMap())) {
+            // Composed types shouldn't be added directly because types like `Cat | Dog` won't have resolvable imports.
+            imports.add(this.getComplexType());
+            imports.add(this.getBaseType());


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

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

spacether · 2022-01-13T23:51:14Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -6727,6 +6740,9 @@ private CodegenParameter heeaderToCodegenParameter(Header header, String headerN
            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));


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

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.

eak24 · 2022-01-14T12:41:29Z

I hope those comments make sense - I was really trying to not change anything about how existing samples would be generated, under the assumption that those work. Instead, I wanted to only effect samples that have the deep recursion.

spacether · 2022-01-14T20:37:38Z

Circleci error looks unrelated

spacether

Looks good; thank you for your PR!

eak24 force-pushed the 11220_nested_type_imports branch from 9011377 to 763cdb1 Compare January 4, 2022 13:20

wing328 added Issue: Bug Enhancement: New generator Feature: Generator and removed Enhancement: New generator labels Jan 7, 2022

wing328 added this to the 5.4.0 milestone Jan 7, 2022

eak24 added 6 commits January 7, 2022 09:39

recursively search for types during import type collection in deeply …

12768ab

…nested schemas OpenAPITools#11220

composed schema recursive type import handling

3910036

change Importable to ComplexType to modularize related type-wrangling.

039e3e2

Problems...

3613d54

Revert "Problems..."

61263ff

This reverts commit 6154f202a0f4db7a706fe3d61b45573581d164d4.

Reverted attempts to reuse recursive type-finding code.

96a6ecd

eak24 force-pushed the 11220_nested_type_imports branch from 763cdb1 to 96a6ecd Compare January 7, 2022 14:39

eak24 added 2 commits January 7, 2022 09:56

add javadoc comments

ff4f9eb

fix javadoc warning.

68ec26e

fix NPE

005b382

spacether reviewed Jan 9, 2022

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/ComplexType.java Outdated Show resolved Hide resolved

eak24 added 2 commits January 13, 2022 12:40

PR feedback incorporation

9aeee78

Include additionalProperties recursion.

5dd54d0

spacether reviewed Jan 13, 2022

View reviewed changes

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java Outdated Show resolved Hide resolved

spacether reviewed Jan 13, 2022

View reviewed changes

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java Outdated Show resolved Hide resolved

More feedback

0181fdd

spacether reviewed Jan 13, 2022

View reviewed changes

...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java Show resolved Hide resolved

More feedback

0a9d5b9

spacether reviewed Jan 13, 2022

View reviewed changes

Add comments from feedback

404e5b4

spacether approved these changes Jan 14, 2022

View reviewed changes

spacether merged commit 6430aaf into OpenAPITools:master Jan 14, 2022

wing328 changed the title ~~recursively search for types during import type collection in deeply …~~ Recursively search for types during import type collection in deeply nested schemas Jan 28, 2022

eak24 deleted the 11220_nested_type_imports branch January 29, 2022 01:42

FreekBes mentioned this pull request Apr 10, 2024

[BUG] DefaultCodeGen doesn't recurse into nested types when looking for imports #11220

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eak24 commented Jan 3, 2022 •

edited by spacether

wing328 commented Jan 7, 2022

eak24 commented Jan 7, 2022

eak24 commented Jan 9, 2022 •

edited

eak24 commented Jan 13, 2022

eak24 commented Jan 13, 2022

spacether Jan 13, 2022 •

edited

eak24 Jan 14, 2022

spacether Jan 13, 2022 •

edited

eak24 Jan 14, 2022

eak24 commented Jan 14, 2022

spacether commented Jan 14, 2022

spacether left a comment

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

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

Conversation

eak24 commented Jan 3, 2022 • edited by spacether

PR checklist

wing328 commented Jan 7, 2022

eak24 commented Jan 7, 2022

eak24 commented Jan 9, 2022 • edited

eak24 commented Jan 13, 2022

eak24 commented Jan 13, 2022

spacether Jan 13, 2022 • edited

Choose a reason for hiding this comment

eak24 Jan 14, 2022

Choose a reason for hiding this comment

spacether Jan 13, 2022 • edited

Choose a reason for hiding this comment

eak24 Jan 14, 2022

Choose a reason for hiding this comment

eak24 commented Jan 14, 2022

spacether commented Jan 14, 2022

spacether left a comment

Choose a reason for hiding this comment

eak24 commented Jan 3, 2022 •

edited by spacether

eak24 commented Jan 9, 2022 •

edited

spacether Jan 13, 2022 •

edited

spacether Jan 13, 2022 •

edited