Improve ConfigProperty lookup

[Emily-Jiang]

Description

As a:

• Application user/user of the configuration itself
• API user (application developer)
• SPI user (container or runtime developer)
• Specification implementer

...I need to be able to:

use @Inject @ConfigProperty("property.name") instead of having to put a name attribute there
@Inject @ConfigProperty(name="property.name")

...which enables me to:
simply my lookup.

[Emily-Jiang]

This can be achieved by adding value attribute on ConfigProperty annotation, which will be doing what name attribute does. If someone happens use both attributes and the supplied the string is different, DeploymentException will be thrown.
For an instance,
@Inject @ConfigProperty(name="customer.name", value="Bob") String name;
The above should cause the exception DeploymentException to be thrown.

[radcortez]

The initial API was using value but it was later changed to name here: #99.

I can't find any reason for why it was changed. Do you remember?

[Emily-Jiang]

The initial API was using value but it was later changed to name here: #99.

I can't find any reason for why it was changed. Do you remember?

@radcortez thanks for digging further! Originally I proposed to have the value attribute. However, after introducing defaultValue attribute, it will be confusing to have something:
@Inject @ConfigProperty(value="customer.name", defaultValue="Bob") String cName;
At that time, I did not think about having both name and value. Hence, this issue is raised to add that.

With both name and value, end users can use value whendefultValue is absent.
@Inject @ConfigProperty("customer.name")
when defaultValue is present, they can continue using:
@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String cName;

Basically, the attribute name and value are identical. However, if someone uses both with a different content, an exception should be thrown.

[ljnelson]

I'd like to make sure I understand this absolutely clearly.

You are proposing that:

1. three elements will exist on the ConfigProperty annotation: name, value and defaultValue
2. All will be declared as not required, so that an annotation of, simply, @ConfigProperty is at least syntactically legal (if not semantically legal)
3. name and value will represent exactly the same thing even though they designate completely different concepts
4. All usages of this new ConfigProperty annotation must be scanned and validated to ensure that incompatible values are not specified together, and that at least one of either name or value is specified

Does that correctly represent what you are proposing?

This will almost certainly be immensely confusing to end users and consequently I object to it.

