From f40fc454e14ded7520907e58fd4e0604bbec0816 Mon Sep 17 00:00:00 2001 From: Stephan Pauxberger Date: Thu, 16 Jan 2025 16:19:48 +0100 Subject: [PATCH] Single Object DSL create and Map DSL create now add existing values Resolves #325 --- CHANGES.md | 1 + .../klum/ast/util/KlumInstanceProxy.java | 30 ++ .../transform/OverwriteStrategyTest.groovy | 121 ++++++++ .../configdsl/transform/TransformSpec.groovy | 264 +++++++++++++++++- wiki/Basics.md | 2 +- wiki/Migration.md | 80 +++++- wiki/Templates.md | 4 +- 7 files changed, 493 insertions(+), 9 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index eb96084a..e3869c0d 100644 --- a/CHANGES.md +++ b/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)) diff --git a/klum-ast-runtime/src/main/java/com/blackbuild/klum/ast/util/KlumInstanceProxy.java b/klum-ast-runtime/src/main/java/com/blackbuild/klum/ast/util/KlumInstanceProxy.java index 474cf3c2..b09f258f 100644 --- a/klum-ast-runtime/src/main/java/com/blackbuild/klum/ast/util/KlumInstanceProxy.java +++ b/klum-ast-runtime/src/main/java/com/blackbuild/klum/ast/util/KlumInstanceProxy.java @@ -254,15 +254,35 @@ public Set getOwners() { public T createSingleChild(Map namedParams, String fieldOrMethodName, Class type, String key, Closure body) { try { BreadcrumbCollector.getInstance().enter(fieldOrMethodName, key); + + T existingValue = null; + Optional 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 addNewDslElementToMap(Map namedParams, String mapName, Class type, String key, Closure body) { try { BreadcrumbCollector.getInstance().enter(mapName, key); + + T existing = ((Map) 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 { diff --git a/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/OverwriteStrategyTest.groovy b/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/OverwriteStrategyTest.groovy index 43fee9e4..e571d879 100644 --- a/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/OverwriteStrategyTest.groovy +++ b/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 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 + } + } \ No newline at end of file diff --git a/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/TransformSpec.groovy b/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/TransformSpec.groovy index 134430ed..7dd93af7 100644 --- a/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/TransformSpec.groovy +++ b/klum-ast/src/test/groovy/com/blackbuild/groovy/configdsl/transform/TransformSpec.groovy @@ -503,7 +503,7 @@ class TransformSpec extends AbstractDSLSpec { def "create inner object via key and closure"() { given: - createInstance(''' + createClass(''' package pk @DSL @@ -520,7 +520,7 @@ class TransformSpec extends AbstractDSLSpec { when: def innerInstance - instance.apply { + instance = Foo.Create.With { innerInstance = inner("Dieter") { value 15 } @@ -534,7 +534,7 @@ class TransformSpec extends AbstractDSLSpec { innerInstance != null when: "Allow named arguments for simple objects" - instance.apply { + instance = Foo.Create.With { innerInstance = inner("Hans", value: 16) } @@ -543,6 +543,143 @@ class TransformSpec extends AbstractDSLSpec { innerInstance.value == 16 } + @Issue("325") + def "create with double apply should be retained if keys are identical"() { + given: + createClass(''' + package pk + + @DSL + class Foo { + Bar inner + } + + @DSL + class Bar { + @Key String name + int value + } + ''') + + when: + def innerInstance + def secondInnerInstance + instance = Foo.Create.With { + innerInstance = inner("Dieter") { + value 15 + } + secondInnerInstance = inner("Dieter") { + value 1 + } + } + + then: + instance.inner.name == "Dieter" + instance.inner.value == 1 + innerInstance.is(secondInnerInstance) + + when: "second apply with different key" + instance = Foo.Create.With { + inner("Dieter") { + value 15 + } + inner("Hans") { + value 1 + } + } + + then: + def e = thrown(IllegalArgumentException) + e.message == "Key mismatch: Hans != Dieter, either use 'inner.apply()' to keep existing object or explicitly create and assign a new object." + + when: "second apply with different key and explicit set" + def Bar = getClass("pk.Bar") // need a local variable, since propertyMissing of Test is not called with Delegate Only + instance = Foo.Create.With { + innerInstance = inner("Dieter") { + value 15 + } + inner = Bar.Create.With("Hans") { + value 1 + } + } + + then: + !innerInstance.is(instance.inner) + instance.inner.name == "Hans" + instance.inner.value == 1 + } + + @Issue("325") + def "create with double apply fail if different classes are used"() { + given: + createClass(''' + package pk + + @DSL + class Foo { + Bar inner + } + + @DSL + class Bar { + int value + } + + @DSL + class Bier extends Bar { + } + ''') + def Bar = getClass("pk.Bar") + def Bier = getClass("pk.Bier") + + when: "second apply with different class" + def innerInstance + def secondInnerInstance + instance = Foo.Create.With { + innerInstance = inner { + value 15 + } + secondInnerInstance = inner(Bier) { + value 1 + } + } + + then: + thrown(IllegalArgumentException) + + when: "second apply uses default class" + instance = Foo.Create.With { + innerInstance = inner(Bier) { + value 15 + } + secondInnerInstance = inner { + value 1 + } + } + + then: + instance.inner.value == 1 + innerInstance.is(secondInnerInstance) + instance.inner.is(innerInstance) + instance.inner.getClass() == Bier + + when: "both applys use the same subclass" + instance = Foo.Create.With { + innerInstance = inner(Bier) { + value 15 + } + secondInnerInstance = inner(Bier) { + value 1 + } + } + + then: + instance.inner.value == 1 + innerInstance.is(secondInnerInstance) + instance.inner.is(innerInstance) + instance.inner.getClass() == Bier + } + def "create list of inner objects"() { given: createInstance(''' @@ -1201,6 +1338,126 @@ import org.codehaus.groovy.control.CompilePhase instance.bars.Felix.url == "4" } + @Issue("325") + def "create map of inner objects merges to existing objects"() { + given: + createClass(''' + package pk + + @DSL + class Foo { + Map bars + } + + @DSL + class Bar { + @Key String name + String foo + String bar + } + ''') + + when: + def firstApply + def secondApply + instance = Foo.Create.With { + bars { + firstApply = bar("Dieter") { + foo "fromFirst" + } + secondApply = bar("Dieter") { + bar "fromSecond" + } + } + } + + then: + instance.bars.Dieter.foo == "fromFirst" + instance.bars.Dieter.bar == "fromSecond" + firstApply.is(secondApply) + instance.bars.Dieter.is(firstApply) + } + + @Issue("325") + def "create map of inner objects merge fails when using incompatible subclasses"() { + given: + createClass(''' + package pk + + @DSL + class Foo { + Map bars + } + + @DSL + class Bar { + @Key String name + String foo + String bar + } + + @DSL class Bier extends Bar { + } + ''') + def Bar = getClass("pk.Bar") + def Bier = getClass("pk.Bier") + + when: + def firstApply + def secondApply + instance = Foo.Create.With { + bars { + firstApply = bar(Bier, "Dieter") { + foo "fromFirst" + } + secondApply = bar("Dieter") { + bar "fromSecond" + } + } + } + + then: "Works, because the second apply uses the default class" + instance.bars.Dieter.foo == "fromFirst" + instance.bars.Dieter.bar == "fromSecond" + firstApply.is(secondApply) + instance.bars.Dieter.is(firstApply) + instance.bars.Dieter.getClass() == Bier + + when: + instance = Foo.Create.With { + bars { + firstApply = bar("Dieter") { + foo "fromFirst" + } + secondApply = bar(Bier, "Dieter") { + bar "fromSecond" + } + } + } + + then: "fails, because the second apply uses a different class" + thrown(IllegalArgumentException) + + when: + instance = Foo.Create.With { + bars { + firstApply = bar(Bier, "Dieter") { + foo "fromFirst" + } + secondApply = bar(Bier, "Dieter") { + bar "fromSecond" + } + } + } + + then: "works, because the second apply uses the same class" + instance.bars.Dieter.foo == "fromFirst" + instance.bars.Dieter.bar == "fromSecond" + firstApply.is(secondApply) + instance.bars.Dieter.is(firstApply) + instance.bars.Dieter.getClass() == Bier + } + @SuppressWarnings("GroovyVariableNotAssigned") def "creation of inner objects in map should return the create object"() { given: @@ -1350,6 +1607,7 @@ import org.codehaus.groovy.control.CompilePhase then: instance.bars.klaus.url == "welt" + instance.bars.klaus.is(aBar) } def "equals, hashcode and toString methods are created"() { diff --git a/wiki/Basics.md b/wiki/Basics.md index 0aecb452..d130d30a 100644 --- a/wiki/Basics.md +++ b/wiki/Basics.md @@ -392,7 +392,7 @@ Foo.Create.With { } ``` -Usually, default implentation is only used on interfaces or abstract classes, but this is not enforced, since there +Usually, default implementation is only used on interfaces or abstract classes, but this is not enforced, since there might be some corner cases where it is useful. `defaultImpl` can also be used on collections, maps and virtual fields. diff --git a/wiki/Migration.md b/wiki/Migration.md index 82f03821..8228b9a8 100644 --- a/wiki/Migration.md +++ b/wiki/Migration.md @@ -3,14 +3,90 @@ Migration Guide # to 2.0 +## multiple inner create calls on the same field (or key in a map field) now stack instead of replacing + +Previously, multiple calls to the same inner create method would replace the previous value. + +```groovy +Foo.Create.With { + bar { + value = 1 + anotherValue = 2 + } + bar { + anotherValue = 3 + } +} +``` + +Previously, the second call of "bar" would result in a new object, i.e. the object created by the first call would be +replaced, resulting in: + +```groovy +foo.bar.anotherValue == 3 +foo.bar.value == null +``` + +Now those calls stack, so the result would be: + +```groovy +foo.bar.anotherValue == 3 +foo.bar.value == 1 +``` + +This is especially useful for using deep templates to set the first object: + +```groovy +def template = Foo.Create.Template { + bar { + value = 1 + anotherValue = 2 + } +} + +Foo.withTemplate(template) { + Foo.Create.With { + bar { + anotherValue = 3 + } + } +} +``` + +If the existing object does not match the new object (either because a different key is provided or specific type is given that is +different from the existing type), an Exception is thrown. In that case, the behaviour can be explicitly overridden by using +either apply or a setter (to explicitly merge or replace): + +```groovy +Foo.withTemplate(template) { + Foo.Create.With { + bar.apply { // explicitly force merge + anotherValue = 3 + } + } +} +``` + +or + +```groovy +Foo.withTemplate(template) { + Foo.Create.With { + bar = Bar.Create.With { // explicitly force overwrite + anotherValue = 3 + } + } +} +``` + ## Default Values are actually set, not only returned This makes objects used in copyFrom behave differently. Previously, the copyFrom methode explicitly ignored default values, -now the would be copied as well if already set. This can lead to different results if the copy source a) is not a template +now they would be copied as well if already set. This can lead to different results if the copy source a) is not a template object and b) was created outside the current phase run. If the object was created outside the phase run, it will most likely be a template, so using the `Create.Template()` -creator method (or the deprecate `createAsTemplate()` method) will lead to the same result as before. +creator method (or the deprecated `createAsTemplate()` method) will lead to the same result as before. ## Owners are now set in the owner phase diff --git a/wiki/Templates.md b/wiki/Templates.md index 54dd1a0f..06da072a 100644 --- a/wiki/Templates.md +++ b/wiki/Templates.md @@ -32,8 +32,6 @@ As with normal factory methods, templates can be created using the `Create.Templ closure, or by using the `Create.TemplateFrom` method, which take a file or URL which is parsed as a DelegatingScript, similar to to `Create.From` methods. -```groovy - There currently four options to apply templates, all examples use the following class and template: ```groovy @@ -204,7 +202,7 @@ Config.withTemplates((Environment) : [status: 'valid'], (Server) : [os: 'linux', } ``` -Note that Groovy requires the key object to be in parantheses if it is not a String. +Note that Groovy requires the key object to be in parentheses if it is not a String. # templates for abstract classes