From 33617ee86797bccfe36d533d14661f4414c6bace Mon Sep 17 00:00:00 2001 From: Pavel Miller <167202565+0x0-pavel-0x0@users.noreply.github.com> Date: Sun, 19 May 2024 12:46:50 +0300 Subject: [PATCH] Improve generation of selected models with dependent models (#18462) * Issue-18444: recursively trace variables and support of new option * Issue-18444: suppoting inheritance, but interfaces * Issue-18444: build project instructions executed * code review from wing328: tab-spaces removed * code review by wing328: added a line of comment for the private method --- .../codegen/plugin/CodeGenMojo.java | 7 + .../codegen/CodegenConstants.java | 1 + .../codegen/DefaultGenerator.java | 116 +++++++++-- .../codegen/DefaultGeneratorTest.java | 133 ++++++++++++ .../src/test/resources/bugs/issue_18444.json | 194 ++++++++++++++++++ 5 files changed, 429 insertions(+), 22 deletions(-) create mode 100755 modules/openapi-generator/src/test/resources/bugs/issue_18444.json diff --git a/modules/openapi-generator-maven-plugin/src/main/java/org/openapitools/codegen/plugin/CodeGenMojo.java b/modules/openapi-generator-maven-plugin/src/main/java/org/openapitools/codegen/plugin/CodeGenMojo.java index 6c3b717189be..f7f1af538c84 100644 --- a/modules/openapi-generator-maven-plugin/src/main/java/org/openapitools/codegen/plugin/CodeGenMojo.java +++ b/modules/openapi-generator-maven-plugin/src/main/java/org/openapitools/codegen/plugin/CodeGenMojo.java @@ -443,6 +443,12 @@ public class CodeGenMojo extends AbstractMojo { @Parameter(name = "generateModels", property = "openapi.generator.maven.plugin.generateModels") private Boolean generateModels = true; + /** + * Generate the models recursively if models should generate selectively (see modelsToGenerate) and all dependent models are to generate + */ + @Parameter(name = "generateRecursiveDependentModels", property = "openapi.generator.maven.plugin.generateRecursiveDependentModels") + private Boolean generateRecursiveDependentModels = false; + /** * A comma separated list of models to generate. All models is the default. */ @@ -795,6 +801,7 @@ public void execute() throws MojoExecutionException { GlobalSettings.setProperty(CodegenConstants.API_TESTS, generateApiTests.toString()); GlobalSettings.setProperty(CodegenConstants.API_DOCS, generateApiDocumentation.toString()); GlobalSettings.setProperty(CodegenConstants.WITH_XML, withXml.toString()); + GlobalSettings.setProperty(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS, generateRecursiveDependentModels.toString()); if (configOptions != null) { // Retained for backwards-compatibility with configOptions -> instantiation-types diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConstants.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConstants.java index 017bf79406b0..c9ea2be1a178 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConstants.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConstants.java @@ -25,6 +25,7 @@ public class CodegenConstants { // NOTE: We may want to move these to a separate class to avoid confusion or modification. public static final String APIS = "apis"; public static final String MODELS = "models"; + public static final String GENERATE_RECURSIVE_DEPENDENT_MODELS = "generateRecursiveDependentModels"; public static final String SUPPORTING_FILES = "supportingFiles"; public static final String MODEL_TESTS = "modelTests"; public static final String MODEL_DOCS = "modelDocs"; diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java index 416dba5a8ac2..8337b77be4d0 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java @@ -29,6 +29,7 @@ import io.swagger.v3.oas.models.parameters.Parameter; import io.swagger.v3.oas.models.security.*; import io.swagger.v3.oas.models.tags.Tag; + import org.apache.commons.io.FilenameUtils; import org.apache.commons.io.comparator.PathFileComparator; import org.apache.commons.lang3.ObjectUtils; @@ -65,6 +66,7 @@ import java.util.*; import java.util.concurrent.ConcurrentSkipListSet; import java.util.function.Function; +import java.util.function.Supplier; import java.util.stream.Collectors; import static org.apache.commons.lang3.StringUtils.removeStart; @@ -81,6 +83,7 @@ public class DefaultGenerator implements Generator { protected CodegenIgnoreProcessor ignoreProcessor; private Boolean generateApis = null; private Boolean generateModels = null; + private Boolean generateRecursiveDependentModels = null; private Boolean generateSupportingFiles = null; private Boolean generateWebhooks = null; private Boolean generateApiTests = null; @@ -232,6 +235,7 @@ void configureGeneratorProperties() { generateModelDocumentation = GlobalSettings.getProperty(CodegenConstants.MODEL_DOCS) != null ? Boolean.valueOf(GlobalSettings.getProperty(CodegenConstants.MODEL_DOCS)) : getGeneratorPropertyDefaultSwitch(CodegenConstants.MODEL_DOCS, true); generateApiTests = GlobalSettings.getProperty(CodegenConstants.API_TESTS) != null ? Boolean.valueOf(GlobalSettings.getProperty(CodegenConstants.API_TESTS)) : getGeneratorPropertyDefaultSwitch(CodegenConstants.API_TESTS, true); generateApiDocumentation = GlobalSettings.getProperty(CodegenConstants.API_DOCS) != null ? Boolean.valueOf(GlobalSettings.getProperty(CodegenConstants.API_DOCS)) : getGeneratorPropertyDefaultSwitch(CodegenConstants.API_DOCS, true); + generateRecursiveDependentModels = GlobalSettings.getProperty(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS) != null ? Boolean.valueOf(GlobalSettings.getProperty(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS)) : getGeneratorPropertyDefaultSwitch(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS, false); // Additional properties added for tests to exclude references in project related files config.additionalProperties().put(CodegenConstants.GENERATE_API_TESTS, generateApiTests); @@ -243,6 +247,7 @@ void configureGeneratorProperties() { config.additionalProperties().put(CodegenConstants.GENERATE_APIS, generateApis); config.additionalProperties().put(CodegenConstants.GENERATE_MODELS, generateModels); config.additionalProperties().put(CodegenConstants.GENERATE_WEBHOOKS, generateWebhooks); + config.additionalProperties().put(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS, generateRecursiveDependentModels); if (!generateApiTests && !generateModelTests) { config.additionalProperties().put(CodegenConstants.EXCLUDE_TESTS, true); @@ -443,36 +448,21 @@ private void generateModel(List files, Map models, String } void generateModels(List files, List allModels, List unusedModels, List aliasModels) { + generateModels(files, allModels, unusedModels, aliasModels, new ArrayList<>(), DefaultGenerator.this::modelKeys); + } + + void generateModels(List files, List allModels, List unusedModels, List aliasModels, List processedModels, Supplier> modelKeysSupplier) { if (!generateModels) { // TODO: Process these anyway and add to dryRun info LOGGER.info("Skipping generation of models."); return; } - final Map schemas = ModelUtils.getSchemas(this.openAPI); - if (schemas == null) { - LOGGER.warn("Skipping generation of models because specification document has no schemas."); + Set modelKeys = modelKeysSupplier.get(); + if(modelKeys.isEmpty()) { return; } - String modelNames = GlobalSettings.getProperty("models"); - Set modelsToGenerate = null; - if (modelNames != null && !modelNames.isEmpty()) { - modelsToGenerate = new HashSet<>(Arrays.asList(modelNames.split(","))); - } - - Set modelKeys = schemas.keySet(); - if (modelsToGenerate != null && !modelsToGenerate.isEmpty()) { - Set updatedKeys = new HashSet<>(); - for (String m : modelKeys) { - if (modelsToGenerate.contains(m)) { - updatedKeys.add(m); - } - } - - modelKeys = updatedKeys; - } - // store all processed models Map allProcessedModels = new TreeMap<>((o1, o2) -> ObjectUtils.compare(config.toModelName(o1), config.toModelName(o2))); @@ -508,7 +498,7 @@ void generateModels(List files, List allModels, List unu } } - Schema schema = schemas.get(name); + Schema schema = ModelUtils.getSchemas(this.openAPI).get(name); if (schema.getExtensions() != null && Boolean.TRUE.equals(schema.getExtensions().get("x-internal"))) { LOGGER.info("Model {} not generated since x-internal is set to true", name); @@ -549,6 +539,24 @@ void generateModels(List files, List allModels, List unu // post process all processed models allProcessedModels = config.postProcessAllModels(allProcessedModels); + if (generateRecursiveDependentModels) { + for(ModelsMap modelsMap : allProcessedModels.values()) { + for(ModelMap mm: modelsMap.getModels()) { + CodegenModel cm = mm.getModel(); + if (cm != null) { + for(CodegenProperty variable : cm.getVars()) { + generateModelsForVariable(files, allModels, unusedModels, aliasModels, processedModels, variable); + } + //TODO: handle interfaces + String parentSchema = cm.getParentSchema(); + if (parentSchema != null && !processedModels.contains(parentSchema) && ModelUtils.getSchemas(this.openAPI).containsKey(parentSchema)) { + generateModels(files, allModels, unusedModels, aliasModels, processedModels, () -> Set.of(parentSchema)); + } + } + } + } + } + // generate files based on processed models for (String modelName : allProcessedModels.keySet()) { ModelsMap models = allProcessedModels.get(modelName); @@ -592,7 +600,71 @@ void generateModels(List files, List allModels, List unu LOGGER.info("############ Model info ############"); Json.prettyPrint(allModels); } + } + + /** + * this method guesses the schema type of in parent model used variable and if the schema type is available it let the generate the model for the type of this variable + */ + private void generateModelsForVariable(List files, List allModels, List unusedModels, List aliasModels, List processedModels, CodegenProperty variable) { + if (variable == null) { + return; + } + + final String schemaKey = calculateModelKey(variable.getOpenApiType(), variable.getRef()); + Map allSchemas = ModelUtils.getSchemas(this.openAPI); + if (!processedModels.contains(schemaKey) && allSchemas.containsKey(schemaKey)) { + generateModels(files, allModels, unusedModels, aliasModels, processedModels, () -> Set.of(schemaKey)); + } else if (variable.getComplexType() != null && variable.getComposedSchemas() == null) { + String ref = variable.getHasItems() ? variable.getItems().getRef() : variable.getRef(); + final String key = calculateModelKey(variable.getComplexType(), ref); + if (allSchemas.containsKey(key)) { + generateModels(files, allModels, unusedModels, aliasModels, processedModels, () -> Set.of(key)); + } else { + LOGGER.info("Type " + variable.getComplexType()+" of variable " + variable.getName() + " could not be resolve because it is not declared as a model."); + } + } else { + LOGGER.info("Type " + variable.getOpenApiType()+" of variable " + variable.getName() + " could not be resolve because it is not declared as a model."); + } + } + + private String calculateModelKey(String type, String ref) { + Map schemaMap = ModelUtils.getSchemas(this.openAPI); + Set keys = schemaMap.keySet(); + String simpleRef; + if(keys.contains(type)) { + return type; + } else if (keys.contains(simpleRef = ModelUtils.getSimpleRef(ref))) { + return simpleRef; + } else { + return type; + } + } + + private Set modelKeys() { + final Map schemas = ModelUtils.getSchemas(this.openAPI); + if (schemas == null) { + LOGGER.warn("Skipping generation of models because specification document has no schemas."); + return Collections.emptySet(); + } + String modelNames = GlobalSettings.getProperty("models"); + Set modelsToGenerate = null; + if (modelNames != null && !modelNames.isEmpty()) { + modelsToGenerate = new HashSet<>(Arrays.asList(modelNames.split(","))); + } + + Set modelKeys = schemas.keySet(); + if (modelsToGenerate != null && !modelsToGenerate.isEmpty()) { + Set updatedKeys = new HashSet<>(); + for (String m : modelKeys) { + if (modelsToGenerate.contains(m)) { + updatedKeys.add(m); + } + } + + modelKeys = updatedKeys; + } + return modelKeys; } @SuppressWarnings("unchecked") diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultGeneratorTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultGeneratorTest.java index 0a7cd881b14c..33cee6130212 100644 --- a/modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultGeneratorTest.java +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultGeneratorTest.java @@ -775,4 +775,137 @@ public void testRecursionBug4650() { generator.generateModels(files, allModels, filteredSchemas, aliasModels); // all fine, we have passed } + + + private DefaultGenerator generatorGenerateRecursiveDependentModelsBackwardCompatibility(String recursively) throws IOException { + DefaultGenerator generator = new DefaultGenerator(false); + generator.setGeneratorPropertyDefault(CodegenConstants.MODELS, "true"); + generator.setGeneratorPropertyDefault(CodegenConstants.MODEL_TESTS, "true"); + generator.setGeneratorPropertyDefault(CodegenConstants.MODEL_DOCS, "false"); + generator.setGeneratorPropertyDefault(CodegenConstants.APIS, "true"); + generator.setGeneratorPropertyDefault(CodegenConstants.SUPPORTING_FILES, "true"); + generator.setGeneratorPropertyDefault(CodegenConstants.API_DOCS, "false"); + generator.setGeneratorPropertyDefault(CodegenConstants.API_TESTS, "false"); + generator.setGeneratorPropertyDefault(CodegenConstants.GENERATE_RECURSIVE_DEPENDENT_MODELS, recursively); + return generator; + } + + private ClientOptInput createOptInputIssue18444(Path target) { + final CodegenConfigurator configurator = new CodegenConfigurator() + .setGeneratorName("spring") + .setInputSpec("src/test/resources/bugs/issue_18444.json") + .setOutputDir(target.toAbsolutePath().toString()); + return configurator.toClientOptInput(); + } + + @Test + public void testGenerateRecursiveDependentModelsBackwardCompatibilityIssue18444() throws IOException { + Path target = Files.createTempDirectory("test"); + File output = target.toFile(); + String oldModelsProp = GlobalSettings.getProperty("models"); + + try { + DefaultGenerator generator = generatorGenerateRecursiveDependentModelsBackwardCompatibility("false"); + GlobalSettings.setProperty("models", "RQ1,RS1"); + ClientOptInput clientOptInput = createOptInputIssue18444(target); + List files = generator.opts(clientOptInput ).generate(); + Assert.assertEquals(files.size(), 17); + + // Check expected generated files + // api sanity check + String apiJavaFileName = "src/main/java/org/openapitools/api/ApiApi.java"; + TestUtils.ensureContainsFile(files, output, apiJavaFileName); + Assert.assertTrue(new File(output, apiJavaFileName).exists()); + + // model sanity check + String rq1FileName = "src/main/java/org/openapitools/model/RQ1.java"; + TestUtils.ensureContainsFile(files, output, rq1FileName); + Assert.assertTrue(new File(output, rq1FileName).exists()); + + String rs1FileName = "src/main/java/org/openapitools/model/RS1.java"; + TestUtils.ensureContainsFile(files, output, rs1FileName ); + Assert.assertTrue(new File(output, rs1FileName).exists()); + + // Check not generated cause backwards compatibility files + String ft1FileName = "src/main/java/org/openapitools/model/FT1.java"; + TestUtils.ensureDoesNotContainsFile(files, output, ft1FileName); + Assert.assertFalse(new File(output, ft1FileName).exists()); + + String ft2FileName = "src/main/java/org/openapitools/model/FT2.java"; + TestUtils.ensureDoesNotContainsFile(files, output, ft2FileName); + Assert.assertFalse(new File(output, ft2FileName).exists()); + + String ft3FileName = "src/main/java/org/openapitools/model/FT3.java"; + TestUtils.ensureDoesNotContainsFile(files, output, ft3FileName); + Assert.assertFalse(new File(output, ft3FileName).exists()); + + String bttFileName = "src/main/java/org/openapitools/model/BTT.java"; + TestUtils.ensureDoesNotContainsFile(files, output, bttFileName); + Assert.assertFalse(new File(output, bttFileName).exists()); + + } finally { + output.deleteOnExit(); + if (oldModelsProp != null) { + GlobalSettings.setProperty("models", oldModelsProp); + } else { + GlobalSettings.clearProperty("models"); + } + } + } + + @Test + public void testGenerateRecursiveDependentModelsIssue18444() throws IOException { + Path target = Files.createTempDirectory("test"); + File output = target.toFile(); + String oldModelsProp = GlobalSettings.getProperty("models"); + + try { + DefaultGenerator generator = generatorGenerateRecursiveDependentModelsBackwardCompatibility("true"); + GlobalSettings.setProperty("models", "RQ1,RS1"); + ClientOptInput clientOptInput = createOptInputIssue18444(target); + List files = generator.opts(clientOptInput ).generate(); + Assert.assertEquals(files.size(), 21); + + // Check expected generated files + // api sanity check + String apiJavaFileName = "src/main/java/org/openapitools/api/ApiApi.java"; + TestUtils.ensureContainsFile(files, output, apiJavaFileName); + Assert.assertTrue(new File(output, apiJavaFileName).exists()); + + // model sanity check + String rq1FileName = "src/main/java/org/openapitools/model/RQ1.java"; + TestUtils.ensureContainsFile(files, output, rq1FileName); + Assert.assertTrue(new File(output, rq1FileName).exists()); + + String rs1FileName = "src/main/java/org/openapitools/model/RS1.java"; + TestUtils.ensureContainsFile(files, output, rs1FileName ); + Assert.assertTrue(new File(output, rs1FileName).exists()); + + // Check generated cause RQ1 and RS1 dependents of FT1,FT2,FT3 files + String ft1FileName = "src/main/java/org/openapitools/model/FT1.java"; + TestUtils.ensureContainsFile(files, output, ft1FileName); + Assert.assertTrue(new File(output, ft1FileName).exists()); + + String ft2FileName = "src/main/java/org/openapitools/model/FT2.java"; + TestUtils.ensureContainsFile(files, output, ft2FileName); + Assert.assertTrue(new File(output, ft2FileName).exists()); + + String ft3FileName = "src/main/java/org/openapitools/model/FT3.java"; + TestUtils.ensureContainsFile(files, output, ft3FileName); + Assert.assertTrue(new File(output, ft3FileName).exists()); + + String bttFileName = "src/main/java/org/openapitools/model/BTT.java"; + TestUtils.ensureContainsFile(files, output, bttFileName); + Assert.assertTrue(new File(output, bttFileName).exists()); + + } finally { + output.deleteOnExit(); + if (oldModelsProp != null) { + GlobalSettings.setProperty("models", oldModelsProp); + } else { + GlobalSettings.clearProperty("models"); + } + } + } + } diff --git a/modules/openapi-generator/src/test/resources/bugs/issue_18444.json b/modules/openapi-generator/src/test/resources/bugs/issue_18444.json new file mode 100755 index 000000000000..3509c2b78e4f --- /dev/null +++ b/modules/openapi-generator/src/test/resources/bugs/issue_18444.json @@ -0,0 +1,194 @@ +{ + "openapi": "3.0.1", + "info": { + "title": "OpenAPI definition", + "version": "v0" + }, + "servers": [ + { + "url": "http://localhost/service", + "description": "Generated server url" + } + ], + "paths": { + "/api/v1/subservice/m1": { + "post": { + "tags": [ + "subservice" + ], + "summary": "subservice", + "operationId": "m1", + "parameters": [ + { + "name": "n1", + "in": "header", + "description": "n1 description", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/RQ1" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/RS1" + } + } + } + }, + "403": { + "description": "Forbidden" + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BadRequestError" + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "B_TT": { + "discriminator": { + "propertyName": "otype" + }, + "required": [ + "ident", + "otype" + ], + "type": "object", + "properties": { + "ident": { + "type": "integer", + "format": "int32" + }, + "otype": { + "type": "object" + } + } + }, + "BadRequestError": { + "allOf": [ + { "$ref": "#/components/schemas/B_TT" }, + { + "type": "object", + "properties": { + "status": { + "type": "integer", + "format": "int32", + "default": 400 + }, + "error": { + "type": "string" + }, + "message": { + "type": "string" + } + } + } + ] + }, + "RQ1": { + "allOf": [ + { "$ref": "#/components/schemas/B_TT" }, + { "type": "object", + "properties": { + "f1": { + "$ref": "#/components/schemas/FT1" + }, + "f2": { + "$ref": "#/components/schemas/FT2" + } + } + } + ] + }, + "FT1": { + "required": [ + "code" + ], + "type": "object", + "properties": { + "code": { + "type": "integer", + "format": "int64" + }, + "name": { + "type": "string" + } + } + }, + "FT2": { + "type": "object", + "properties": { + "id": { + "type": "integer", + "format": "int64", + "readOnly": true + }, + "date": { + "type": "string", + "format": "date-time", + "readOnly": true + } + } + }, + "RS1": { + "type": "object", + "properties": { + "f1": { + "type": "array", + "items": { + "$ref": "#/components/schemas/FT3" + } + }, + "f2": { + "type": "integer", + "format": "int32" + } + } + }, + "FT3": { + "required": [ + "f1", + "f2" + ], + "type": "object", + "properties": { + "f2": { + "type": "string", + "enum": [ + "E1", + "E2" + ] + }, + "f1": { + "type": "integer", + "format": "int64" + } + } + } + } + } +}