Perhaps a replacement annotation is called for instead (perhaps without default-value semantics as well, given how haphazardly #446 (and #531 and #532 and #533) was addressed, and how thorny default values are in general, and how difficult they are, therefore, to encode in the primary qualifier annotation, as many issues have borne out)?

Simpler, clearer replacement possibilities might include:

• ConfigKey("foo")
• ConfigKeyNamed("foo")
• Value("foo")
• ValueNamed("foo")
• Setting("foo")
• SettingNamed("foo")

[Emily-Jiang]

@ljnelson I really don't understand what you are objecting about. This issue neither fix or make the existing behaviours you listed above any worse though as I am not touching any defaultValue attribute. I have not seen any end users reporting an issue with defaultValue either. By the way, I don't see introducing new annotations any help.

[ljnelson]

I believe that using name and value to mean the same thing is confusing, since these words mean entirely different things.

See #671 for one issue about defaultValue.

[radcortez]

In SR Config we added additional annotations (they work only for the mapping classes), but maybe they can serve as an inspirations: @WithName, @WithDefault, @WithConverter, etc.

[NottyCode]

@Emily-Jiang I know this came from a discussion we had, but one downside of this is to make this work we would have to mark both value and name as optional. That would means someone could use the annotation with no attributes which would be wrong. However I have confirmed if there is an attribute of value then you can indeed do @ConfigProperty("my.config.prop") which leads to the simplification in the case of no default value.

[rdebusscher]

Is this 'simplification' worth the possible confusion about the meaning of all attributes?

[tomas-langer]

I think that a better solution would be to introduce a new annotation and deprecate the old one.
Such as @ConfigValue("my.config.prop") or @Value("my.config.prop")

This way there is no need to handle multiple optional annotation attributes - each annotation has a mandatory attribute.

I also think that default value should not be part of the annotation - we could do similar what JAX-RS is doing:

@Inject
public MyClass(@Value("my.config.prop") @DefaultValue("default") String value) {
...
}

[Emily-Jiang]

I don't see much value to introduce a brandnew annotation in order to achieve a fairly minimal improvement. This leads end users further learning new things. I would rather stay with ConfigProperty(name="something"). Anyway, it is just a few characters and IDE can help with it.

[m0mus]

I agree with @tomas-langer and @rdebusscher. Having 2 attributes meaning the same thing is confusing and it's not a simplification. I am voting for deprecation of @ConfigProperty with name and defaultValue attributes in favour of @ConfigValue and @DefaultValue. It's more clean and aligned with what JAX-RS folks are doing.

[NottyCode]

I can understand the perspective that says having name and value meaning the same thing would be confusing and would be more confusing than having people write name. However I think deprecating ConfigProperty at this point would be a mistake. ConfigProperty is the primary API for MP Config, it has persisted through multiple years of revision and throwing it away now would be very disruptive and I think it would alienate people who have adopted MicroProfile for little benefit.

If I saw @ConfigValue as an annotation I would expect that it provides a config value, not that it consumes one based just on the name. I would be very confused at using a @DefaultValue from Jakarta REST and if MP Config duplicated the name you'll have the development time headache that you'll likely see people selecting the wrong one when doing type auto-import in tools. Also if you have two annotations you have to get into the question of what happens if they are not used in combination, whereas at least for attributes on a single annotation there is conceptual grouping via the @ConfigProperty annotation.

[m0mus]

I share the concern of logical combination. @DefaultValue doesn't make sense if @ConfigValue is not present.

If a Java property is annotated with @ConfigProperty without either name or value specified, the actual configuration property name can be constructed basing on the Java property name using some rules defined by the spec.

Example:

@ConfigProperty
Integer foo; // Read from "foo" configuration property

@ConfigProperty
Integer fooBar; // Read from "foo.bar" configuration property

@ConfigProperty(defaultValue="5")
Integer fooBarBaz; // Read from "foo.bar.baz" configuration property with default value of 5

[Emily-Jiang]

For ConfigProperty, if the name is not present, the property name resolves to the fully qualified classname and then variable name. See here. The reason for the name default is to avoid the name clashes. However, I do feel it is not very user friendly. We could improve the name default.

[Emily-Jiang]

After this discussion, I think we have two viable options to improve the current ConfigProperty lookup. I will list them here. Please click thumb up for your preferred option.

[Emily-Jiang]

Option 1 : add value attribute on the ConfigProperty.

@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String name; 
@Inject @ConfigProperty("customer.name") String name;

[Emily-Jiang]

Option 2: update the default value name if name is not present. The name will be the property name. If the property name contains uppercase letter, the letter will be transformed to dot and lower case.

@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String name; 
@Inject @ConfigProperty String customerName; //read from the property customer.name

//more examples
@ConfigProperty
Integer foo; // Read from "foo" configuration property

@ConfigProperty
Integer fooBar; // Read from "foo.bar" configuration property

@ConfigProperty(defaultValue="5")
Integer fooBarBaz; // Read from "foo.bar.baz" configuration property with default value of 5

[m0mus]

@Emily-Jiang, I am sorry to say that, but the options you provided are both wrong. In the first option value attrbute must be replaced with defaultValue. The second option is a complete mismatch. The correct options (IMO) are below:

Option 1:

Introduce value attribute to @ConfigProperty

// Initializes name variable from customer.name config property without default value
@Inject @ConfigProperty("customer.name") String name;

// Initializes name variable from customer.name config property with default value
@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String name; 

// The same as above
@Inject @ConfigProperty(value="customer.name", defaultValue="Bob") String name; 

Option 2:

Introduce 2 additional annotations @ConfigValue (name is still not concrete) and @DefaultValue.

// Initializes fooBar variable from foo.bar config property without default value
@ConfigValue("foo.bar")
Integer fooBar;

// Initializes fooBar variable from foo.bar config property with default value 42
@ConfigValue("foo.bar")
@DefaultValue(42);
Integer fooBar;

[Emily-Jiang]

@Emily-Jiang, I am sorry to say that, but the options you provided are both wrong. In the first option value attrbute must be replaced with defaultValue. The second option is a complete mismatch. The correct options (IMO) are below:

@m0mus not really. The value should not be replaced by defaultValue as the attribute value is for property name specification.

Option 1:

Introduce value attribute to @ConfigProperty

// Initializes name variable from customer.name config property without default value
@Inject @ConfigProperty("customer.name") String name;

// Initializes name variable from customer.name config property with default value
@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String name; 

// The same as above
@Inject @ConfigProperty(value="customer.name", defaultValue="Bob") String name; 

This should work, but I don't think it should be promoted as specifying name is much more readable.

Option 2:

Introduce 2 additional annotations `@ConfigValue` (name is still not concrete) and `@DefaultValue`.

ConfigValue is an interface already in MP Config. I don't think we want use this. Besides, from your last comments it is confusing to have two annotations and one depends on the presence of the other. I lifted your suggestion from that comment for this option 2.

// Initializes fooBar variable from foo.bar config property without default value
@ConfigValue("foo.bar")
Integer fooBar;

// Initializes fooBar variable from foo.bar config property with default value 42
@ConfigValue("foo.bar")
@DefaultValue(42);
Integer fooBar;

[m0mus]

@m0mus not really. The value should not be replaced by defaultValue as the attribute value is for property name specification.

I was talking about your sample code. On the first like of your code value attribute holds a default value. In the second line value (hidden) specified the config property name.

So, this code (your option one)

@Inject @ConfigProperty(name="customer.name", value="Bob") String name; 
@Inject @ConfigProperty("customer.name") String name;

must be replaced with this one:

@ConfigProperty(name="customer.name", defaultValue="Bob") String name; 
@Inject @ConfigProperty("customer.name") String name;

The annotation name in option 2 is not agreed. I actually said that in my previous comment. We may come up with a different name. Important is the concept of 2 annotations.

[tomas-langer]

@Emily-Jiang - in both options you propose, you use:

@Inject @ConfigProperty(name="customer.name", value="Bob") String name; 

This is very confusing, as you use value once for default value, and once for property name. You still have defaultValue option on the ConfigProperty.

This is confusing and neither of the options is good

[Emily-Jiang]

@Emily-Jiang - in both options you propose, you use:

@Inject @ConfigProperty(name="customer.name", value="Bob") String name; 

This is very confusing, as you use value once for default value, and once for property name. You still have defaultValue option on the ConfigProperty.

This is confusing and neither of the options is good

ah. Sorry for the confusion. It was a typo. I meant
@Inject @ConfigProperty(name="customer.name", defaultValue="Bob") String name;
As mentioned in the very first comment, if someone specifies both name and value, this should resolve to an error. The value attribute in the annotations has special meanings. Putting value in this annotation means the name of the property might not be a good fit, which is even true if this property has no other attributes.
I appreciate the suggestions etc. After this conversation, I think the current usage is the best approach among others and less confusion. As an end user, I'm happy to type in name= to make my code readable. Besides, IDE will help me anyway. With this, I'm going to close my issue.

[m0mus]

I appreciate the suggestions etc. After this conversation, I think the current usage is the best approach among others and less confusion.

@Emily-Jiang, I understand that you are prefer the first option, but I don't see that the consensus is reached. There are other opinions. I would appreciate if you fix your voting options and users actually vote. Please reopen this issue.

[Emily-Jiang]

I appreciate the suggestions etc. After this conversation, I think the current usage is the best approach among others and less confusion.

@Emily-Jiang, I understand that you are prefer the first option, but I don't see that the consensus is reached. There are other opinions. I would appreciate if you fix your voting options and users actually vote. Please reopen this issue.

@m0mus this issue is about improving @ConfigProperty not removing it because as an end uers I am happy with this annotation as it is and try to explore whether it can be further improved. If the option you talked about is to deprecate this annotation and go with alternative annotations, I don't think it fits with this issue though. If you want to focus on the two options I highlighted earlier, we can continue the conversation.

[Emily-Jiang]

As mentoned earlier, repopen this issue for further conversation to see whether we can reach any consensus.