From d8c0f0f3c8335216e60a94cbd37b7eb08b85b020 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 27 Feb 2024 13:00:14 -0600 Subject: [PATCH 1/8] Avoid using replicated default values for Lists when creating from builder or instances Signed-off-by: Tim Quinn --- .../codegen/GenerateAbstractBuilder.java | 13 +++- .../codegen/TypeHandlerCollection.java | 15 ++++ .../DualValuedDefaultValuesBlueprint.java | 32 ++++++++ .../SingleValuedDefaultValuesBlueprint.java | 31 ++++++++ .../builder/test/ListDefaultValuesTest.java | 76 +++++++++++++++++++ 5 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/DualValuedDefaultValuesBlueprint.java create mode 100644 builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java create mode 100644 builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index e220f843ad0..987b2f0abbf 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -354,8 +354,11 @@ private static void fromInstanceMethod(InnerClass.Builder builder, TypeContext t for (PrototypeProperty property : typeContext.propertyData().properties()) { TypeName declaredType = property.typeHandler().declaredType(); if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - methodBuilder.addContent("add"); - methodBuilder.addContent(capitalize(property.name())); + // add for Set and Map take care of enforcing distinct values themselves, but do so ourselves for List when + // adding values from an existing builder which will already have the default(s). + methodBuilder.addContent("add" + + (declaredType.isList() ? "Distinct" : "") + + capitalize(property.name())); methodBuilder.addContent("(prototype."); methodBuilder.addContent(property.typeHandler().getterName()); methodBuilder.addContentLine("());"); @@ -412,7 +415,11 @@ private static void fromBuilderMethod(InnerClass.Builder classBuilder, methodBuilder.addContentLine("builder." + getterName + "().ifPresent(this::" + setterName + ");"); } else { if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - methodBuilder.addContent("add" + capitalize(property.name())); + // add for Set and Map take care of enforcing distinct values themselves, but do so ourselves for List when + // adding values from an existing instance which will already have the default(s). + methodBuilder.addContent("add" + + (declaredType.isList() ? "Distinct" : "") + + capitalize(property.name())); } else { methodBuilder.addContent(setterName); } diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java index 957f3dc7b29..b1d9fb3de2c 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java @@ -48,6 +48,7 @@ import io.helidon.codegen.classmodel.InnerClass; import io.helidon.codegen.classmodel.Javadoc; import io.helidon.codegen.classmodel.Method; +import io.helidon.common.types.AccessModifier; import io.helidon.common.types.TypeName; import io.helidon.common.types.TypeNames; @@ -449,5 +450,19 @@ private void declaredSetters(InnerClass.Builder classBuilder, .addContentLine("this." + name() + ".addAll(" + name() + ");") .addContentLine("return self();"); classBuilder.addMethod(builder); + + // Primarily for use in preventing default values from being included in a list twice using Builder.from. + builder.name("addDistinct" + capitalize(name())) + .accessModifier(AccessModifier.PROTECTED) + .clearContent() + .addContentLine("Objects.requireNonNull(" + name() + ");") + .addContentLine(name() + ".forEach(newItem -> {") + .addContentLine(" if (!this." + name() + ".contains(newItem)) {") + .addContentLine(" this." + name() + ".add(newItem);") + .addContentLine("}") + .addContentLine("}") + .addContentLine(");") + .addContentLine("return self();"); + classBuilder.addMethod(builder); } } diff --git a/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/DualValuedDefaultValuesBlueprint.java b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/DualValuedDefaultValuesBlueprint.java new file mode 100644 index 00000000000..3c59ae17e6f --- /dev/null +++ b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/DualValuedDefaultValuesBlueprint.java @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.helidon.builder.test.testsubjects; + +import java.util.List; + +import io.helidon.builder.api.Option; +import io.helidon.builder.api.Prototype; + +@Prototype.Blueprint +interface DualValuedDefaultValuesBlueprint { + + String DEFAULT_1 = "default1"; + String DEFAULT_2 = "default2"; + + @Option.Default({DEFAULT_1, DEFAULT_2}) + List strings(); + +} diff --git a/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java new file mode 100644 index 00000000000..f310621ee90 --- /dev/null +++ b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.helidon.builder.test.testsubjects; + +import java.util.List; + +import io.helidon.builder.api.Option; +import io.helidon.builder.api.Prototype; + +@Prototype.Blueprint +interface SingleValuedDefaultValuesBlueprint { + + String DEFAULT_STRING = "defaultValue"; + + @Option.Default(DEFAULT_STRING) + List strings(); + +} diff --git a/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java b/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java new file mode 100644 index 00000000000..fdaeb40b78a --- /dev/null +++ b/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.helidon.builder.test; + +import io.helidon.builder.test.testsubjects.DualValuedDefaultValues; +import io.helidon.builder.test.testsubjects.SingleValuedDefaultValues; + +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasItems; + +class ListDefaultValuesTest { + + @Test + void checkSingleDefaultFromInstance() { + SingleValuedDefaultValues original = SingleValuedDefaultValues.create(); + assertThat("Original value", + original.strings(), + hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + SingleValuedDefaultValues copy = SingleValuedDefaultValues.builder().from(original).build(); + assertThat("Copied value", + copy.strings(), + hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + + } + + @Test + void checkSingleDefaultFromBuilder() { + SingleValuedDefaultValues.Builder builder = SingleValuedDefaultValues.builder(); + + SingleValuedDefaultValues copy = SingleValuedDefaultValues.builder().from(builder).build(); + + assertThat("Copied value", + copy.strings(), + hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + + } + + @Test + void checkDualDefaultFromInstance() { + DualValuedDefaultValues original = DualValuedDefaultValues.create(); + assertThat("Original values", + original.strings(), + hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + + DualValuedDefaultValues copy = DualValuedDefaultValues.builder().from(original).build(); + assertThat("Copied values", + original.strings(), + hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + } + + @Test + void checkDualDefaultFromBuilder() { + DualValuedDefaultValues.Builder builder = DualValuedDefaultValues.builder(); + + DualValuedDefaultValues copy = DualValuedDefaultValues.builder().from(builder).build(); + assertThat("Copied values", + copy.strings(), + hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + } +} From d9a0349cc6293820ec3e00bad168b53c5e8944fc Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 27 Feb 2024 14:09:54 -0600 Subject: [PATCH 2/8] Move addDistinct generation to TypeHandlerList; revise processing of list a bit --- .../codegen/GenerateAbstractBuilder.java | 6 +-- .../codegen/TypeHandlerCollection.java | 15 ------- .../builder/codegen/TypeHandlerList.java | 42 +++++++++++++++++++ 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index 987b2f0abbf..5512114a98d 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -354,7 +354,7 @@ private static void fromInstanceMethod(InnerClass.Builder builder, TypeContext t for (PrototypeProperty property : typeContext.propertyData().properties()) { TypeName declaredType = property.typeHandler().declaredType(); if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - // add for Set and Map take care of enforcing distinct values themselves, but do so ourselves for List when + // Sets and maps take care of enforcing distinct values themselves, but do so ourselves for lists when // adding values from an existing builder which will already have the default(s). methodBuilder.addContent("add" + (declaredType.isList() ? "Distinct" : "") @@ -415,8 +415,8 @@ private static void fromBuilderMethod(InnerClass.Builder classBuilder, methodBuilder.addContentLine("builder." + getterName + "().ifPresent(this::" + setterName + ");"); } else { if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - // add for Set and Map take care of enforcing distinct values themselves, but do so ourselves for List when - // adding values from an existing instance which will already have the default(s). + // Sets and maps take care of enforcing distinct values themselves, but do so ourselves for lists when + // adding values from an existing builder which will already have the default(s). methodBuilder.addContent("add" + (declaredType.isList() ? "Distinct" : "") + capitalize(property.name())); diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java index b1d9fb3de2c..957f3dc7b29 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java @@ -48,7 +48,6 @@ import io.helidon.codegen.classmodel.InnerClass; import io.helidon.codegen.classmodel.Javadoc; import io.helidon.codegen.classmodel.Method; -import io.helidon.common.types.AccessModifier; import io.helidon.common.types.TypeName; import io.helidon.common.types.TypeNames; @@ -450,19 +449,5 @@ private void declaredSetters(InnerClass.Builder classBuilder, .addContentLine("this." + name() + ".addAll(" + name() + ");") .addContentLine("return self();"); classBuilder.addMethod(builder); - - // Primarily for use in preventing default values from being included in a list twice using Builder.from. - builder.name("addDistinct" + capitalize(name())) - .accessModifier(AccessModifier.PROTECTED) - .clearContent() - .addContentLine("Objects.requireNonNull(" + name() + ");") - .addContentLine(name() + ".forEach(newItem -> {") - .addContentLine(" if (!this." + name() + ".contains(newItem)) {") - .addContentLine(" this." + name() + ".add(newItem);") - .addContentLine("}") - .addContentLine("}") - .addContentLine(");") - .addContentLine("return self();"); - classBuilder.addMethod(builder); } } diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java index 90277f33b9b..c0ae497feb6 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java @@ -17,9 +17,15 @@ package io.helidon.builder.codegen; import java.util.Optional; +import java.util.function.Predicate; +import io.helidon.codegen.classmodel.InnerClass; +import io.helidon.codegen.classmodel.Javadoc; +import io.helidon.codegen.classmodel.Method; +import io.helidon.common.types.AccessModifier; import io.helidon.common.types.TypeName; +import static io.helidon.codegen.CodegenUtil.capitalize; import static io.helidon.common.types.TypeNames.LIST; class TypeHandlerList extends TypeHandlerCollection { @@ -27,4 +33,40 @@ class TypeHandlerList extends TypeHandlerCollection { TypeHandlerList(String name, String getterName, String setterName, TypeName declaredType) { super(name, getterName, setterName, declaredType, LIST, "toList()", Optional.empty()); } + + @Override + void setters(InnerClass.Builder classBuilder, + AnnotationDataOption configured, + FactoryMethods factoryMethods, + TypeName returnType, + Javadoc blueprintJavadoc) { + super.setters(classBuilder, configured, factoryMethods, returnType, blueprintJavadoc); + declaredDistinctSetter(classBuilder, returnType, blueprintJavadoc); + } + + private void declaredDistinctSetter(InnerClass.Builder classBuilder, TypeName returnType, Javadoc blueprintJavadoc) { + + // Primarily for use in preventing default values from being included in a list twice using Builder.from. + classBuilder.addImport(Predicate.class); + Method.Builder builder = Method.builder() + .name("addDistinct" + capitalize(name())) + .accessModifier(AccessModifier.PROTECTED) + .returnType(returnType, "updated builder instance") + .description("Adds distinct values") + .addJavadocTag("see", "#" + getterName() + "()") + .addParameter(param -> param.name(name()) + .type(argumentTypeName()) + .description(blueprintJavadoc.returnDescription())) + + .addContentLine("Objects.requireNonNull(" + name() + ");") + .addContentLine(name() + ".stream()") + .increaseContentPadding() + .increaseContentPadding() + .addContentLine(".filter(Predicate.not(this." + name() + "::contains))") + .addContentLine(".forEach(this." + name() + "::add);") + .decreaseContentPadding() + .decreaseContentPadding() + .addContentLine("return self();"); + classBuilder.addMethod(builder); + } } From fd51901b0914747dc8701a4a5145d29d0122d4fe Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Sun, 24 Mar 2024 13:22:55 -0500 Subject: [PATCH 3/8] Use isMutated flags for lists to know how to handle them in 'from' methods --- .../codegen/GenerateAbstractBuilder.java | 32 +++++--- .../codegen/TypeHandlerCollection.java | 11 +++ .../builder/codegen/TypeHandlerList.java | 44 +++-------- .../helidon/builder/test/CustomNamedTest.java | 6 +- .../builder/test/ListDefaultValuesTest.java | 79 +++++++++++++++++-- 5 files changed, 120 insertions(+), 52 deletions(-) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index 5512114a98d..7ce2b94d23a 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -354,11 +354,15 @@ private static void fromInstanceMethod(InnerClass.Builder builder, TypeContext t for (PrototypeProperty property : typeContext.propertyData().properties()) { TypeName declaredType = property.typeHandler().declaredType(); if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - // Sets and maps take care of enforcing distinct values themselves, but do so ourselves for lists when - // adding values from an existing builder which will already have the default(s). - methodBuilder.addContent("add" - + (declaredType.isList() ? "Distinct" : "") - + capitalize(property.name())); + if (declaredType.isList()) { + // A list might contain only default values. If it has not been updated, clear it of default values + // before adding the values from the other builder. + methodBuilder.addContentLine("if (!is" + capitalize(property.name()) + "Mutated) {") + .addContentLine(property.name() + ".clear();") + .addContentLine("}"); + } + methodBuilder.addContent("add"); + methodBuilder.addContent(capitalize(property.name())); methodBuilder.addContent("(prototype."); methodBuilder.addContent(property.typeHandler().getterName()); methodBuilder.addContentLine("());"); @@ -415,11 +419,14 @@ private static void fromBuilderMethod(InnerClass.Builder classBuilder, methodBuilder.addContentLine("builder." + getterName + "().ifPresent(this::" + setterName + ");"); } else { if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { - // Sets and maps take care of enforcing distinct values themselves, but do so ourselves for lists when - // adding values from an existing builder which will already have the default(s). - methodBuilder.addContent("add" - + (declaredType.isList() ? "Distinct" : "") - + capitalize(property.name())); + if (declaredType.isList()) { + // A list might contain only default values. If it has not been updated, clear it of default values + // before adding the values from the other builder. + methodBuilder.addContentLine("if (!is" + capitalize(property.name()) + "Mutated) {") + .addContentLine(property.name() + ".clear();") + .addContentLine("}"); + } + methodBuilder.addContent("add" + capitalize(property.name())); } else { methodBuilder.addContent(setterName); } @@ -460,6 +467,11 @@ private static void fields(InnerClass.Builder classBuilder, TypeContext typeCont .name(child.name() + "DiscoverServices") .defaultValue(String.valueOf(child.configuredOption().providerDiscoverServices()))); } + if (child.typeHandler().declaredType().isList()) { + classBuilder.addField(builder -> builder.type(boolean.class) + .name("is" + capitalize(child.name()) + "Mutated") + .accessModifier(AccessModifier.PRIVATE)); + } } } diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java index 957f3dc7b29..ffd2b1d516f 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java @@ -438,6 +438,7 @@ private void declaredSetters(InnerClass.Builder classBuilder, .accessModifier(setterAccessModifier(configured)) .addContent(Objects.class) .addContentLine(".requireNonNull(" + name() + ");") + .update(this::extraSetterContent) .addContentLine("this." + name() + ".clear();") .addContentLine("this." + name() + ".addAll(" + name() + ");") .addContentLine("return self();"); @@ -446,8 +447,18 @@ private void declaredSetters(InnerClass.Builder classBuilder, builder.name("add" + capitalize(name())) .clearContent() .addContentLine("Objects.requireNonNull(" + name() + ");") //Overwrites existing content + .update(this::extraAdderContent) .addContentLine("this." + name() + ".addAll(" + name() + ");") .addContentLine("return self();"); classBuilder.addMethod(builder); } + + Method.Builder extraSetterContent(Method.Builder builder) { + return builder; + } + + Method.Builder extraAdderContent(Method.Builder builder) { + return builder; + } + } diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java index c0ae497feb6..a02bea4b874 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java @@ -17,15 +17,11 @@ package io.helidon.builder.codegen; import java.util.Optional; -import java.util.function.Predicate; -import io.helidon.codegen.classmodel.InnerClass; -import io.helidon.codegen.classmodel.Javadoc; +import io.helidon.codegen.CodegenUtil; import io.helidon.codegen.classmodel.Method; -import io.helidon.common.types.AccessModifier; import io.helidon.common.types.TypeName; -import static io.helidon.codegen.CodegenUtil.capitalize; import static io.helidon.common.types.TypeNames.LIST; class TypeHandlerList extends TypeHandlerCollection { @@ -35,38 +31,16 @@ class TypeHandlerList extends TypeHandlerCollection { } @Override - void setters(InnerClass.Builder classBuilder, - AnnotationDataOption configured, - FactoryMethods factoryMethods, - TypeName returnType, - Javadoc blueprintJavadoc) { - super.setters(classBuilder, configured, factoryMethods, returnType, blueprintJavadoc); - declaredDistinctSetter(classBuilder, returnType, blueprintJavadoc); + Method.Builder extraAdderContent(Method.Builder builder) { + return builder.addContentLine(isMutatedField() + " = true;"); } - private void declaredDistinctSetter(InnerClass.Builder classBuilder, TypeName returnType, Javadoc blueprintJavadoc) { - - // Primarily for use in preventing default values from being included in a list twice using Builder.from. - classBuilder.addImport(Predicate.class); - Method.Builder builder = Method.builder() - .name("addDistinct" + capitalize(name())) - .accessModifier(AccessModifier.PROTECTED) - .returnType(returnType, "updated builder instance") - .description("Adds distinct values") - .addJavadocTag("see", "#" + getterName() + "()") - .addParameter(param -> param.name(name()) - .type(argumentTypeName()) - .description(blueprintJavadoc.returnDescription())) + @Override + Method.Builder extraSetterContent(Method.Builder builder) { + return builder.addContentLine(isMutatedField() + " = true;"); + } - .addContentLine("Objects.requireNonNull(" + name() + ");") - .addContentLine(name() + ".stream()") - .increaseContentPadding() - .increaseContentPadding() - .addContentLine(".filter(Predicate.not(this." + name() + "::contains))") - .addContentLine(".forEach(this." + name() + "::add);") - .decreaseContentPadding() - .decreaseContentPadding() - .addContentLine("return self();"); - classBuilder.addMethod(builder); + private String isMutatedField() { + return "is" + CodegenUtil.capitalize(name()) + "Mutated"; } } diff --git a/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java b/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java index 217023a93d6..8aa33a0b71b 100644 --- a/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java +++ b/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java @@ -56,7 +56,11 @@ void testIt() throws Exception { .build(); DefaultPrettyPrinter printer = new DefaultPrettyPrinter(); String json = mapper.writer(printer).writeValueAsString(customNamed); - assertThat(json, equalTo("{\n" + " \"stringSet\" : [ \"b\", \"a\", \"y\" ]\n" + "}")); + assertThat(json, equalTo(""" + { + "isStringListMutated" : false, + "stringSet" : [ "b", "a", "y" ] + }""")); } } diff --git a/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java b/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java index fdaeb40b78a..3ae6339be36 100644 --- a/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java +++ b/builder/tests/builder/src/test/java/io/helidon/builder/test/ListDefaultValuesTest.java @@ -15,14 +15,18 @@ */ package io.helidon.builder.test; +import java.util.List; + import io.helidon.builder.test.testsubjects.DualValuedDefaultValues; import io.helidon.builder.test.testsubjects.SingleValuedDefaultValues; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.iterableWithSize; class ListDefaultValuesTest { @@ -31,11 +35,13 @@ void checkSingleDefaultFromInstance() { SingleValuedDefaultValues original = SingleValuedDefaultValues.create(); assertThat("Original value", original.strings(), - hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + allOf(hasItem(SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(1))); SingleValuedDefaultValues copy = SingleValuedDefaultValues.builder().from(original).build(); assertThat("Copied value", copy.strings(), - hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + allOf(hasItem(SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(1))); } @@ -47,7 +53,8 @@ void checkSingleDefaultFromBuilder() { assertThat("Copied value", copy.strings(), - hasItem(SingleValuedDefaultValues.DEFAULT_STRING)); + allOf(hasItem(SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(1))); } @@ -56,12 +63,14 @@ void checkDualDefaultFromInstance() { DualValuedDefaultValues original = DualValuedDefaultValues.create(); assertThat("Original values", original.strings(), - hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + allOf(hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2), + iterableWithSize(2))); DualValuedDefaultValues copy = DualValuedDefaultValues.builder().from(original).build(); assertThat("Copied values", original.strings(), - hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + allOf(hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2), + iterableWithSize(2))); } @Test @@ -71,6 +80,64 @@ void checkDualDefaultFromBuilder() { DualValuedDefaultValues copy = DualValuedDefaultValues.builder().from(builder).build(); assertThat("Copied values", copy.strings(), - hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2)); + allOf(hasItems(DualValuedDefaultValues.DEFAULT_1, DualValuedDefaultValues.DEFAULT_2), + iterableWithSize(2))); + } + + @Test + void checkSingleDefaultDoubledInBuilder() { + SingleValuedDefaultValues original = SingleValuedDefaultValues.builder() + .strings(List.of(SingleValuedDefaultValues.DEFAULT_STRING, SingleValuedDefaultValues.DEFAULT_STRING)) + .build(); + + SingleValuedDefaultValues copy = SingleValuedDefaultValues.builder() + .from(original) + .build(); + assertThat("Copied values", + copy.strings(), + allOf(hasItems(SingleValuedDefaultValues.DEFAULT_STRING, SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(2))); + } + + @Test + void checkSingleDefaultWithUpdate() { + String value = "non-default"; + SingleValuedDefaultValues original = SingleValuedDefaultValues.builder() + .addStrings(List.of(value)) + .build(); + + assertThat("Original with update", + original.strings(), + allOf(hasItems(value, SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(2))); + } + + @Test + void checkDualDefaultWithUpdate() { + String value = "non-default"; + DualValuedDefaultValues original = DualValuedDefaultValues.builder() + .addStrings(List.of(value)) + .build(); + assertThat("Original with update", original.strings(), allOf(hasItems(value, + DualValuedDefaultValues.DEFAULT_1, + DualValuedDefaultValues.DEFAULT_2), + iterableWithSize(3))); + } + + @Test + void checkSingleDefaultWithoutUpdate() { + SingleValuedDefaultValues original = SingleValuedDefaultValues.create(); + + assertThat("Original with no update", original.strings(), allOf(hasItems(SingleValuedDefaultValues.DEFAULT_STRING), + iterableWithSize(1))); + } + + @Test + void checkDualDefaultWithoutUpdate() { + DualValuedDefaultValues original = DualValuedDefaultValues.create(); + + assertThat("Original with no updates", original.strings(), allOf(hasItems(DualValuedDefaultValues.DEFAULT_1, + DualValuedDefaultValues.DEFAULT_2), + iterableWithSize(2))); } } From 1e05e2756b2ec00f478e91f43b363e5c18eff58b Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Sun, 24 Mar 2024 16:04:30 -0500 Subject: [PATCH 4/8] Generate isXxxMutated only for builder, not for impl also --- .../io/helidon/builder/codegen/GenerateAbstractBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index 7ce2b94d23a..ea8b617c4e2 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -467,7 +467,7 @@ private static void fields(InnerClass.Builder classBuilder, TypeContext typeCont .name(child.name() + "DiscoverServices") .defaultValue(String.valueOf(child.configuredOption().providerDiscoverServices()))); } - if (child.typeHandler().declaredType().isList()) { + if (isBuilder && child.typeHandler().declaredType().isList()) { classBuilder.addField(builder -> builder.type(boolean.class) .name("is" + capitalize(child.name()) + "Mutated") .accessModifier(AccessModifier.PRIVATE)); From 58e0894b77f26e6757c37c3f6e45da6622c52d0b Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Sun, 24 Mar 2024 16:34:36 -0500 Subject: [PATCH 5/8] Undo unneeded test change (now that impls do not get the new mutated flag) --- .../test/java/io/helidon/builder/test/CustomNamedTest.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java b/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java index 8aa33a0b71b..217023a93d6 100644 --- a/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java +++ b/builder/tests/builder/src/test/java/io/helidon/builder/test/CustomNamedTest.java @@ -56,11 +56,7 @@ void testIt() throws Exception { .build(); DefaultPrettyPrinter printer = new DefaultPrettyPrinter(); String json = mapper.writer(printer).writeValueAsString(customNamed); - assertThat(json, equalTo(""" - { - "isStringListMutated" : false, - "stringSet" : [ "b", "a", "y" ] - }""")); + assertThat(json, equalTo("{\n" + " \"stringSet\" : [ \"b\", \"a\", \"y\" ]\n" + "}")); } } From b3869531f04ea8eaca4d89840b5eb6b1629c7f80 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Mon, 25 Mar 2024 11:31:30 -0500 Subject: [PATCH 6/8] from(builder) should not add other builder default values if its own values have been mutated --- .../codegen/GenerateAbstractBuilder.java | 36 +++++++++++++++---- .../builder/codegen/TypeHandlerList.java | 6 +++- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index ea8b617c4e2..e64d44b542d 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -420,17 +420,41 @@ private static void fromBuilderMethod(InnerClass.Builder classBuilder, } else { if (declaredType.isSet() || declaredType.isList() || declaredType.isMap()) { if (declaredType.isList()) { - // A list might contain only default values. If it has not been updated, clear it of default values - // before adding the values from the other builder. - methodBuilder.addContentLine("if (!is" + capitalize(property.name()) + "Mutated) {") + /* + If this builder's list HAS been mutated, add the other builder's values ONLY if they are not defaults. + + If this builder's list HAS NOT been mutated, clear it (to remove defaults) and add the other builder's + values regardless of whether they are defaulted or explicitly set. + + Generated code: + + if (isXxxMutated) { + if (builder.isXxxMutated) { + xxx.addAll(builder.xxx()); + } + } else { + xxx.clear(); + xxx.addAll(builder.xxx()); + } + */ + String isMutatedProperty = TypeHandlerList.isMutatedField(property.name()); + methodBuilder.addContentLine("if (" + isMutatedProperty + ") {") + .addContentLine("if (builder." + isMutatedProperty + ") {") + .addContentLine("add" + capitalize(property.name()) + "(builder." + property.name() + ");") + .addContentLine("} else {") .addContentLine(property.name() + ".clear();") + .addContentLine("add" + capitalize(property.name()) + "(builder." + property.name() + ");") + .addContentLine("}") .addContentLine("}"); + + } else { + // Non-list collection case. + methodBuilder.addContentLine("add" + capitalize(property.name()) + "(builder." + property.name() + ");"); } - methodBuilder.addContent("add" + capitalize(property.name())); } else { - methodBuilder.addContent(setterName); + // Non-collection case. + methodBuilder.addContentLine(setterName + "(builder." + getterName + "());"); } - methodBuilder.addContentLine("(builder." + getterName + "());"); } } diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java index a02bea4b874..7a551950c99 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerList.java @@ -30,6 +30,10 @@ class TypeHandlerList extends TypeHandlerCollection { super(name, getterName, setterName, declaredType, LIST, "toList()", Optional.empty()); } + static String isMutatedField(String propertyName) { + return "is" + CodegenUtil.capitalize(propertyName) + "Mutated"; + } + @Override Method.Builder extraAdderContent(Method.Builder builder) { return builder.addContentLine(isMutatedField() + " = true;"); @@ -41,6 +45,6 @@ Method.Builder extraSetterContent(Method.Builder builder) { } private String isMutatedField() { - return "is" + CodegenUtil.capitalize(name()) + "Mutated"; + return isMutatedField(name()); } } From 2b44e8008b5cfe1e54af40f2ba48aee8cc559ccd Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 26 Mar 2024 10:44:27 -0500 Subject: [PATCH 7/8] Set isMutated in single-update method also --- .../java/io/helidon/builder/codegen/TypeHandlerCollection.java | 1 + .../test/testsubjects/SingleValuedDefaultValuesBlueprint.java | 1 + 2 files changed, 2 insertions(+) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java index ffd2b1d516f..5d101949f5a 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/TypeHandlerCollection.java @@ -388,6 +388,7 @@ private void singularSetter(InnerClass.Builder classBuilder, .addContent(Objects.class) .addContentLine(".requireNonNull(" + singularName + ");") .addContentLine("this." + name() + ".add(" + singularName + ");") + .update(this::extraAdderContent) .addContentLine("return self();"); classBuilder.addMethod(builder); } diff --git a/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java index f310621ee90..515214721e6 100644 --- a/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java +++ b/builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/SingleValuedDefaultValuesBlueprint.java @@ -26,6 +26,7 @@ interface SingleValuedDefaultValuesBlueprint { String DEFAULT_STRING = "defaultValue"; @Option.Default(DEFAULT_STRING) + @Option.Singular List strings(); } From 7846d92e329ff2be96461308af8e46f7d0470a99 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 26 Mar 2024 13:04:49 -0500 Subject: [PATCH 8/8] Fix incorrect if nesting; fix formatting --- .../helidon/builder/codegen/GenerateAbstractBuilder.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java index e64d44b542d..314fcdc8b36 100644 --- a/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java +++ b/builder/codegen/src/main/java/io/helidon/builder/codegen/GenerateAbstractBuilder.java @@ -430,21 +430,22 @@ private static void fromBuilderMethod(InnerClass.Builder classBuilder, if (isXxxMutated) { if (builder.isXxxMutated) { - xxx.addAll(builder.xxx()); + addXxx(builder.xxx()); } } else { xxx.clear(); - xxx.addAll(builder.xxx()); + addXxx(builder.xxx()); } */ String isMutatedProperty = TypeHandlerList.isMutatedField(property.name()); methodBuilder.addContentLine("if (" + isMutatedProperty + ") {") .addContentLine("if (builder." + isMutatedProperty + ") {") .addContentLine("add" + capitalize(property.name()) + "(builder." + property.name() + ");") + .addContentLine("}") + .decreaseContentPadding() // Ideally would not be needed but makes for nicer generated code. .addContentLine("} else {") .addContentLine(property.name() + ".clear();") .addContentLine("add" + capitalize(property.name()) + "(builder." + property.name() + ");") - .addContentLine("}") .addContentLine("}"); } else {