Skip to content

Log a fail/warn when config metadata default values are unable to be resolved #28062

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.

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

Closed
wants to merge 11 commits into from
Closed
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@
"description": "Whether to enable disk space health check.",
"defaultValue": true
},
{
"name": "management.health.diskspace.path",
"defaultValue": "."
},
{
"name": "management.health.elasticsearch.enabled",
"type": "java.lang.Boolean",
Expand Down Expand Up @@ -462,6 +466,10 @@
"level": "error"
}
},
{
"name": "management.metrics.export.newrelic.client-provider-type",
"defaultValue": "INSIGHTS_API"
},
{
"name": "management.metrics.export.prometheus.enabled",
"type": "java.lang.Boolean",
Expand Down Expand Up @@ -540,6 +548,10 @@
"level": "error"
}
},
{
"name": "management.metrics.export.wavefront.sender.message-size",
"defaultValue": "2147483647B"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting one DataSize.ofBytes(Integer.MAX_VALUE).

},
{
"name": "management.metrics.mongo.command.enabled",
"description": "Whether to enable Mongo client command metrics.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@
"level": "error"
}
},
{
"name": "server.undertow.accesslog.dir",
"defaultValue": "logs"
},
{
"name": "server.undertow.buffers-per-region",
"type": "java.lang.Integer",
Expand All @@ -258,6 +262,14 @@
"level": "error"
}
},
{
"name": "server.undertow.max-headers",
"defaultValue": 200
},
{
"name": "server.undertow.max-parameters",
"defaultValue": 1000
},
{
"name": "server.use-forward-headers",
"type": "java.lang.Boolean",
Expand Down Expand Up @@ -336,6 +348,14 @@
"level": "error"
}
},
{
"name": "spring.artemis.embedded.cluster-password",
"defaultValue": "UUID.randomUUID().toString()"
Copy link
Contributor Author

@onobc onobc Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, like you said, this is an interesting one. It is a random string value. We could instead do something like <random-uuid-string> but "UUID.randomUUID().toString()" says that as well and also gives a hint to how its random generated.

},
{
"name": "spring.artemis.embedded.server-id",
"defaultValue": 0
},
{
"name": "spring.artemis.pool.maximum-active-session-per-connection",
"deprecation": {
Expand Down Expand Up @@ -1088,10 +1108,22 @@
"name": "spring.info.git.location",
"defaultValue": "classpath:git.properties"
},
{
"name": "spring.integration.channel.max-broadcast-subscribers",
"defaultValue": 2147483647
},
{
"name": "spring.integration.channel.max-unicast-subscribers",
"defaultValue": 2147483647
},
{
"name": "spring.integration.jdbc.initialize-schema",
"defaultValue": "embedded"
},
{
"name": "spring.integration.poller.max-messages-per-poll",
"defaultValue": "-2147483648"
},
{
"name": "spring.jackson.constructor-detector",
"defaultValue": "default"
Expand Down Expand Up @@ -1459,10 +1491,20 @@
"level": "error"
}
},
{
"name": "spring.kafka.bootstrap-servers",
"defaultValue": [
"localhost:9092"
]
},
{
"name": "spring.kafka.consumer.isolation-level",
"defaultValue": "read-uncommitted"
},
{
"name": "spring.kafka.consumer.key-deserializer",
"defaultValue": "org.apache.kafka.common.serialization.StringDeserializer"
},
{
"name": "spring.kafka.consumer.ssl.keystore-location",
"type": "org.springframework.core.io.Resource",
Expand Down Expand Up @@ -1499,6 +1541,10 @@
"level": "error"
}
},
{
"name": "spring.kafka.consumer.value-deserializer",
"defaultValue": "org.apache.kafka.common.serialization.StringDeserializer"
},
{
"name": "spring.kafka.jaas.control-flag",
"defaultValue": "required"
Expand All @@ -1507,6 +1553,10 @@
"name": "spring.kafka.listener.type",
"defaultValue": "single"
},
{
"name": "spring.kafka.producer.key-serializer",
"defaultValue": "org.apache.kafka.common.serialization.StringSerializer"
},
{
"name": "spring.kafka.producer.ssl.keystore-location",
"type": "org.springframework.core.io.Resource",
Expand Down Expand Up @@ -1543,6 +1593,10 @@
"level": "error"
}
},
{
"name": "spring.kafka.producer.value-serializer",
"defaultValue": "org.apache.kafka.common.serialization.StringSerializer"
},
{
"name": "spring.kafka.ssl.keystore-location",
"type": "org.springframework.core.io.Resource",
Expand Down Expand Up @@ -1736,6 +1790,10 @@
"level": "error"
}
},
{
"name": "spring.rabbitmq.stream.port",
"defaultValue": 5552
},
{
"name": "spring.rabbitmq.template.queue",
"type": "java.lang.String",
Expand Down Expand Up @@ -1967,6 +2025,10 @@
"name": "spring.security.filter.order",
"defaultValue": -100
},
{
"name": "spring.security.user.password",
"defaultValue": "UUID.randomUUID().toString()"
},
{
"name": "spring.session.hazelcast.flush-mode",
"defaultValue": "on-save"
Expand Down Expand Up @@ -2007,6 +2069,10 @@
"request"
]
},
{
"name": "spring.session.servlet.filter-order",
"defaultValue": -2147483598
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not convey the same hint that SessionRepositoryFilter.DEFAULT_ORDER does (which in turn is = Integer.MIN_VALUE + 50)

},
{
"name": "spring.sql.init.enabled",
"type": "java.lang.Boolean",
Expand All @@ -2021,6 +2087,13 @@
"name": "spring.sql.init.mode",
"defaultValue": "embedded"
},
{
"name": "spring.task.execution.pool.max-size",
"defaultValue": 2147483647
}, {
"name": "spring.task.execution.pool.queue-capacity",
"defaultValue": 2147483647
},
{
"name": "spring.web.locale-resolver",
"defaultValue": "accept-header"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import javax.lang.model.util.ElementFilter;
import javax.tools.Diagnostic.Kind;

import org.springframework.boot.configurationprocessor.fieldvalues.ValueWrapper;
import org.springframework.boot.configurationprocessor.metadata.ConfigurationMetadata;
import org.springframework.boot.configurationprocessor.metadata.InvalidConfigurationMetadataException;
import org.springframework.boot.configurationprocessor.metadata.ItemMetadata;
Expand All @@ -57,6 +58,7 @@
* @author Phillip Webb
* @author Kris De Volder
* @author Jonas Keßler
* @author Chris Bono
* @since 1.2.0
*/
@SupportedAnnotationTypes({ ConfigurationMetadataAnnotationProcessor.CONFIGURATION_PROPERTIES_ANNOTATION,
Expand All @@ -71,6 +73,8 @@ public class ConfigurationMetadataAnnotationProcessor extends AbstractProcessor

static final String ADDITIONAL_METADATA_LOCATIONS_OPTION = "org.springframework.boot.configurationprocessor.additionalMetadataLocations";

static final String FAIL_ON_UNDETERMINED_DEFAULT_VALUE_OPTION = "org.springframework.boot.configurationprocessor.failOnUndeterminedDefaultValue";

static final String CONFIGURATION_PROPERTIES_ANNOTATION = "org.springframework.boot.context.properties.ConfigurationProperties";

static final String NESTED_CONFIGURATION_PROPERTY_ANNOTATION = "org.springframework.boot.context.properties.NestedConfigurationProperty";
Expand All @@ -97,8 +101,8 @@ public class ConfigurationMetadataAnnotationProcessor extends AbstractProcessor

static final String NAME_ANNOTATION = "org.springframework.boot.context.properties.bind.Name";

private static final Set<String> SUPPORTED_OPTIONS = Collections
.unmodifiableSet(Collections.singleton(ADDITIONAL_METADATA_LOCATIONS_OPTION));
private static final Set<String> SUPPORTED_OPTIONS = Collections.unmodifiableSet(new HashSet<>(
Arrays.asList(ADDITIONAL_METADATA_LOCATIONS_OPTION, FAIL_ON_UNDETERMINED_DEFAULT_VALUE_OPTION)));

private MetadataStore metadataStore;

Expand Down Expand Up @@ -328,6 +332,7 @@ protected ConfigurationMetadata writeMetaData() throws Exception {
ConfigurationMetadata metadata = this.metadataCollector.getMetadata();
metadata = mergeAdditionalMetadata(metadata);
if (!metadata.getItems().isEmpty()) {
validateDefaultValuesResolved(metadata);
this.metadataStore.writeMetadata(metadata);
return metadata;
}
Expand All @@ -353,6 +358,26 @@ private ConfigurationMetadata mergeAdditionalMetadata(ConfigurationMetadata meta
return metadata;
}

private void validateDefaultValuesResolved(ConfigurationMetadata metadata) {

boolean failOnBadDefaultValues = Boolean.valueOf(this.processingEnv.getOptions().getOrDefault(
ConfigurationMetadataAnnotationProcessor.FAIL_ON_UNDETERMINED_DEFAULT_VALUE_OPTION, "true"));

metadata.getItems().forEach((itemMetadata) -> {
if (itemMetadata.getDefaultValue() instanceof ValueWrapper) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not propagated the change from Object -> ValueWrapper up from FVP to ItemMetadata yet but will do that shortly. Unless you can think of a reason to keep it Object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason to keep it Object is to prevent the ripple around all areas that are accessing ItemMetadata#defaultValue (JsonConverter, MetadataStore, etc..).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I didn't even think of changing ItemMetadata to be honest. I was hoping we could trigger the warning/error locally to field resolution by passing the necessary bits to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we have to wait until the additional metadata is merged to see if the user added a manual hint.

ValueWrapper defaultValue = (ValueWrapper) itemMetadata.getDefaultValue();
if (!defaultValue.valueDetermined()) {
String msg = String.format(
"Could not resolve default value for property '%s' using expression '%s'. Add 'defaultValue' in additional-spring-configuration.metadata.json",
itemMetadata.getName(), defaultValue.valueInitializerExpression());
log(failOnBadDefaultValues ? Kind.ERROR : Kind.WARNING, msg);
}
itemMetadata.setDefaultValue(defaultValue.value());
}

});
}

private String getStackTrace(Exception ex) {
StringWriter writer = new StringWriter();
ex.printStackTrace(new PrintWriter(writer, true));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2019 the original author or authors.
* Copyright 2012-2021 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -27,6 +27,7 @@
* Parser which can be used to obtain the field values from an {@link TypeElement}.
*
* @author Phillip Webb
* @author Chris Bono
* @since 1.1.2
* @see JavaCompilerFieldValuesParser
*/
Expand All @@ -42,8 +43,8 @@ public interface FieldValuesParser {
* Return the field values for the given element.
* @param element the element to inspect
* @return a map of field names to values.
* @throws Exception if the values cannot be extracted
* @throws Exception if extraction fails unexpectedly
*/
Map<String, Object> getFieldValues(TypeElement element) throws Exception;
Map<String, ValueWrapper> getFieldValues(TypeElement element) throws Exception;

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Copyright 2012-2021 the original author or authors.
*
* 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
*
* https://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 org.springframework.boot.configurationprocessor.fieldvalues;

import java.util.Objects;

import javax.lang.model.element.TypeElement;

/**
* The value assigned to a field in a {@link TypeElement}, including information related
* to value resolution.
*
* @author Chris Bono
* @since 2.6
*/
public final class ValueWrapper {

private final Object value;

private final String valueInitializerExpression;

private final boolean valueDetermined;

private ValueWrapper(Object value, String valueInitializerExpression, boolean valueDetermined) {
this.value = value;
this.valueInitializerExpression = valueInitializerExpression;
this.valueDetermined = valueDetermined;
}

public static ValueWrapper of(Object value, String valueInitializerExpression) {
return new ValueWrapper(value, valueInitializerExpression, true);
}

public static ValueWrapper unresolvable(String valueInitializerExpression) {
return new ValueWrapper(null, valueInitializerExpression, false);
}

/**
* Return the wrapped value.
* @return the wrapped value or {@code null} if the value was initialized with
* {@code null} or the initializer expression was {@link #valueDetermined() unable to
* be determined}.
*/
public Object value() {
return this.value;
}

/**
* Return the expression used to initialize the value.
* @return the expression used to initialize the value or {@code null} if no
* initializer was used to determine the value
*/
public String valueInitializerExpression() {
return this.valueInitializerExpression;
}

/**
* Return whether or not the value was able to be determined.
* @return {@code true} if the value was able to be determined, {@code false}
* otherwise.
*/
public boolean valueDetermined() {
return this.valueDetermined;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
ValueWrapper other = (ValueWrapper) o;
return this.valueDetermined == other.valueDetermined && Objects.deepEquals(this.value, other.value)
&& Objects.equals(this.valueInitializerExpression, other.valueInitializerExpression);
}

@Override
public int hashCode() {
return Objects.hash(this.value, this.valueInitializerExpression, this.valueDetermined);
}

@Override
public String toString() {
return "ValueWrapper{value=" + this.value + ", valueInitializerExpression=" + this.valueInitializerExpression
+ ", valueDetermined=" + this.valueDetermined + "}";
}

}
Loading