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.

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

Property Placeholder No Longer Resolves to Null for Nullable Arguments When Environment Variable Does Not Exist #10797

Closed
helios2k6 opened this issue May 3, 2024 · 6 comments

Comments

@helios2k6
Copy link
Contributor

helios2k6 commented May 3, 2024

Expected Behavior

Property placeholders should resolve to null when they are not specified by an environment variable.

Summary

Given a Bean Factory, a @Nullable argument that retrieves its value from a property via the @Value annotation, and this value is expected to be resolved through the environment property source, Micronaut will now throw an exception stating that it could not resolve the placeholder.

Example

Configuration File

some:
  optional:
    value: ${SOME_OPTIONAL_VALUE_PLACEHOLDER}

Assume the SOME_OPTIONAL_VALUE_PLACEHOLDER environment variable has not been set.

Factory Class

@Factory
class ExampleBeanFactory {
  @Singleton
  fun exampleBean(
    @Nullable
    @Value("\${some.optional.value}")
    optionalValue: String?,
  ): ExampleBean {
    return ExampleBean(optionalValue)
  }

Actual Behaviour

An exception is thrown stating that the placeholder could not be resolved.

Steps To Reproduce

  1. Use Kotlin
  2. Create configuration file that uses a placeholder
  3. Use the Environment Property Source
  4. Create bean factory
  5. Create a function within the bean factory class with the @Singleton annotation and a nullable argument that has the @Nullable and @Value annotations
  6. Do not set the placeholder value equal to anything in your environment
  7. Observe the exception is thrown

Environment Information

OS: Pop!_OS 22.04 LTS
Java Version:

java --version
openjdk 17.0.8.1 2023-08-24 LTS
OpenJDK Runtime Environment Zulu17.44+53-CA (build 17.0.8.1+1-LTS)
OpenJDK 64-Bit Server VM Zulu17.44+53-CA (build 17.0.8.1+1-LTS, mixed mode, sharing)

Micronaut Dependencies and Versions:

gradle.properties

micronaut_kotlin_runtime_version=4.3.0
micronaut_version=4.4.6
snake_yaml_version=2.2
kotlin_version=1.9.23
kotlinx_coroutines_core_version=1.8.0
all_open_plugin_version=1.9.23

gradle.build

plugins {
    id "application"
    id "idea"
    id "org.jetbrains.kotlin.jvm"
    id "org.jetbrains.kotlin.kapt"
    id "org.jetbrains.kotlin.plugin.allopen" version "$all_open_plugin_version"
    id "org.jlleitschuh.gradle.ktlint"
}

kapt {
    correctErrorTypes true
}

dependencies {
    // Kotlin stdlib
    implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
    implementation "org.jetbrains.kotlin:kotlin-reflect:$kotlin_version"
    implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$kotlinx_coroutines_core_version"
    implementation "org.jetbrains.kotlinx:kotlinx-coroutines-jdk8:$kotlinx_coroutines_core_version"

    // Micronaut
    kapt "io.micronaut:micronaut-inject-java:$micronaut_version"
    implementation "io.micronaut.kotlin:micronaut-kotlin-runtime:$micronaut_kotlin_runtime_version"
    implementation "io.micronaut:micronaut-runtime:$micronaut_version"

    // YAML configuration.
    runtimeOnly "org.yaml:snakeyaml:$snake_yaml_version"
}

<snip>

It should be noted that this bug does not present itself in micronaut versions:

micronaut_kotlin_runtime_version=4.0.2
micronaut_version=4.1.8

With the remaining dependencies held the same.

Example Application

No response

Version

4.4.6

@graemerocher
Copy link
Contributor

you actually shouldn't need the nullable annotation

@helios2k6
Copy link
Contributor Author

I just tried it without the nullable annotation. It still throws an exception: Message: Could not resolve placeholder ${SOME_OPTIONAL_VALUE_PLACEHOLDER}

@helios2k6
Copy link
Contributor Author

helios2k6 commented May 3, 2024

Looking through the code and change history, I've narrowed down the change that has caused the issue.

In #9701, @dstepanov removed a try-catch statement in the DefaultPropertyPlaceholderResolver::resolvePlaceholders() function which originally would have returned an Optional.empty() should a placeholder not be resolved, which would then be checked in the AbstractInitializableBeanDefinition::resolvePropertyValue() at line 2135 (https://github.com/micronaut-projects/micronaut-core/blob/4.5.x/inject/src/main/java/io/micronaut/context/AbstractInitializableBeanDefinition.java#L2135) by calling argument.isDeclaredNullable().

Now, since the DefaultPropertyPlaceHolderResolver simply throws a ConfigurationException without catching it and then determining if the argument was nullable, the application crashes, changing previous behavior of the framework.

I propose we reintroduce this try-catch statement to preserve original behavior.

@helios2k6
Copy link
Contributor Author

I should have added this before, but here's the relevant stack-trace for my bug:

Caused by: io.micronaut.context.exceptions.ConfigurationException: Could not resolve placeholder ${SOME_PLACEHOLDER}
	at io.micronaut.context.env.DefaultPropertyPlaceholderResolver$PlaceholderSegment.getValue(DefaultPropertyPlaceholderResolver.java:391)
	at io.micronaut.context.env.DefaultPropertyPlaceholderResolver.resolveRequiredPlaceholdersObject(DefaultPropertyPlaceholderResolver.java:116)
	at io.micronaut.context.env.PropertySourcePropertyResolver.resolvePlaceHoldersIfNecessary(PropertySourcePropertyResolver.java:791)
	at io.micronaut.context.env.PropertySourcePropertyResolver.getProperty(PropertySourcePropertyResolver.java:388)
	at io.micronaut.core.value.PropertyResolver.getProperty(PropertyResolver.java:140)
	at io.micronaut.core.value.PropertyResolver.getProperty(PropertyResolver.java:157)
	at io.micronaut.context.env.DefaultPropertyPlaceholderResolver.resolveOptionalExpression(DefaultPropertyPlaceholderResolver.java:245)
	at io.micronaut.context.env.DefaultPropertyPlaceholderResolver$PlaceholderSegment.findValue(DefaultPropertyPlaceholderResolver.java:398)
	at io.micronaut.context.env.DefaultPropertyPlaceholderResolver.resolvePlaceholders(DefaultPropertyPlaceholderResolver.java:96)
	at io.micronaut.context.DefaultApplicationContext.resolvePlaceholders(DefaultApplicationContext.java:649)
	at io.micronaut.context.AbstractInitializableBeanDefinition.resolvePropertyValue(AbstractInitializableBeanDefinition.java:2101)
	at io.micronaut.context.AbstractInitializableBeanDefinition.getPropertyPlaceholderValueForConstructorArgument(AbstractInitializableBeanDefinition.java:1437)
	at XXXXXXXXX.$XXXXXXXX$XXXXXXXXXXXXX1$Definition.instantiate(Unknown Source)
	at io.micronaut.context.DefaultBeanContext.resolveByBeanFactory(DefaultBeanContext.java:2311)

@helios2k6
Copy link
Contributor Author

In another note, the Segment interface that the PlaceholderSegment class overrides has a default implementation for findValue() that explicitly catches the ConfigurationException, but the overridden version of that function does not, breaking the contract of the interface itself.

@helios2k6
Copy link
Contributor Author

Submitted PR #10798 for review.

@sdelamo sdelamo closed this as completed in 12fa3c3 May 8, 2024
sdelamo pushed a commit that referenced this issue May 8, 2024
…laceholderResolver (#10798)

Fixes #10797 
* Adding try-catch for ConfigurationExceptions which are thrown when resolving env properties
* Adding test cases for placeholder resolution, both with and without the placeholder set
* Removing irrelevant print statement and method call to vehicle for the purposes of the specified unit tests
sdelamo pushed a commit that referenced this issue May 9, 2024
…laceholderResolver (#10798)

Fixes #10797 
* Adding try-catch for ConfigurationExceptions which are thrown when resolving env properties
* Adding test cases for placeholder resolution, both with and without the placeholder set
* Removing irrelevant print statement and method call to vehicle for the purposes of the specified unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants