Merge pull request #1553 from unkish/issues/1550

Prevent generating duplicate constructors when all properties are required; and both includeAllPropertiesConstructor and includeRequiredPropertiesConstructor are true

Author: joelittlejohn

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/ConstructorRule.java

@@ -28,6 +28,7 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.StringJoiner;
+import java.util.stream.Stream;
 
 import org.apache.commons.lang3.StringUtils;
 import org.jsonschema2pojo.GenerationConfig;
@@ -48,6 +49,8 @@
 import com.sun.codemodel.JMod;
 import com.sun.codemodel.JVar;
 
+import static java.util.stream.Collectors.toSet;
+
 public class ConstructorRule implements Rule<JDefinedClass, JDefinedClass> {
 
   private final RuleFactory ruleFactory;
@@ -139,6 +142,10 @@ private void handleMultiChoiceConstructorConfiguration(JsonNode node, JDefinedCl
   }
 
   private void addFieldsConstructor(JDefinedClass instanceClass, Map<String, String> classProperties, Map<String, String> combinedSuperProperties) {
+    if (isConstructorAlreadyAdded(instanceClass, classProperties, combinedSuperProperties)) {
+      return;
+    }
+
     GenerationConfig generationConfig = ruleFactory.getGenerationConfig();
 
     // Generate the constructor with the properties which were located
@@ -153,6 +160,17 @@ private void addFieldsConstructor(JDefinedClass instanceClass, Map<String, Strin
     }
   }
 
+  private boolean isConstructorAlreadyAdded(JDefinedClass instanceClass, Map<String, String> classProperties, Map<String, String> combinedSuperProperties) {
+    final Set<String> allProperties = Stream.of(classProperties.keySet(), combinedSuperProperties.keySet()).flatMap(Set::stream).collect(toSet());
+    for (Iterator<JMethod> constructorIterator = instanceClass.constructors(); constructorIterator.hasNext(); ) {
+      final Set<String> constructorParams = constructorIterator.next().params().stream().map(JVar::name).collect(toSet());
+      if (constructorParams.equals(allProperties)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   private void addCopyConstructor(JDefinedClass instanceClass, Map<String, String> classProperties, Map<String, String> combinedSuperProperties) {
     GenerationConfig generationConfig = ruleFactory.getGenerationConfig();
 

jsonschema2pojo-integration-tests/src/test/java/org/jsonschema2pojo/integration/ConstructorsIT.java

@@ -267,6 +267,24 @@ public void testAllFieldsConstructorAssignsFields() throws Exception {
       assertEquals("provider", getValue(instance, "getProvider"));
       assertEquals("startTime", getValue(instance, "getStarttime"));
     }
+
+    /**
+     * Test that duplicate constructors are not generated (compile time error is not thrown) when:
+     * <ul>
+     *     <li>all properties are required</li>
+     *     <li>{@code includeAllPropertiesConstructor} configuration property is {@code true}</li>
+     *     <li>{@code includeRequiredPropertiesConstructor} configuration property is {@code true}</li>
+     */
+    @Test
+    public void testGeneratesConstructorWithAllPropertiesRequired() throws Exception {
+      classSchemaRule.generate(
+          "/schema/constructors/allPropertiesRequiredConstructor.json",
+          "com.example",
+          config("includeConstructors", true, "includeAllPropertiesConstructor", true, "includeRequiredPropertiesConstructor", true));
+      Class<?> type = classSchemaRule.compile().loadClass("com.example.AllPropertiesRequiredConstructor");
+      assertHasModifier(JMod.PUBLIC, getAllPropertiesConstructor(type).getModifiers(), "public");
+    }
+
   }
 
   /**

jsonschema2pojo-integration-tests/src/test/java/org/jsonschema2pojo/integration/config/UseInnerClassBuildersIT.java

@@ -195,7 +195,7 @@ public void innerBuilderWithAllPropertyConstructor()
    * with the required properties will be created
    */
   @Test
-  public void innerBuilderWithRequiredPropertyConstructor()
+  public void innerBuilderWithRequiredPropertyOnlyConstructor()
       throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException, InstantiationException {
     ClassLoader resultsClassLoader = schemaRule.generateAndCompile("/schema.useInnerClassBuilders/child.json", "com.example",
         config("generateBuilders", true, "useInnerClassBuilders", true, "includeConstructors", true, "constructorsRequiredPropertiesOnly", true));
@@ -225,6 +225,47 @@ public void innerBuilderWithRequiredPropertyConstructor()
     assertEquals(sharedProperty, getSharedProperty.invoke(childObject));
   }
 
+  /**
+   * This method confirms that duplicate constructors are not generated (compile time error is not thrown) when:
+   * <ul>
+   *     <li>all properties are required</li>
+   *     <li>{@code includeAllPropertiesConstructor} configuration property is {@code true}</li>
+   *     <li>{@code includeRequiredPropertiesConstructor} configuration property is {@code true}</li>
+   */
+  @Test
+  public void innerBuilderWithRequiredPropertyConstructor() throws ReflectiveOperationException {
+    ClassLoader resultsClassLoader = schemaRule.generateAndCompile(
+            "/schema.useInnerClassBuilders/child_parent_all_required.json",
+            "com.example",
+            config("generateBuilders", true,
+                    "useInnerClassBuilders", true,
+                    "includeConstructors", true,
+                    "includeAllPropertiesConstructor", true,
+                    "includeRequiredPropertiesConstructor", true));
+
+    Class<?> builderClass = resultsClassLoader.loadClass("com.example.ChildParentAllRequired$ChildParentAllRequiredBuilder");
+    Constructor<?> constructor = builderClass.getConstructor(String.class, String.class);
+    Method buildMethod = builderClass.getMethod("build");
+    Method withChildProperty = builderClass.getMethod("withChildProperty", Integer.class);
+
+    int childProperty = 1;
+    String parentProperty = "parentProperty";
+    String sharedProperty = "sharedProperty";
+
+    Object builder = constructor.newInstance(sharedProperty, parentProperty);
+    withChildProperty.invoke(builder, childProperty);
+    Object childObject = buildMethod.invoke(builder);
+
+    Class<?> childClass = resultsClassLoader.loadClass("com.example.ChildParentAllRequired");
+    Method getChildProperty = childClass.getMethod("getChildProperty");
+    Method getParentProperty = childClass.getMethod("getParentProperty");
+    Method getSharedProperty = childClass.getMethod("getSharedProperty");
+
+    assertEquals(childProperty, getChildProperty.invoke(childObject));
+    assertEquals(parentProperty, getParentProperty.invoke(childObject));
+    assertEquals(sharedProperty, getSharedProperty.invoke(childObject));
+  }
+
   /**
    * This method confirms that if innerBuilders are used, a "builder" method is created on the class that returns an instance of the builder
    */

jsonschema2pojo-integration-tests/src/test/resources/schema.useInnerClassBuilders/child_parent_all_required.json

@@ -0,0 +1,15 @@
+{
+  "type": "object",
+  "extends": {
+    "$ref": "parent_all_required.json"
+  },
+  "properties": {
+    "childProperty": {
+      "type": "integer"
+    },
+    "sharedProperty": {
+      "type": "string",
+      "required": true
+    }
+  }
+}

jsonschema2pojo-integration-tests/src/test/resources/schema.useInnerClassBuilders/parent_all_required.json

@@ -0,0 +1,8 @@
+{
+  "type" : "object",
+  "properties" : {
+    "parentProperty" : { "type" : "string" },
+    "sharedProperty" : { "type" : "string" }
+  },
+  "required" : ["parentProperty", "sharedProperty"]
+}

jsonschema2pojo-integration-tests/src/test/resources/schema/constructors/allPropertiesRequiredConstructor.json

@@ -0,0 +1,26 @@
+{
+  "type" : "object",
+  "properties" : {
+    "type" : {
+      "type" : "string",
+      "default" : "event",
+      "required": true
+    },
+    "id" : {
+      "type" : "integer",
+      "required": true
+    },
+    "has_tickets" : {
+      "type" : "boolean",
+      "required": true
+    },
+    "provider" : {
+      "type" : "string",
+      "required": true
+    },
+    "starttime" : {
+      "type" : "string",
+      "required": true
+    }
+  }
+}