quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.58k stars 2.63k forks source link

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

Open gsmet opened 4 weeks ago

gsmet commented 4 weeks ago

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.

quarkus-bot[bot] commented 4 weeks ago

/cc @radcortez (config)

gsmet commented 3 weeks ago

@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 commented 3 weeks ago

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.

gsmet commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/smallrye/smallrye-config/issues/1077.

radcortez commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.