Skip to content

Fix/declarative config no exceptions #8079

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

Merged
jack-berg merged 6 commits into from
Mar 30, 2026
Merged

Fix/declarative config no exceptions #8079

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df9a72b
Implement Environment Variable Context Propagation carriers in api/in…
Bhagirath00 Feb 12, 2026
6eed41c
Update DeclarativeConfigProperties to return null on type mismatch
Bhagirath00 Feb 13, 2026
2012cd9
Address feedback: clean up branch and improve safety in DeclarativeCo…
Bhagirath00 Feb 16, 2026
153c5d1
Improve null check messages for config property name and default value
Bhagirath00 Feb 18, 2026
f6e7582
Apply suggestion from @jack-berg
jack-berg Mar 30, 2026
df0a7b7
spotless
jack-berg Mar 30, 2026
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
57 changes: 23 additions & 34 deletions ...ator/src/main/java/io/opentelemetry/api/incubator/config/DeclarativeConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ static Map<String, Object> toMap(DeclarativeConfigProperties declarativeConfigPr
/**
* Returns a {@link String} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar string
* @return null if the property has not been configured or if the property is not a valid scalar
* string
*/
@Nullable
String getString(String name);
Expand All @@ -56,8 +56,7 @@ static Map<String, Object> toMap(DeclarativeConfigProperties declarativeConfigPr
* Returns a {@link String} configuration property.
*
* @return a {@link String} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar string
* {@code name} has not been configured or is not a valid scalar string
*/
default String getString(String name, String defaultValue) {
Copy link
Copy Markdown
Contributor

@robsunday robsunday Feb 18, 2026

Choose a reason for hiding this comment

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

For consistency I'd recommend adding here also:
Objects.requireNonNull(defaultValue, "Null default value");

Copy link
Copy Markdown
Contributor Author

@Bhagirath00 Bhagirath00 Feb 18, 2026

Choose a reason for hiding this comment

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

Makes sense, added the null check for defaultValue too.

Copy link
Copy Markdown
Member

@jack-berg jack-berg Mar 10, 2026

Choose a reason for hiding this comment

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

We need to get consistent about whether or not we do this. We rely on javax.annotation.Nullable annotation, the assumption that params and returns are non-null unless otherwise specified, leveraging the net.ltgt.nullaway error prone plugin to validate at build time. Of course, at runtime there is no guarantee that callers adhere to these annotations, and so we have code like this which checks for null despite the params being non-null according to the annotations.

We need to be consistent about:

  • When and why do we add additional null checks
  • Where do we do this (i.e. in interface default methods or in the actual implementation)
  • What is the behavior when the null annotation is violated (throw or noop)

Let's hold off here until we get some repo level guidance on this. ConfigProperties, the env var / sys property analog of DeclarativeConfigProperties, has the exact same method and does not do this null check.

Copy link
Copy Markdown
Member

@jack-berg jack-berg Mar 10, 2026

Choose a reason for hiding this comment

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

Created #8173 to track.

return defaultIfNull(getString(name), defaultValue);
Expand All @@ -67,8 +66,8 @@ default String getString(String name, String defaultValue) {
* Returns a {@link Boolean} configuration property. Implementations should use the same rules as
* {@link Boolean#parseBoolean(String)} for handling the values.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar boolean
* @return null if the property has not been configured or if the property is not a valid scalar
* boolean
*/
@Nullable
Boolean getBoolean(String name);
Expand All @@ -77,8 +76,7 @@ default String getString(String name, String defaultValue) {
* Returns a {@link Boolean} configuration property.
*
* @return a {@link Boolean} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar boolean
* {@code name} has not been configured or is not a valid scalar boolean
*/
default boolean getBoolean(String name, boolean defaultValue) {
return defaultIfNull(getBoolean(name), defaultValue);
Expand All @@ -90,8 +88,8 @@ default boolean getBoolean(String name, boolean defaultValue) {
* <p>If the underlying config property is {@link Long}, it is converted to {@link Integer} with
* {@link Long#intValue()} which may result in loss of precision.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar integer
* @return null if the property has not been configured or if the property is not a valid scalar
* integer
*/
@Nullable
Integer getInt(String name);
Expand All @@ -103,8 +101,7 @@ default boolean getBoolean(String name, boolean defaultValue) {
* {@link Long#intValue()} which may result in loss of precision.
*
* @return a {@link Integer} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar integer
* {@code name} has not been configured or is not a valid scalar integer
*/
default int getInt(String name, int defaultValue) {
return defaultIfNull(getInt(name), defaultValue);
Expand All @@ -113,8 +110,8 @@ default int getInt(String name, int defaultValue) {
/**
* Returns a {@link Long} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar long
* @return null if the property has not been configured or if the property is not a valid scalar
* long
*/
@Nullable
Long getLong(String name);
Expand All @@ -123,8 +120,7 @@ default int getInt(String name, int defaultValue) {
* Returns a {@link Long} configuration property.
*
* @return a {@link Long} configuration property or {@code defaultValue} if a property with {@code
* name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar long
* name} has not been configured or is not a valid scalar long
*/
default long getLong(String name, long defaultValue) {
return defaultIfNull(getLong(name), defaultValue);
Expand All @@ -133,8 +129,8 @@ default long getLong(String name, long defaultValue) {
/**
* Returns a {@link Double} configuration property.
*
* @return null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar double
* @return null if the property has not been configured or if the property is not a valid scalar
* double
*/
@Nullable
Double getDouble(String name);
Expand All @@ -143,8 +139,7 @@ default long getLong(String name, long defaultValue) {
* Returns a {@link Double} configuration property.
*
* @return a {@link Double} configuration property or {@code defaultValue} if a property with
* {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid scalar double
* {@code name} has not been configured or is not a valid scalar double
*/
default double getDouble(String name, double defaultValue) {
return defaultIfNull(getDouble(name), defaultValue);
Expand All @@ -158,8 +153,8 @@ default double getDouble(String name, double defaultValue) {
* @param scalarType the scalar type, one of {@link String}, {@link Boolean}, {@link Long} or
* {@link Double}
* @return a {@link List} configuration property, or null if the property has not been configured
* @throws DeclarativeConfigException if the property is not a valid sequence of scalars, or if
* {@code scalarType} is not supported
* or if the property is not a valid sequence of scalars
* @throws DeclarativeConfigException if {@code scalarType} is not supported
*/
@Nullable
<T> List<T> getScalarList(String name, Class<T> scalarType);
Expand All @@ -172,8 +167,7 @@ default double getDouble(String name, double defaultValue) {
* @param scalarType the scalar type, one of {@link String}, {@link Boolean}, {@link Long} or
* {@link Double}
* @return a {@link List} configuration property or {@code defaultValue} if a property with {@code
* name} has not been configured
* @throws DeclarativeConfigException if the property is not a valid sequence of scalars
* name} has not been configured or is not a valid sequence of scalars
*/
default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defaultValue) {
return defaultIfNull(getScalarList(name, scalarType), defaultValue);
Expand All @@ -183,8 +177,7 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
* Returns a {@link DeclarativeConfigProperties} configuration property.
*
* @return a map-valued configuration property, or {@code null} if {@code name} has not been
* configured
* @throws DeclarativeConfigException if the property is not a mapping
* configured or is not a mapping
*/
@Nullable
DeclarativeConfigProperties getStructured(String name);
Expand All @@ -193,8 +186,7 @@ default <T> List<T> getScalarList(String name, Class<T> scalarType, List<T> defa
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a map-valued configuration property, or {@code defaultValue} if {@code name} has not
* been configured
* @throws DeclarativeConfigException if the property is not a mapping
* been configured or is not a mapping
*/
default DeclarativeConfigProperties getStructured(
String name, DeclarativeConfigProperties defaultValue) {
Expand All @@ -210,8 +202,7 @@ default DeclarativeConfigProperties getStructured(
* but empty vs. not set, use {@link #getStructured(String)}.
*
* @return a map-valued configuration property, or an empty {@link DeclarativeConfigProperties}
* instance if {@code name} has not been configured
* @throws DeclarativeConfigException if the property is not a mapping
* instance if {@code name} has not been configured or is not a mapping
*/
default DeclarativeConfigProperties get(String name) {
return getStructured(name, empty());
Expand All @@ -221,8 +212,7 @@ default DeclarativeConfigProperties get(String name) {
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a list of map-valued configuration property, or {@code null} if {@code name} has not
* been configured
* @throws DeclarativeConfigException if the property is not a sequence of mappings
* been configured or is not a sequence of mappings
*/
@Nullable
List<DeclarativeConfigProperties> getStructuredList(String name);
Expand All @@ -231,8 +221,7 @@ default DeclarativeConfigProperties get(String name) {
* Returns a list of {@link DeclarativeConfigProperties} configuration property.
*
* @return a list of map-valued configuration property, or {@code defaultValue} if {@code name}
* has not been configured
* @throws DeclarativeConfigException if the property is not a sequence of mappings
* has not been configured or is not a sequence of mappings
*/
default List<DeclarativeConfigProperties> getStructuredList(
String name, List<DeclarativeConfigProperties> defaultValue) {
Expand Down
11 changes: 10 additions & 1 deletion .../io/opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,21 @@ private static boolean isMap(Object object) {
@Nullable
@Override
public String getString(String name) {
Objects.requireNonNull(name, "name");
Copy link
Copy Markdown
Contributor

@robsunday robsunday Feb 18, 2026

Choose a reason for hiding this comment

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

The only doubt I have here is that the result of getString(null) will be NPE with quite cryptic message "name".
Maybe the message should be a bit more descriptive? Like "Null configuration property name".
This applies to all changes below.

Copy link
Copy Markdown
Contributor Author

@Bhagirath00 Bhagirath00 Feb 18, 2026

Choose a reason for hiding this comment

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

Good catch, updated the message to be more descriptive. Thanks!

return stringOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Boolean getBoolean(String name) {
Objects.requireNonNull(name, "name");
return booleanOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Integer getInt(String name) {
Objects.requireNonNull(name, "name");
Object value = simpleEntries.get(name);
if (value instanceof Integer) {
return (Integer) value;
Expand All @@ -184,19 +187,22 @@ public Integer getInt(String name) {
@Nullable
@Override
public Long getLong(String name) {
Objects.requireNonNull(name, "name");
return longOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
public Double getDouble(String name) {
Objects.requireNonNull(name, "name");
return doubleOrNull(simpleEntries.get(name), name);
}

@Nullable
@Override
@SuppressWarnings("unchecked")
public <T> List<T> getScalarList(String name, Class<T> scalarType) {
Objects.requireNonNull(name, "name");
if (!SUPPORTED_SCALAR_TYPES.contains(scalarType)) {
throw new DeclarativeConfigException(
"Unsupported scalar type "
Expand Down Expand Up @@ -291,12 +297,14 @@ private static Double doubleOrNull(@Nullable Object value, String name) {
@Nullable
@Override
public DeclarativeConfigProperties getStructured(String name) {
Objects.requireNonNull(name, "name");
return mapEntries.get(name);
}

@Nullable
@Override
public List<DeclarativeConfigProperties> getStructuredList(String name) {
Objects.requireNonNull(name, "name");
List<YamlDeclarativeConfigProperties> value = listEntries.get(name);
if (value != null) {
return Collections.unmodifiableList(value);
Expand All @@ -315,7 +323,8 @@ public Set<String> getPropertyKeys() {

@Override
public String toString() {
StringJoiner joiner = new StringJoiner(", ", "YamlDeclarativeConfigProperties{", "}");
StringJoiner joiner =
new StringJoiner(", ", YamlDeclarativeConfigProperties.class.getSimpleName() + "{", "}");
simpleEntries.forEach((key, value) -> joiner.add(key + "=" + value));
listEntries.forEach((key, value) -> joiner.add(key + "=" + value));
mapEntries.forEach((key, value) -> joiner.add(key + "=" + value));
Expand Down
28 changes: 27 additions & 1 deletion ...opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ void additionalProperties() {
.isEqualTo(Arrays.asList(1.1d, 2.2d));
assertThat(otherProps.getScalarList("bool_list_key", Boolean.class))
.isEqualTo(Arrays.asList(true, false));
// If reading a scalar list which is mixed, entries which are not aligned with the requested
// If reading a scalar list which is mixed, entries which are not aligned with
// the requested
// type are filtered out
assertThat(otherProps.getScalarList("mixed_list_key", String.class))
.isEqualTo(Collections.singletonList("val1"));
Expand Down Expand Up @@ -230,8 +231,13 @@ void wrongType() {
assertThat(otherProps.getDouble("str_key")).isNull();
assertThat(otherProps.getBoolean("str_key")).isNull();
assertThat(otherProps.getScalarList("str_key", String.class)).isNull();
assertThat(otherProps.getScalarList("str_list_key", Long.class)).isNull();
Comment thread
Bhagirath00 marked this conversation as resolved.
assertThat(otherProps.getScalarList("str_list_key", Boolean.class)).isNull();
assertThat(otherProps.getScalarList("str_list_key", Double.class)).isNull();
assertThat(otherProps.getStructured("str_key")).isNull();
assertThat(otherProps.getStructuredList("str_key")).isNull();
assertThat(otherProps.getStructured("str_list_key")).isNull();
assertThat(otherProps.getStructuredList("map_key")).isNull();

assertWarning("Ignoring value for key [int_key] because it is Integer instead of String: 1");
assertWarning(
Expand All @@ -240,6 +246,26 @@ void wrongType() {
"Ignoring value for key [str_key] because it is String instead of Double: str_value");
assertWarning(
"Ignoring value for key [str_key] because it is String instead of Boolean: str_value");
assertWarning(
"Ignoring value for key [str_list_key] because it is String instead of Long: val1");
}

@Test
void wrongTypeWithDefault() {
DeclarativeConfigProperties otherProps = structuredConfigProps.getStructured("other");
assertThat(otherProps).isNotNull();

assertThat(otherProps.getString("int_key", "default")).isEqualTo("default");
assertThat(otherProps.getInt("str_key", 100)).isEqualTo(100);
assertThat(otherProps.getLong("str_key", 100L)).isEqualTo(100L);
assertThat(otherProps.getDouble("str_key", 1.1)).isEqualTo(1.1);
assertThat(otherProps.getBoolean("str_key", true)).isTrue();
assertThat(
otherProps.getScalarList("str_key", String.class, Collections.singletonList("default")))
.isEqualTo(Collections.singletonList("default"));
assertThat(otherProps.getStructured("str_key", empty())).isEqualTo(empty());
assertThat(otherProps.getStructuredList("str_key", Collections.emptyList()))
.isEqualTo(Collections.emptyList());
}

private void assertWarning(String message) {
Expand Down
Loading