From 8a7528c9efa085f65344b2adbab436ca81821994 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Sun, 24 Mar 2024 13:22:55 -0500 Subject: [PATCH] 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))); } }