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

Only convert config placeholders when necessary #9310

Merged
merged 2 commits into from
May 25, 2023
Merged

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented May 25, 2023

No description provided.

@yawkat yawkat requested a review from graemerocher May 25, 2023 12:00
@graemerocher graemerocher merged commit ac216c5 into 4.0.x May 25, 2023
3 of 5 checks passed
@graemerocher graemerocher deleted the object-placeholder branch May 25, 2023 13:09
yawkat added a commit that referenced this pull request May 25, 2023
This fixes the build broken by #9310. Basically:

https://github.com/micronaut-projects/micronaut-core/blob/94f458ac04b0180a989d0cf28984d5e3b5df592f/http-client/src/test/groovy/io/micronaut/http/client/filter/StaticClientFilterSpec.groovy#L24-L28

The resolution of micronaut.server.port used to request the value of test-port as String. With this patch, it requests the value as Object instead (`.getValue(Object.class)`). Object is not cachable however, so it gets a new uncached random port, which causes the test failure.

I don't really understand the distinction between cachable and non-cachable types. Making all types cacheable causes other test failures that request types like Map. This change (allowing caching for Object) seems to only affect placeholder resolution, and does not cause additional test failures.
@yawkat
Copy link
Member Author

yawkat commented May 25, 2023

@graemerocher this breaks the build because it breaks caching for placeholders. Basically:

['spec.name': StaticClientFilterSpec.simpleName,
'test-port': '${random.port}',
'micronaut.server.port': '${test-port}',
'micronaut.http.services.a.url': 'http://localhost:${test-port}',
'micronaut.http.services.b.url': 'http://localhost:${test-port}',

The resolution of micronaut.server.port used to request the value of test-port as String. With this patch, it requests the value as Object instead (.getValue(Object.class)). Object is not cachable however, so it gets a new uncached random port, which causes the test failure.

I'm not really sure how to fix it. I tried making Object cachable. That did not resolve the issue, because the caching for Object and String happens separately. I think this logic is a bit weird in the first place, e.g. if you were to request test-port manually as int, it will return a new random port. But I don't want to open the can of worms of rewriting this.

public <T> Optional<T> getProperty(@NonNull String name, @NonNull ArgumentConversionContext<T> conversionContext) {

Maybe we just need to revert this PR.

@yawkat
Copy link
Member Author

yawkat commented May 25, 2023

It seems to me like caching is a bit "misused" here. For the caching purposes (converting the same value to the same type every time) it makes sense to cache this way. But for preventing regeneration of the random value, the value should be cached regardless of requested type. Maybe there should be a two-level cache, one depending on the requested type, one depending on the actual unconverted type of the value.

yawkat added a commit that referenced this pull request May 26, 2023
This fixes the build broken by #9310. Basically:

https://github.com/micronaut-projects/micronaut-core/blob/94f458ac04b0180a989d0cf28984d5e3b5df592f/http-client/src/test/groovy/io/micronaut/http/client/filter/StaticClientFilterSpec.groovy#L24-L28

The resolution of micronaut.server.port used to request the value of test-port as String. With this patch, it requests the value as Object instead (`.getValue(Object.class)`). The caching in PropertySourcePropertyResolver depends on the requested type however, so the Object request (micronaut.server.port) and the String request (micronaut.http.services.a.url) are cached separately and have different computed ports. (Additionally Object request type is not cached at all, but this is not the core issue.)

This patch adds another cache *before* conversion that is independent of the requested type. This cache ensures that the value is not recomputed for different request types.
@yawkat yawkat mentioned this pull request May 26, 2023
graemerocher pushed a commit that referenced this pull request May 26, 2023
This fixes the build broken by #9310. Basically:

https://github.com/micronaut-projects/micronaut-core/blob/94f458ac04b0180a989d0cf28984d5e3b5df592f/http-client/src/test/groovy/io/micronaut/http/client/filter/StaticClientFilterSpec.groovy#L24-L28

The resolution of micronaut.server.port used to request the value of test-port as String. With this patch, it requests the value as Object instead (`.getValue(Object.class)`). The caching in PropertySourcePropertyResolver depends on the requested type however, so the Object request (micronaut.server.port) and the String request (micronaut.http.services.a.url) are cached separately and have different computed ports. (Additionally Object request type is not cached at all, but this is not the core issue.)

This patch adds another cache *before* conversion that is independent of the requested type. This cache ensures that the value is not recomputed for different request types.
@dstepanov dstepanov added the type: improvement A minor improvement to an existing feature label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants