Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.x: Allowed values are now checked when validating a builder. #7400

Merged
merged 4 commits into from
Aug 21, 2023
Merged

4.x: Allowed values are now checked when validating a builder. #7400

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5aee27c
Allowed values are now checked when validating a builder.
tomas-langer Aug 17, 2023
6f4b116
Spotbugs fix - impl type same name as interface
tomas-langer Aug 17, 2023
81bd342
Revert expected back.
tomas-langer Aug 17, 2023
fa5fdf3
Fix review comments.
tomas-langer Aug 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions builder/processor/src/main/java/io/helidon/builder/processor/CustomMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,7 @@ private static List<CustomMethod> findMethods(TypeContext.TypeInformation typeIn

// annotations to be added to generated code
List<String> annotations = it.findAnnotation(Types.PROTOTYPE_ANNOTATED_TYPE)
.flatMap(Annotation::value)
.map(annotation -> annotation.split(","))
.map(List::of)
.flatMap(Annotation::stringValues)
.orElseGet(List::of)
.stream()
.map(String::trim) // to remove spaces after commas when used
Expand Down
134 changes: 92 additions & 42 deletions builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;

Expand All @@ -43,6 +45,7 @@
import static io.helidon.builder.processor.Types.PROTOTYPE_CONFIGURED_BUILDER;
import static io.helidon.builder.processor.Types.STRING_TYPE;
import static io.helidon.common.processor.GeneratorTools.capitalize;
import static io.helidon.common.processor.classmodel.ClassModel.TYPE_TOKEN;
import static io.helidon.common.types.TypeNames.LIST;
import static io.helidon.common.types.TypeNames.MAP;
import static io.helidon.common.types.TypeNames.OPTIONAL;
Expand Down Expand Up @@ -128,44 +131,6 @@ static void generate(ClassModel.Builder classModel,
});
}

private static void addCustomBuilderMethods(TypeContext typeContext, InnerClass.Builder builder) {
for (CustomMethods.CustomMethod customMethod : typeContext.customMethods().builderMethods()) {
// builder specific custom methods (not part of interface)
CustomMethods.Method generated = customMethod.generatedMethod().method();
// public Builder type(Type) with implementation
Method.Builder method = Method.builder()
.name(generated.name())
.returnType(TypeName.createFromGenericDeclaration("BUILDER"))
.addLine(customMethod.generatedMethod().callCode() + ";");
for (String annotation : customMethod.generatedMethod().annotations()) {
method.addAnnotation(Annotation.parse(annotation));
}
for (CustomMethods.Argument argument : generated.arguments()) {
method.addParameter(param -> param.name(argument.name())
.type(argument.typeName()));
}
if (!generated.javadoc().isEmpty()) {
Javadoc javadoc = Javadoc.builder()
.from(Javadoc.parse(generated.javadoc()))
.returnDescription("updated builder instance")
.build();
method.javadoc(javadoc);
}
builder.addMethod(method);
}
}

private static void createConstructor(Constructor.Builder constructor, TypeContext typeContext) {
constructor.description("Protected to support extensibility.")
.accessModifier(AccessModifier.PROTECTED);
// overriding defaults
for (var prop : typeContext.propertyData().overridingProperties()) {
if (prop.configuredOption().hasDefault()) {
constructor.addLine(prop.setterName() + "(" + prop.configuredOption().defaultValue() + ");");
}
}
}

