Reference only property declared in *.properties file in property expression

[angelozerr]

Reference only property declared in *.properties file in property expresion

Fixes #205

Signed-off-by: azerr azerr@redhat.com

[angelozerr]

Here a demo with referenced property coming from Java which is forbidden:

@fbricon what do you think about the error message : Reference the property 'quarkus.application.name' declared in Java is not allowed ?

[fbricon]

maybe "Cannot reference a property programmatically defined".

@maxandersen wdyt

[maxandersen]

Pinged @radcortez as he knows the "rules" better than I do.

[maxandersen]

i.e. i'm surprised if programmatically defined properties can't be referred to ...thats news to me (but I kinda understand why ... but just wonder how you refer to http.port in a url without having to override the default ?)

[radcortez]

i.e. i'm surprised if programmatically defined properties can't be referred to ...thats news to me (but I kinda understand why ... but just wonder how you refer to http.port in a url without having to override the default ?)

They can. They just need to be valid on runtime, when the expression is evaluated.

The message Reference the property [property] declared in Java is not allowed ? is not coming from Config. So, something else is doing that validation.

[maxandersen]

@radcortez yeah so what you are saying ( which is what I thought too ) is that what is described in #205 does not hold true in all cases ? (not sure if spec is more restrictive and we allow it or @angelozerr interpreted the spec too strict ?)

[radcortez]

Yes, that is the exception. Consider:

@ConfigProperty(name = "my.prop", defaultValue = "foo")
String foo;
@ConfigProperty(name = "my.prop", defaultValue = "bar")
String bar

In this case, the same property may be injected in two different injection points with two different default values. If the default value is used, then how do you expand ${my.prop}? Is it foo or bar? There is a long discussion here: eclipse/microprofile-config#475. Ideally, the behavior should be modified so that you can only have a single default value (and not a default value per injection point).

[angelozerr]

(not sure if spec is more restrictive and we allow it or @angelozerr interpreted the spec too strict ?)

@maxandersen according to the spec at https://download.eclipse.org/microprofile/microprofile-config-2.0/microprofile-config-spec-2.0.html#property-expressions it seems that we cannot reference property which comes from @ConfigProperty:

A default value defined via org.eclipse.microprofile.config.inject.ConfigProperty#defaultValue is not eligible to be expanded since multiple candidates may be available.

I tested this usecase in Quarkus application and when I try to reference a property coming from @ConfigProperty, the application cannot be started. It's the reason why I wanted to manage that,because I spend some time to understand why my Quarkus application could not be started.

@radcortez you mean that it's not a correct validation?

[angelozerr]

The message Reference the property [property] declared in Java is not allowed ? is not coming from Config. So, something else is doing that validation.

It's my error validation that I have implemented for the editor.

[angelozerr]

I tested this usecase in Quarkus application

I mean, I write a class with those following properties:

    @ConfigProperty(name="abcd")
    private String abcd;

    @ConfigProperty(name = "my.prop", defaultValue = "foo")
    String foo;
    @ConfigProperty(name = "my.prop", defaultValue = "bar")
    String bar;

And I try to configure abcd by referencing my.prop in microprofile-config.properties:

abcd=${my.prop}

when I start the Quarkus application, I have this error:

2021-10-18 13:59:46,479 INFO  [io.qua.arc.pro.BeanProcessor] (build-49) Found unrecommended usage of private members (use package-private instead) in application beans:
        - @Inject field org.acme.SomePage#abcd
Failed to load config value of type class java.lang.String for: abcd

and I cannot access to my web page.

[radcortez]

Correct, because there is no way right now to determine the value to use. Which value should ${my.prop} use? foo or bar?

[angelozerr]

Correct, because there is no way right now to determine the value to use.

Ok I understand, but when I write just:

    @ConfigProperty(name="abcd")
    private String abcd;

    @ConfigProperty(name = "my.prop", defaultValue = "bar")
    String bar;

I have the same error:

2021-10-18 13:59:46,479 INFO  [io.qua.arc.pro.BeanProcessor] (build-49) Found unrecommended usage of private members (use package-private instead) in application beans:
        - @Inject field org.acme.SomePage#abcd
Failed to load config value of type class java.lang.String for: abcd

Is it a Quarkus bug?

[radcortez]

It's the same case. Even if you only have one injection point there and you could use the value from it, the case is disallowed because it is unreliable. For instance, a dependency of your application might use the same config. Or maybe someone else, later on, adds an injection point to the same config name in another part of the code.

It doesn't make sense for the same configuration name to have two different values, but unfortunately, this is possible due to the way the defaultValue and injection works. This should change so it actually throws a validation error, but right now the specification allows it.

My recommendation is to just set any default values in the properties files. Are you experiencing any issue that requires you to set the default in the annotation directly?

[angelozerr]

My recommendation is to just set any default values in the properties files.

Indeed if I write

abcd=${my.prop:xxxx} it works.

So I think having validation for

abcd=${my.prop}

Reference the property [my.prop] declared in Java is not allowed is good now? Or perhaps

Cannot reference the property [my.prop] programmatically defined. is better?

@radcortez what do you think about that?

[angelozerr]

Are you experiencing any issue that requires you to set the default in the annotation directly?

No, I have just tested corner usecase to implement my validator.

[radcortez]

@radcortez what do you think about that?

I think the message needs to state that you cannot use default properties to expand other values. For instance, nothing stops you to create your own ConfigSource with programmatically defined values and in that case, it would work.

[angelozerr]

You are speaking about ConfigSource, but if I use ConfigProperty annotation, I have not found a case (with a defaultValue, without a defaultValue, etc) that I can write to reference the ConfigProperty/@name which works. It's the reason why I report this error.

Our tooling is able to scan ConfigProperty (and another annotation like ConfigProperties, etc) to collect properties and use it for validation, completion, etc, but we don't support custom ConfigSource.

According to my test, I cannot reference any MP or Quarkus properties coming from ConfigProperty, etc

If you see my demo #206 (comment) if I assign value for a property which comes from Java (Quarkus annotation) inside properties file, it works.

[angelozerr]

The spec says:

A default value defined via org.eclipse.microprofile.config.inject.ConfigProperty#defaultValue is not eligible to be expanded since multiple candidates may be available. but it's the same rule for @configroot for Quarkus. My impression is that when we try to use a config from an annotation (it's the properties that we collect), inside a property expression, it's forbidden.

[radcortez]

Yes, the same applies to @ConfigRoot, @ConfigMapping or @ConfigProperties because there is no restriction about a property name being present in two different roots, so the case where you can have different defaults is the same.

Feel free to provide the message that suits best :)

[angelozerr]

Many thanks @radcortez to take time to explain this behavior.

Feel free to provide the message that suits best :)

What do you think about :

A default value defined via annotation like ConfigProperty is not eligible to be expanded since multiple candidates may be available.

[radcortez]

Sounds good :)

[rgrunber]

I think this is fine to merge when you're ready.

[angelozerr]

All tests passed. I merge the PR. Thanks @rgrunber for your review!