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

Add an annotation to better handle deprecation of configuration properties #42735

Open
gsmet opened this issue Aug 23, 2024 · 9 comments
Open
Labels
area/config kind/enhancement New feature or request

Comments

@gsmet
Copy link
Member

gsmet commented Aug 23, 2024

As we make progress on providing better quality information about our config to IDEs, there seems to be a consensus that we should follow the Spring format for configuration metadata (with some addition from us).

One thing they are handling is the deprecation of configuration properties with the ability to define a reason and a replacement with an annotation: https://docs.spring.io/spring-boot/docs/2.6.14/api/org/springframework/boot/context/properties/DeprecatedConfigurationProperty.html .

I think we should have something similar to better structure the info that might be in the @deprecated tag of the Javadoc.

Copy link

quarkus-bot bot commented Aug 23, 2024

/cc @radcortez (config)

@gsmet
Copy link
Member Author

gsmet commented Aug 26, 2024

@radcortez I wonder if this annotation should live in SmallRye Config instead? The reason I'm asking is that we could use the annotation to improve the deprecated error message and point to the replacement?

@radcortez
Copy link
Member

If we want to add an annotation, let's add it to SR Config.

My preference is always to avoid such annotations because @Deprecated confuses users about which annotation to use.

We shouldn't use @DeprecatedConfigProperty because it may further confuse users with @ConfigProperty, which is MP Config only for CDI Beans injection.

@radcortez radcortez added the kind/enhancement New feature or request label Aug 26, 2024
@gsmet
Copy link
Member Author

gsmet commented Aug 29, 2024

@radcortez yeah I totally agree, I gave it some thoughts and I was wondering if we could go with @WithReplacement(value = , reason = ).

That would tie the reason to having a replacement though.

BTW, while it doesn't appear to be that critical, I think it will be a bit in the way of my work for the IDEs so it would be nice if we could make progress on this.

BTW, I need the annotation to be kept at runtime.

@gsmet gsmet removed their assignment Aug 29, 2024
@gsmet
Copy link
Member Author

gsmet commented Aug 29, 2024

Also, I'm not sure how to deal with the replacement the best.

Because it might be hard to have an absolute path in some cases (let's say if the same group is used in different places, or the map case with an unnamed key is also relevant) but still in some cases you definitely will point to a totally different path.

I have no idea how they handled it in Spring.

@radcortez
Copy link
Member

Hum, I wonder if we should offer a @WithRelocate and @WithFallback instead. We could have a simple mapping, accept a function and, the reason for the relocate / fallback. This could be applied to members of the entire mapping class. In case of deprecation, you combine it with @Deprecated.

See smallrye/smallrye-config#1077.

@radcortez
Copy link
Member

I have no idea how they handled it in Spring.

I have a sample Spring app with some of their config stuff that I use to compare behaviour. I can try it and see what happens.

@gsmet
Copy link
Member Author

gsmet commented Aug 29, 2024

Yeah so what we need to be careful about is that we need it to work in the context of the annotation processor. Because I will have to execute this code to get the replacement path.

And from my perspective, we need to support moving a specific property. But we should also support moving a whole tree.

@radcortez
Copy link
Member

Regarding Spring, DeprecatedConfigurationProperty does not affect the runtime code:

This annotation has no bearing on the actual binding processes, but it is used by the spring-boot-configuration-processor to add deprecation meta-data.

I tried it anyway, and that is the case. There is not even a warning in the runtime code. Their scenario is more straightforward because they only support a simple String replacement, while we do support a programmatic rewrite of the names. While we could find a way to execute such code, the tricky part is that depending on the code location, you may not have the code compiled yet to be executed at the processor level. Maybe we should leave these cases out and just warn about the deprecation and point to the documentation, while in straightforward cases, we can provide more alternatives. We can consider something like:

@WithRelocate(String relocate, Function relocateFunction, reason)

Where relocate and relocateFunction are mutually exclusive.

And then:

@ConfigMapping(prefix = "old.prefix")
@WithRelocate("new.prefix")
interface Config {
  String foo();

  @WithRelocate("baz")
  String bar();
}

So the relocate member, will act in the context of the segment where being applied:

old.prefix.foo -> new.prefix.foo
old.prefix.bar -> new.prefix.bar

And the annotation processor could easily come up with the relocation paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/enhancement New feature or request
Projects
Development

No branches or pull requests

2 participants