static void buildRuntimeObjectMethod(InnerClass.Builder classBuilder, TypeContext typeContext, boolean isBuilder) {
TypeContext.TypeInformation typeInformation = typeContext.typeInfo();
boolean hasRuntimeObject = typeInformation.runtimeObject().isPresent();
Expand Down Expand Up @@ -207,6 +172,44 @@ static boolean hasConfig(List<PrototypeProperty> properties) {
.anyMatch(it -> CONFIG_TYPE.equals(it.typeHandler().actualType()));
}

private static void addCustomBuilderMethods(TypeContext typeContext, InnerClass.Builder builder) {
for (CustomMethods.CustomMethod customMethod : typeContext.customMethods().builderMethods()) {
// builder specific custom methods (not part of interface)
CustomMethods.Method generated = customMethod.generatedMethod().method();
// public Builder type(Type) with implementation
Method.Builder method = Method.builder()
.name(generated.name())
.returnType(TypeName.createFromGenericDeclaration("BUILDER"))
.addLine(customMethod.generatedMethod().callCode() + ";");
for (String annotation : customMethod.generatedMethod().annotations()) {
method.addAnnotation(Annotation.parse(annotation));
}
for (CustomMethods.Argument argument : generated.arguments()) {
method.addParameter(param -> param.name(argument.name())
.type(argument.typeName()));
}
if (!generated.javadoc().isEmpty()) {
Javadoc javadoc = Javadoc.builder()
.from(Javadoc.parse(generated.javadoc()))
.returnDescription("updated builder instance")
.build();
method.javadoc(javadoc);
}
builder.addMethod(method);
}
}

private static void createConstructor(Constructor.Builder constructor, TypeContext typeContext) {
constructor.description("Protected to support extensibility.")
.accessModifier(AccessModifier.PROTECTED);
// overriding defaults
for (var prop : typeContext.propertyData().overridingProperties()) {
if (prop.configuredOption().hasDefault()) {
constructor.addLine(prop.setterName() + "(" + prop.configuredOption().defaultValue() + ");");
}
}
}

private static void builderMethods(InnerClass.Builder classBuilder, TypeContext typeContext) {
List<PrototypeProperty> properties = typeContext.propertyData().properties();
TypeContext.ConfiguredData configured = typeContext.configuredData();
Expand Down Expand Up @@ -415,6 +418,20 @@ private static void fields(InnerClass.Builder classBuilder, TypeContext typeCont
classBuilder.addField(builder -> builder.type(CONFIG_TYPE).name("config"));
}
for (PrototypeProperty child : typeContext.propertyData().properties()) {
if (isBuilder && child.configuredOption().hasAllowedValues()) {
String allowedValues = child.configuredOption().allowedValues()
.stream()
.map(PrototypeProperty.AllowedValue::value)
.map(it -> "\"" + it + "\"")
.collect(Collectors.joining(", "));
// private static final Set<String> PROPERTY_ALLOWED_VALUES = Set.of("a", "b", "c");
classBuilder.addField(it -> it.isFinal(true)
.isStatic(true)
.name(child.name().toUpperCase(Locale.ROOT) + "_ALLOWED_VALUES")
.type(TypeName.builder(SET).addTypeArgument(STRING_TYPE).build())
.defaultValue(TYPE_TOKEN + Set.class.getName() + TYPE_TOKEN + ".of(" + allowedValues + ")")
);
}
if (!isBuilder || !child.typeHandler().actualType().equals(CONFIG_TYPE)) {
classBuilder.addField(child.fieldDeclaration(isBuilder));
}
Expand Down Expand Up @@ -516,7 +533,9 @@ private static void validatePrototypeMethod(InnerClass.Builder classBuilder, Typ
.superPrototype()
.ifPresent(it -> validateBuilder.addLine("super.validatePrototype();"));

if (typeContext.propertyData().hasRequired() || typeContext.propertyData().hasNonNulls()) {
if (typeContext.propertyData().hasRequired()
tomas-langer marked this conversation as resolved.
Show resolved Hide resolved
|| typeContext.propertyData().hasNonNulls()
|| typeContext.propertyData().hasAllowedValues()) {
requiredValidation(validateBuilder, typeContext);
}
classBuilder.addMethod(validateBuilder);
Expand All @@ -529,11 +548,13 @@ private static void requiredValidation(Method.Builder validateBuilder, TypeConte
.addLine(".collector();");

for (PrototypeProperty property : typeContext.propertyData().properties()) {
String configKey = property.configuredOption().configKey();
String propertyName = property.name();

if (property.configuredOption().validateNotNull() && !property.configuredOption().hasDefault()) {
String configKey = property.configuredOption().configKey();
validateBuilder.addLine("if (" + property.typeHandler().name() + " == null) {")
validateBuilder.addLine("if (" + propertyName + " == null) {")
.add("collector.fatal(getClass(), \"Property \\\"")
.add(configKey == null ? property.typeHandler().name() : configKey);
.add(configKey == null ? propertyName : configKey);

if (property.configuredOption().required()) {
validateBuilder.addLine("\\\" is required, but not set\");");
Expand All @@ -542,6 +563,35 @@ private static void requiredValidation(Method.Builder validateBuilder, TypeConte
}
validateBuilder.addLine("}");
}
if (property.configuredOption().hasAllowedValues()) {
String allowedValuesConstant = propertyName.toUpperCase(Locale.ROOT) + "_ALLOWED_VALUES";
TypeName declaredType = property.typeHandler().declaredType();

if (declaredType.isList() || declaredType.isSet()) {
String single = "single" + capitalize(propertyName);
validateBuilder.addLine("for (var " + single + " : " + propertyName + ") {");
validateBuilder.addLine("if (!" + allowedValuesConstant + ".contains(String.valueOf(" + single + "))) {")
.add("collector.fatal(getClass(), \"Property \\\"")
.add(configKey == null ? propertyName : configKey)
.add("\\\" contains value that is not within allowed values. Configured: \\\"\" + "
+ single + " + \"\\\"")
.addLine(", expected one of: \\\"\" + " + allowedValuesConstant + " + \"\\\"\");");
validateBuilder.addLine("}");
validateBuilder.addLine("}");

} else {
validateBuilder.add("if (");
if (!declaredType.primitive()) {
validateBuilder.add(propertyName + " != null && ");
}
validateBuilder.addLine("!" + allowedValuesConstant + ".contains(String.valueOf(" + propertyName + "))) {")
.add("collector.fatal(getClass(), \"Property \\\"")
.add(configKey == null ? propertyName : configKey)
.add("\\\" value is not within allowed values. Configured: \\\"\" + " + propertyName + " + \"\\\"")
.addLine(", expected one of: \\\"\" + " + allowedValuesConstant + " + \"\\\"\");");
validateBuilder.addLine("}");
}
}
}
validateBuilder.addLine("collector.collect().checkValid();");
}
Expand Down
34 changes: 27 additions & 7 deletions builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.helidon.builder.processor;

import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
Expand Down Expand Up @@ -115,6 +116,10 @@ static PrototypeProperty create(ProcessingContext processingContext,
);
}

public Field.Builder fieldDeclaration(boolean isBuilder) {
tomas-langer marked this conversation as resolved.
Show resolved Hide resolved
return typeHandler.fieldDeclaration(configuredOption(), isBuilder, !isBuilder);
}

void setters(InnerClass.Builder classBuilder, TypeName builderType, Javadoc blueprintJavadoc) {
typeHandler().setters(classBuilder,
configuredOption(),
Expand Down Expand Up @@ -144,6 +149,7 @@ TypeName builderGetterType() {
return typeHandler.builderGetterType(configuredOption.required(),
configuredOption.hasDefault());
}

String builderGetter() {
return typeHandler.generateBuilderGetter(configuredOption.required(),
configuredOption.hasDefault());
Expand All @@ -154,10 +160,6 @@ boolean builderGetterOptional() {
configuredOption.hasDefault());
}

public Field.Builder fieldDeclaration(boolean isBuilder) {
return typeHandler.fieldDeclaration(configuredOption(), isBuilder, !isBuilder);
}

private static String setterName(String name, boolean beanStyleAccessors) {
if (beanStyleAccessors || RESERVED_WORDS.contains(name)) {
return "set" + capitalize(name);
Expand Down Expand Up @@ -237,7 +239,8 @@ record ConfiguredOption(String configKey,
boolean builderMethod,
boolean notConfigured,
boolean provider,
ProviderOption providerOption) {
ProviderOption providerOption,
List<AllowedValue> allowedValues) {

static ConfiguredOption create(TypeHandler typeHandler, TypedElementInfo element) {
ConfiguredOption configuredOption = element.findAnnotation(CONFIGURED_OPTION_TYPE)
Expand Down Expand Up @@ -269,6 +272,10 @@ boolean hasDefault() {
return defaultValue != null;
}

boolean hasAllowedValues() {
return allowedValues != null && !allowedValues.isEmpty();
}

ConfiguredOption withValidateNotNull() {
return new ConfiguredOption(configKey,
description,
Expand All @@ -278,7 +285,8 @@ ConfiguredOption withValidateNotNull() {
builderMethod,
notConfigured,
provider,
providerOption);
providerOption,
allowedValues);
}

private static ConfiguredOption configuredNoAnnotation(TypeHandler typeHandler, TypedElementInfo element) {
Expand All @@ -295,6 +303,7 @@ private static ConfiguredOption configuredNoAnnotation(TypeHandler typeHandler,
true,
true,
false,
null,
null);
}

Expand All @@ -303,6 +312,13 @@ private static ConfiguredOption configuredFromAnnotation(TypeHandler typeHandler
Annotation configuredAnnotation) {
boolean required = configuredAnnotation.getValue("required").map(Boolean::parseBoolean).orElse(false);
boolean provider = configuredAnnotation.getValue("provider").map(Boolean::parseBoolean).orElse(false);
List<AllowedValue> allowedValues = configuredAnnotation.annotationValues("allowedValues")
.stream()
.flatMap(List::stream)
.map(it -> new AllowedValue(it.stringValue().orElseThrow(),
it.stringValue("description").orElseThrow()))
.toList();

return new ConfiguredOption(
toConfigKey(configuredAnnotation, typeHandler.name()),
configuredAnnotation.getValue("description")
Expand All @@ -322,7 +338,8 @@ private static ConfiguredOption configuredFromAnnotation(TypeHandler typeHandler
configuredAnnotation.getValue("builderMethod").map(Boolean::parseBoolean).orElse(true),
configuredAnnotation.getValue("notConfigured").map(Boolean::parseBoolean).orElse(false),
provider,
provider ? ProviderOption.create(configuredAnnotation) : null);
provider ? ProviderOption.create(configuredAnnotation) : null,
allowedValues);
}

private static String toConfigKey(Annotation configuredOption, String propertyName) {
Expand Down Expand Up @@ -350,4 +367,7 @@ static Singular create(String name, TypedElementInfo element) {
.orElseGet(Singular::empty);
}
}

record AllowedValue(String value, String description) {
}
}
26 changes: 14 additions & 12 deletions builder/processor/src/main/java/io/helidon/builder/processor/TypeContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Stream;

import javax.lang.model.element.TypeElement;
import javax.lang.model.util.Elements;
Expand Down Expand Up @@ -171,6 +170,9 @@ static TypeContext create(ProcessingContext processingContext,
boolean hasNonNulls = propertyMethods.stream()
.map(PrototypeProperty::configuredOption)
.anyMatch(PrototypeProperty.ConfiguredOption::validateNotNull);
boolean hasAllowedValues = propertyMethods.stream()
.map(PrototypeProperty::configuredOption)
.anyMatch(PrototypeProperty.ConfiguredOption::hasAllowedValues);
boolean prototypePublic = blueprintAnnotation.getValue("isPublic")
.map(Boolean::parseBoolean)
.orElse(true);
Expand Down Expand Up @@ -246,27 +248,26 @@ static TypeContext create(ProcessingContext processingContext,
hasRequired,
hasNonNulls,
hasProvider,
hasAllowedValues,
propertyMethods,
overridingProperties),
CustomMethods.create(processingContext, typeInformation));
}

static List<String> annotationsToGenerate(Annotated annotated) {
return annotated.findAnnotation(Types.PROTOTYPE_ANNOTATED_TYPE)
.flatMap(Annotation::value)
.map(annotation -> annotation.split(","))
.map(List::of)
.orElseGet(List::of);
.flatMap(Annotation::stringValues)
.stream()
.flatMap(List::stream)
.toList();
}

private static List<TypeName> prototypeImplements(Annotation annotation) {
return annotation.value()
.map(value -> {
return Stream.of(value.split(","))
.map(TypeName::create)
.toList();
})
.orElseGet(List::of);
return annotation.stringValues()
.stream()
.flatMap(List::stream)
.map(TypeName::create)
.toList();
}

private static void gatherExtends(TypeInfo typeInfo, Set<TypeName> extendList,
Expand Down Expand Up @@ -465,6 +466,7 @@ record PropertyData(
boolean hasRequired,
boolean hasNonNulls,
boolean hasProvider,
boolean hasAllowedValues,
List<PrototypeProperty> properties,
List<PrototypeProperty> overridingProperties) {
}
Expand Down
Loading