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

allArgConstructor for java #18405

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
1 change: 1 addition & 0 deletions bin/configs/java-resttemplate-jakarta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ additionalProperties:
hideGenerationTimestamp: "true"
java8: true
useJakartaEe: true
generatedConstructorWithAllArgs: true
1 change: 1 addition & 0 deletions bin/configs/java-resttemplate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ additionalProperties:
hideGenerationTimestamp: "true"
java8: true
containerDefaultToNull: true
generatedConstructorWithAllArgs: true
1 change: 1 addition & 0 deletions bin/configs/spring-boot-3.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ additionalProperties:
useBeanValidation: true
withXml: true
hideGenerationTimestamp: "true"
generatedConstructorWithAllArgs: true
1 change: 1 addition & 0 deletions bin/configs/spring-boot-delegate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ additionalProperties:
hideGenerationTimestamp: "true"
java8: true
delegatePattern: "true"
generatedConstructorWithAllArgs: true
1 change: 1 addition & 0 deletions bin/configs/spring-boot-lombok-data.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ additionalProperties:
artifactId: springboot-lombok-data
hideGenerationTimestamp: "true"
additionalModelTypeAnnotations: "@lombok.Data;@lombok.Builder;@lombok.NoArgsConstructor;@lombok.AllArgsConstructor"
generatedConstructorWithAllArgs: true
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import java.util.stream.StreamSupport;

import static org.openapitools.codegen.utils.CamelizeOption.*;
import static org.openapitools.codegen.utils.OnceLogger.once;
import static org.openapitools.codegen.utils.StringUtils.*;

public abstract class AbstractJavaCodegen extends DefaultCodegen implements CodegenConfig,
Expand Down Expand Up @@ -93,6 +94,7 @@ public abstract class AbstractJavaCodegen extends DefaultCodegen implements Code
public static final String USE_ONE_OF_INTERFACES = "useOneOfInterfaces";
public static final String LOMBOK = "lombok";
public static final String DEFAULT_TEST_FOLDER = "${project.build.directory}/generated-test-sources/openapi";
public static final String GENERATE_CONSTRUCTOR_WITH_ALL_ARGS = "generatedConstructorWithAllArgs";

protected String dateLibrary = "java8";
protected boolean supportAsync = false;
Expand Down Expand Up @@ -142,7 +144,7 @@ public abstract class AbstractJavaCodegen extends DefaultCodegen implements Code
protected boolean camelCaseDollarSign = false;
protected boolean useJakartaEe = false;
protected boolean containerDefaultToNull = false;

protected boolean generatedConstructorWithAllArgs = false;
private Map<String, String> schemaKeyToModelNameCache = new HashMap<>();

public AbstractJavaCodegen() {
Expand Down Expand Up @@ -282,6 +284,7 @@ public AbstractJavaCodegen() {
cliOptions.add(CliOption.newBoolean(CAMEL_CASE_DOLLAR_SIGN, "Fix camelCase when starting with $ sign. when true : $Value when false : $value"));
cliOptions.add(CliOption.newBoolean(USE_JAKARTA_EE, "whether to use Jakarta EE namespace instead of javax"));
cliOptions.add(CliOption.newBoolean(CONTAINER_DEFAULT_TO_NULL, "Set containers (array, set, map) default to null"));
cliOptions.add(CliOption.newBoolean(GENERATE_CONSTRUCTOR_WITH_ALL_ARGS, "whether to generate a constructor for all arguments").defaultValue(Boolean.FALSE.toString()));

cliOptions.add(CliOption.newString(CodegenConstants.PARENT_GROUP_ID, CodegenConstants.PARENT_GROUP_ID_DESC));
cliOptions.add(CliOption.newString(CodegenConstants.PARENT_ARTIFACT_ID, CodegenConstants.PARENT_ARTIFACT_ID_DESC));
Expand Down Expand Up @@ -355,6 +358,10 @@ public void processOpts() {
additionalProperties.put(ANNOTATION_LIBRARY, AnnotationLibrary.NONE);
}

if (additionalProperties.containsKey(GENERATE_CONSTRUCTOR_WITH_ALL_ARGS)) {
this.setGeneratedConstructorWithAllArgs(convertPropertyToBoolean(GENERATE_CONSTRUCTOR_WITH_ALL_ARGS));
}
writePropertyBack(GENERATE_CONSTRUCTOR_WITH_ALL_ARGS, generatedConstructorWithAllArgs);

if (StringUtils.isEmpty(System.getenv("JAVA_POST_PROCESS_FILE"))) {
LOGGER.info("Environment variable JAVA_POST_PROCESS_FILE not defined so the Java code may not be properly formatted. To define it, try 'export JAVA_POST_PROCESS_FILE=\"/usr/local/bin/clang-format -i\"' (Linux/Mac)");
Expand Down Expand Up @@ -677,11 +684,26 @@ public void processOpts() {
});
}

public void setGeneratedConstructorWithAllArgs(boolean aValue) {
this.generatedConstructorWithAllArgs = aValue;
}

public boolean isGeneratedConstructorWithAllArgs() {
return generatedConstructorWithAllArgs;
}

/**
* Analyse and post process all Models.
* @param objs the models map.
* @return the processed models map.
**/
@Override
public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs) {
objs = super.postProcessAllModels(objs);
objs = super.updateAllModels(objs);

Map<String, CodegenModel> allModels = getAllModels(objs);

if (!additionalModelTypeAnnotations.isEmpty()) {
for (String modelName : objs.keySet()) {
Map<String, Object> models = objs.get(modelName);
Expand All @@ -703,9 +725,101 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
}
}

/*
Add parentVars and parentRequiredVars to every Model which has a parent.
Add isInherited to every model which has children.
This allows
the generation of fluent setter methods for inherited properties
the generation of all arg constructors
ps: This code was specific to SpringCodeGen and now is available to all java generators.
*/
for (ModelsMap modelsAttrs : objs.values()) {
for (ModelMap mo : modelsAttrs.getModels()) {
CodegenModel codegenModel = mo.getModel();
Set<String> inheritedImports = new HashSet<>();
Map<String, CodegenProperty> propertyHash = new HashMap<>(codegenModel.vars.size());
for (final CodegenProperty property : codegenModel.vars) {
propertyHash.put(property.name, property);
}
List<CodegenModel> parentModelList = getParentModelList(codegenModel);
for (CodegenModel parentCodegenModel: parentModelList) {
for (final CodegenProperty property : parentCodegenModel.vars) {
// helper list of parentVars simplifies templating
if (!propertyHash.containsKey(property.name)) {
propertyHash.put(property.name, property);
final CodegenProperty parentVar = property.clone();
parentVar.isInherited = true;
LOGGER.info("adding parent variable {} to {}", property.name, codegenModel.name);
codegenModel.parentVars.add(parentVar);
Set<String> imports = parentVar.getImports(true, this.importBaseType, generatorMetadata.getFeatureSet()).stream().filter(Objects::nonNull).collect(Collectors.toSet());
for (String imp : imports) {
// Avoid dupes
if (!codegenModel.getImports().contains(imp)) {
inheritedImports.add(imp);
codegenModel.getImports().add(imp);
}
}
}
}
}
if (codegenModel.getParentModel() != null) {
codegenModel.parentRequiredVars = new ArrayList<>(codegenModel.getParentModel().requiredVars);
}
// There must be a better way ...
for (String imp: inheritedImports) {
String qimp = importMapping().get(imp);
if (qimp != null) {
Map<String,String> toAdd = new HashMap<>();
toAdd.put("import", qimp);
modelsAttrs.getImports().add(toAdd);
}
}
}
}

if (isGeneratedConstructorWithAllArgs()) {
// conditionally force the generation of all args constructor.
for (CodegenModel cm : allModels.values()) {
if (isConstructorWithAllArgsAllowed(cm)) {
cm.vendorExtensions.put("generatedConstructorWithAllArgs", true);
List<Object> constructorArgs = new ArrayList<>();
// vendorExtensions.allArgsConstructorVars should be equivalent to allVars
// but it is not reliable if openapiNormalizer.REFACTOR_ALLOF_WITH_PROPERTIES_ONLY is disabled
if (cm.vars.size() + cm.parentVars.size() != cm.allVars.size()) {
once(LOGGER).warn("Unexpected allVars for {} expecting:{} vars. actual:{} vars", cm.name, cm.vars.size() + cm.parentVars.size(), cm.allVars.size());
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need this warning as vars+ parentVars may not be equal to allVars when vars and parentVars have overlayed properties (e.g. both child and parent model has a property named id defined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a sample? I don't reach line 789 when I generate all arg constructors using allOfDuplicatedProperties.yaml or allOfMappingDuplicatedProperties.yaml

Even when I add x-parent: true, allVars are still ok (but the code is uncompilable because the overriden getter)

I reach it from SpringCodegenTest.generateAllArgsConstructor() for Cat.
It might be something similar to #18340

Copy link
Member

Choose a reason for hiding this comment

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

if (cm.vars.size() + cm.parentVars.size() != cm.allVars.size()) {

let's step back for a moment. did you encounter such condition before? if yes, can you please share the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find more evidences in #18340

So the merge of parentVars and allVars is needed to ensure that the constructors are correctly generated

}
cm.vendorExtensions.put("allArgsConstructorVars", constructorArgs);
Copy link
Member

Choose a reason for hiding this comment

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

for extensions, please add x- as the prefix and use dash case

e.g. x-java-all-args-constructor-vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

constructorArgs.addAll(cm.vars);
constructorArgs.addAll(cm.parentVars);
}
}
}

return objs;
}

private List<CodegenModel> getParentModelList(CodegenModel codegenModel) {
CodegenModel parentCodegenModel = codegenModel.parentModel;
List<CodegenModel> parentModelList = new ArrayList<>();
while (parentCodegenModel != null) {
parentModelList.add(parentCodegenModel);
parentCodegenModel = parentCodegenModel.parentModel;
}
return parentModelList;
}

/**
* trigger the generation of all arguments constructor or not.
* It avoids generating the same constructor twice.
*
* @return true if an allArgConstructor must be generated
*/
protected boolean isConstructorWithAllArgsAllowed(CodegenModel codegenModel) {
// implementation detail: allVars is not reliable if openapiNormalizer.REFACTOR_ALLOF_WITH_PROPERTIES_ONLY is disabled
return (this.generatedConstructorWithAllArgs &&
(!codegenModel.vars.isEmpty() || codegenModel.parentVars.isEmpty()));
}

private void sanitizeConfig() {
// Sanitize any config options here. We also have to update the additionalProperties because
// the whole additionalProperties object is injected into the main object passed to the mustache layer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@

import java.io.File;
import java.util.*;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -1148,6 +1147,15 @@ public ModelsMap postProcessModels(ModelsMap objs) {
return objs;
}

@Override
protected boolean isConstructorWithAllArgsAllowed(CodegenModel codegenModel) {
// implementation detail: allVars is not reliable if openapiNormalizer.REFACTOR_ALLOF_WITH_PROPERTIES_ONLY is disabled
if (codegenModel.readOnlyVars.size() != codegenModel.vars.size() + codegenModel.parentVars.size()) {
return super.isConstructorWithAllArgsAllowed(codegenModel);
}
return false;
}

public void setUseOneOfDiscriminatorLookup(boolean useOneOfDiscriminatorLookup) {
this.useOneOfDiscriminatorLookup = useOneOfDiscriminatorLookup;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1168,66 +1168,19 @@ public CodegenModel fromModel(String name, Schema model) {
return codegenModel;
}

/**
* Analyse and post process all Models.
* Add parentVars to every Model which has a parent. This allows to generate
* fluent setter methods for inherited properties.
* @param objs the models map.
* @return the processed models map.
*/
@Override
public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs) {
objs = super.postProcessAllModels(objs);
objs = super.updateAllModels(objs);

for (ModelsMap modelsAttrs : objs.values()) {
for (ModelMap mo : modelsAttrs.getModels()) {
CodegenModel codegenModel = mo.getModel();
Set<String> inheritedImports = new HashSet<>();
Map<String, CodegenProperty> propertyHash = new HashMap<>(codegenModel.vars.size());
for (final CodegenProperty property : codegenModel.vars) {
propertyHash.put(property.name, property);
}
CodegenModel parentCodegenModel = codegenModel.parentModel;
while (parentCodegenModel != null) {
for (final CodegenProperty property : parentCodegenModel.vars) {
// helper list of parentVars simplifies templating
if (!propertyHash.containsKey(property.name)) {
propertyHash.put(property.name, property);
final CodegenProperty parentVar = property.clone();
parentVar.isInherited = true;
LOGGER.info("adding parent variable {}", property.name);
codegenModel.parentVars.add(parentVar);
Set<String> imports = parentVar.getImports(true, this.importBaseType, generatorMetadata.getFeatureSet()).stream().filter(Objects::nonNull).collect(Collectors.toSet());
for (String imp: imports) {
// Avoid dupes
if (!codegenModel.getImports().contains(imp)) {
inheritedImports.add(imp);
codegenModel.getImports().add(imp);
}
}
}
}
parentCodegenModel = parentCodegenModel.getParentModel();
}
if (codegenModel.getParentModel() != null) {
codegenModel.parentRequiredVars = new ArrayList<>(codegenModel.getParentModel().requiredVars);
}
// There must be a better way ...
for (String imp: inheritedImports) {
String qimp = importMapping().get(imp);
if (qimp != null) {
Map<String,String> toAdd = new HashMap<>();
toAdd.put("import", qimp);
modelsAttrs.getImports().add(toAdd);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

fyi above code block refactored into abstract java codegen.

@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08)

protected boolean isConstructorWithAllArgsAllowed(CodegenModel codegenModel) {
if (lombokAnnotations != null && lombokAnnotations.containsKey("allArgsConstructor")) {
// constructor generated by lombok
return false;
}
return objs;
if ((!generatedConstructorWithRequiredArgs && !codegenModel.vars.isEmpty() )
|| !codegenModel.optionalVars.isEmpty()) {
return super.isConstructorWithAllArgsAllowed(codegenModel);
}
return false;
}


/*
* Add dynamic imports based on the parameters and vendor extensions of an operation.
* The imports are expanded by the mustache {{import}} tag available to model and api
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,4 +170,4 @@ public class {{classname}} {{#parent}}extends {{{.}}}{{/parent}}{{#vendorExtensi
}
return o.toString().replace("\n", "\n ");
}
}
}
30 changes: 23 additions & 7 deletions modules/openapi-generator/src/main/resources/Java/pojo.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,18 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
{{/vendorExtensions.x-field-extra-annotation}}
{{#vendorExtensions.x-is-jackson-optional-nullable}}
{{#isContainer}}
private JsonNullable<{{{datatypeWithEnum}}}> {{name}} = JsonNullable.<{{{datatypeWithEnum}}}>undefined();
{{#hasChildren}}protected{{/hasChildren}}{{^hasChildren}}private{{/hasChildren}} JsonNullable<{{{datatypeWithEnum}}}> {{name}} = JsonNullable.<{{{datatypeWithEnum}}}>undefined();
{{/isContainer}}
{{^isContainer}}
private JsonNullable<{{{datatypeWithEnum}}}> {{name}} = JsonNullable.<{{{datatypeWithEnum}}}>{{#defaultValue}}of({{{.}}}){{/defaultValue}}{{^defaultValue}}undefined(){{/defaultValue}};
{{#hasChildren}}protected{{/hasChildren}}{{^hasChildren}}private{{/hasChildren}} JsonNullable<{{{datatypeWithEnum}}}> {{name}} = JsonNullable.<{{{datatypeWithEnum}}}>{{#defaultValue}}of({{{.}}}){{/defaultValue}}{{^defaultValue}}undefined(){{/defaultValue}};
{{/isContainer}}
{{/vendorExtensions.x-is-jackson-optional-nullable}}
{{^vendorExtensions.x-is-jackson-optional-nullable}}
{{#isContainer}}
private {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
{{#hasChildren}}protected{{/hasChildren}}{{^hasChildren}}private{{/hasChildren}} {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
{{/isContainer}}
{{^isContainer}}
{{#isDiscriminator}}protected{{/isDiscriminator}}{{^isDiscriminator}}private{{/isDiscriminator}} {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
{{#hasChildren}}protected{{/hasChildren}}{{^hasChildren}}private{{/hasChildren}} {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
{{/isContainer}}
{{/vendorExtensions.x-is-jackson-optional-nullable}}

Expand All @@ -114,7 +114,9 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
}
{{#vendorExtensions.x-has-readonly-properties}}
{{^withXml}}

/**
* Constructor with only readonly parameters{{#generatedConstructorWithAllArgs}}{{^vendorExtensions.generatedConstructorWithAllArgs}} and all parameters{{/vendorExtensions.generatedConstructorWithAllArgs}}{{/generatedConstructorWithAllArgs}}
*/
{{#jsonb}}@JsonbCreator{{/jsonb}}{{#jackson}}@JsonCreator{{/jackson}}
public {{classname}}(
{{#readOnlyVars}}
Expand All @@ -128,8 +130,22 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
}
{{/withXml}}
{{/vendorExtensions.x-has-readonly-properties}}
{{#vendorExtensions.generatedConstructorWithAllArgs}}

/**
* Constructor with all args parameters
*/
public {{classname}}({{#vendorExtensions.allArgsConstructorVars}}{{#jsonb}}@JsonbProperty(value = "{{baseName}}"{{^required}}, nullable = true{{/required}}){{/jsonb}}{{#jackson}}@JsonProperty(JSON_PROPERTY_{{nameInSnakeCase}}){{/jackson}} {{{datatypeWithEnum}}} {{name}}{{^-last}}, {{/-last}}{{/vendorExtensions.allArgsConstructorVars}}) {
{{#parent}}
super({{#parentVars}}{{name}}{{^-last}}, {{/-last}}{{/parentVars}});
{{/parent}}
{{#vars}}
this.{{name}} = {{#vendorExtensions.x-is-jackson-optional-nullable}}{{name}} == null ? JsonNullable.<{{{datatypeWithEnum}}}>undefined() : JsonNullable.of({{name}}){{/vendorExtensions.x-is-jackson-optional-nullable}}{{^vendorExtensions.x-is-jackson-optional-nullable}}{{name}}{{/vendorExtensions.x-is-jackson-optional-nullable}};
{{/vars}}
}
{{/vendorExtensions.generatedConstructorWithAllArgs}}

{{#vars}}
{{^isReadOnly}}
public {{classname}} {{name}}({{{datatypeWithEnum}}} {{name}}) {
{{#vendorExtensions.x-is-jackson-optional-nullable}}this.{{name}} = JsonNullable.<{{{datatypeWithEnum}}}>of({{name}});{{/vendorExtensions.x-is-jackson-optional-nullable}}
Expand Down Expand Up @@ -279,7 +295,7 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens

{{/vars}}
{{#parent}}
{{#allVars}}
{{#readWriteVars}}
{{#isOverridden}}
@Override
public {{classname}} {{name}}({{{datatypeWithEnum}}} {{name}}) {
Expand All @@ -293,7 +309,7 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
}

{{/isOverridden}}
{{/allVars}}
{{/readWriteVars}}
{{/parent}}
@Override
public boolean equals(Object o) {
Expand Down
Loading