Merge pull request #353 from klum-dsl/feature/325-add-with-strategy

Single Object DSL create and Map DSL create now add existing values

CHANGES.md

@@ -27,6 +27,7 @@
   - Owner objects can be converted before handing them to owner fields or methods (see [Owner Converters](https://github.com/klum-dsl/klum-ast/wiki/Basics#owner-converters) and [#189](https://github.com/klum-dsl/klum-ast/issues/189))
   - New `@Role` annotation to infer the name of the owner field containing an object (see [Role fields](https://github.com/klum-dsl/klum-ast/wiki/Layer3#role-fields) and [#86](https://github.com/klum-dsl/klum-ast/issues/86))
 - Overwrite strategies for `copyFrom` and templates (see [Copy Strategies](https://github.com/klum-dsl/klum-ast/wiki/Copy-Strategies) and [#309](https://github.com/klum-dsl/klum-ast/issues/309))
+- Multiple calls to a single object closure now configure the same object instead of completely overriding the previous field, the same for map entries using the same key. (see [#325](https://github.com/klum-dsl/klum-ast/issues/325)). While this is a more natural behaviour, it might break existing code in some corner cases, see [Migration](https://github.com/klum-dsl/klum-ast/wiki/Migration)).
 
 ## Improvements
 - Creator classes also support methods creating multiple instances at once (see [#319](https://github.com/klum-dsl/klum-ast/issues/319))

klum-ast-runtime/src/main/java/com/blackbuild/klum/ast/util/KlumInstanceProxy.java

@@ -254,15 +254,35 @@ public Set<Object> getOwners() {
     public <T> T createSingleChild(Map<String, Object> namedParams, String fieldOrMethodName, Class<T> type, String key, Closure<T> body) {
         try {
             BreadcrumbCollector.getInstance().enter(fieldOrMethodName, key);
+
+            T existingValue = null;
+
             Optional<? extends AnnotatedElement> fieldOrMethod = DslHelper.getField(instance.getClass(), fieldOrMethodName);
 
             if (fieldOrMethod.isEmpty())
                 fieldOrMethod = DslHelper.getVirtualSetter(getRwInstance().getClass(), fieldOrMethodName, type);
+            else
+                existingValue = getInstanceAttribute(fieldOrMethodName);
 
             if (fieldOrMethod.isEmpty())
                 throw new GroovyRuntimeException(format("Neither field nor single argument method named %s with type %s found in %s", fieldOrMethodName, type, instance.getClass()));
 
             String effectiveKey = resolveKeyForFieldFromAnnotation(fieldOrMethodName, fieldOrMethod.get()).orElse(key);
+
+            if (existingValue != null) {
+                if (!Objects.equals(effectiveKey, getProxyFor(existingValue).getNullableKey()))
+                    throw new IllegalArgumentException(
+                            format("Key mismatch: %s != %s, either use '%s.apply()' to keep existing object or explicitly create and assign a new object.",
+                                    effectiveKey, getProxyFor(existingValue).getNullableKey(), fieldOrMethodName));
+
+                if (type != existingValue.getClass() && type != ((Field) fieldOrMethod.get()).getType())
+                    throw new IllegalArgumentException(
+                            format("Type mismatch: %s != %s, either use '%s.apply()' to keep existing object or explicitly create and assign a new object.",
+                            type, existingValue.getClass(), fieldOrMethodName));
+
+                return (T) getProxyFor(existingValue).apply(namedParams, body);
+            }
+
             T created = createNewInstanceFromParamsAndClosure(type, effectiveKey, namedParams, body);
             return callSetterOrMethod(fieldOrMethodName, created);
         } finally {
@@ -434,6 +454,16 @@ public void addElementsToMap(String fieldName, Object... values) {
     public <T> T addNewDslElementToMap(Map<String, Object> namedParams, String mapName, Class<? extends T> type, String key, Closure<T> body) {
         try {
             BreadcrumbCollector.getInstance().enter(mapName, key);
+
+            T existing = ((Map<String, T>) getInstanceAttributeOrGetter(mapName)).get(key);
+            if (existing != null) {
+                if (type != existing.getClass() && type != getElementTypeOfField(instance.getClass(), mapName))
+                    throw new IllegalArgumentException(
+                            format("Type mismatch: %s != %s, either use 'apply()' to keep existing object or explicitly create and assign a new object.",
+                                    type, existing.getClass()));
+                return (T) getProxyFor(existing).apply(namedParams, body);
+            }
+
             T created = createNewInstanceFromParamsAndClosure(type, key, namedParams, body);
             return doAddElementToMap(mapName, key, created);
         } finally {

klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/OverwriteStrategyTest.groovy

@@ -342,4 +342,125 @@ class OverwriteStrategyTest extends AbstractDSLSpec {
         MERGE_VALUES   | [a: [foo: 1, bar: 2], b: [foo: 1, bar: 2]] | [:]                                        || [a: [foo: 1, bar: 2], b: [foo: 1, bar: 2]]
     }
 
+    @Issue("325")
+    def "dsl single adder should merge with existing objects"() {
+        given:
+        createClass """
+            package pk
+
+            import com.blackbuild.klum.ast.util.copy.Overwrite
+            import com.blackbuild.klum.ast.util.copy.OverwriteStrategy
+            
+            @DSL class Inner {
+                Integer foo
+                Integer bar 
+            }
+
+            @DSL class Outer {
+                Inner inner 
+            }
+        """
+
+        when:
+        def template = Outer.Create.Template {
+            inner {
+                foo 1
+            }
+        }
+
+        Outer.withTemplate(template) {
+            instance = Outer.Create.With {
+                inner {
+                    bar 2
+                }
+            }
+        }
+
+        then:
+        instance.inner.foo == 1
+        instance.inner.bar == 2
+    }
+
+    @Issue("325")
+    def "dsl single adder should use merge even if another strategy is configured"() {
+        given:
+        createClass """
+            package pk
+
+            import com.blackbuild.klum.ast.util.copy.Overwrite
+            import com.blackbuild.klum.ast.util.copy.OverwriteStrategy
+            
+            @DSL class Inner {
+                Integer foo
+                Integer bar 
+            }
+
+            @DSL class Outer {
+                @Overwrite.Single(OverwriteStrategy.Single.SET_IF_NULL)
+                Inner inner 
+            }
+        """
+
+        when:
+        def template = Outer.Create.Template {
+            inner {
+                foo 1
+            }
+        }
+
+        Outer.withTemplate(template) {
+            instance = Outer.Create.With {
+                inner {
+                    bar 2
+                }
+            }
+        }
+
+        then:
+        instance.inner.foo == 1
+        instance.inner.bar == 2
+    }
+
+    @Issue("325")
+
+    def "dsl map adder should merge with existing objects"() {
+        given:
+        createClass """
+            package pk
+
+            import com.blackbuild.klum.ast.util.copy.Overwrite
+            import com.blackbuild.klum.ast.util.copy.OverwriteStrategy
+            
+            @DSL class Inner {
+                @Key String key
+                Integer foo
+                Integer bar 
+            }
+
+            @DSL class Outer {
+                Map<String, Inner> inners 
+            }
+        """
+
+        when:
+        def template = Outer.Create.Template {
+            inner("a") {
+                foo 1
+            }
+        }
+
+        def innerInstance
+        Outer.withTemplate(template) {
+            instance = Outer.Create.With {
+                inner("a") {
+                    bar 2
+                }
+            }
+        }
+
+        then:
+        instance.inners.a.foo == 1
+        instance.inners.a.bar == 2
+    }
+
 }