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

CDI @ConfigProperty primitive types should default to 0/false #476

Open
dmlloyd opened this issue Nov 19, 2019 · 2 comments
Open

CDI @ConfigProperty primitive types should default to 0/false #476

dmlloyd opened this issue Nov 19, 2019 · 2 comments
Labels
area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) incompatible change ⚠️ A change that introduces an incompatibility compared to previous releases; major versions only
Milestone

Comments

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 19, 2019

If a CDI @ConfigProperty is defined with a primitive type, and no default value is given for the property, it may be substantially less surprising to default the property to 0 or false (depending on the type of course) than it would be to make the property be required by default. If the property is required by default, the evidence suggests that most people will manually give a default value of 0 or false to match their intuitive understanding of how Java primitive fields are initialized.

It should still be possible to create a default value of "" for such properties, which would cause specification of the property in configuration to be mandatory.

It is worth discussing whether this behavior makes sense to extend to the corresponding primitive wrapper types as well. If a primitive wrapper type is desired, but the value is meant to be optional, then OptionalInt/OptionalLong/OptionalDouble/Optional<WrapperType> should normally be used instead, to prevent users from having to deal with null.

Note that this does not extend to properties of type char, for which a default value of '\0' might be confusing.

The final conclusion to the discussion should be codified in the specification.

@Emily-Jiang
Copy link
Member

We discussed this in the past. If you use
@Inject @ConfigProperty(name="blah") String aVar;
This means the property is mandatory. If there is no supplied value, you will see an exception.
For optional properties, you either use:
@Inject @ConfigProperty(name="blah", defaultValue="myDefault") String aVar or
@Inject @ConfigProperty(name="blah") Optional<String> aVar

@dmlloyd
Copy link
Contributor Author

dmlloyd commented Nov 22, 2019

Correct, however we know we're going to update our empty value handling, so it makes sense to establish these values as defaults rather than forcing the properties to be required. If you read carefully you'll see these cases are well covered.

@dmlloyd dmlloyd added this to the MP Config 2.0 milestone Jan 31, 2020
@Emily-Jiang Emily-Jiang added incompatible change ⚠️ A change that introduces an incompatibility compared to previous releases; major versions only empty labels Feb 13, 2020
@dmlloyd dmlloyd added area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) and removed empty labels Mar 4, 2020
@radcortez radcortez modified the milestone: MP Config 2.1 Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) incompatible change ⚠️ A change that introduces an incompatibility compared to previous releases; major versions only
Projects
None yet
Development

No branches or pull requests

3 participants