Skip to content

Improve ways in which configuration property metadata default values can be provided #28039

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

Open
wilkinsona opened this issue Sep 16, 2021 · 3 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

The hope is that we could avoid problems like #28037. In that case there's a List<String> property with a default value created with new ArrayList<>("…"). The annotation processor can't extract the default value so we rely on an entry in the manual metadata for it. For such a property that doesn't have an entry in the manual metadata, it'd be good if the annotation processor could fail or generate a warning.

@wilkinsona wilkinsona added the type: enhancement A general enhancement label Sep 16, 2021
@wilkinsona wilkinsona added this to the 2.x milestone Sep 16, 2021
@onobc
Copy link
Contributor

onobc commented Sep 17, 2021

I too could have used this protection here.

@onobc
Copy link
Contributor

onobc commented Sep 17, 2021

I was sad when I dug in and discovered that the config annotation processor was not an actual magic lamp that created magic metadata for me but instead was just some code written by humans that can handle only a limited set of default value types. ;)

But all kidding aside, I am interested in working this one @wilkinsona if its up for grabs. I have not yet done anything in the way of the annotation processors yet (other than figure out how to step through debugger during compile step.

If so, I have some questions before moving forward. I see the following options:

### Option 1 - fail

If there is an initializer expression but the default turns out null(eg. new ArrayList<>("…")) then fail. This could be done directly in the FieldValuesParser implementation.

### Option 2 - warn
If there is an initializer expression but the default turns out null (eg. new ArrayList<>("…")) then log warning. We could just catch the the exception thrown from option 1 and handle it by log warning rather than letting it bubble up.

### Option 3 - ability to configure fail or warn
Variation of the above where the "warn lever" of option 2 is used (or not) based on config prop. I am not sure if this is desirable NOR how to set configuration properties for annotation processors.

Thoughts?

I dug deeper and the above options/questions sort of fell out as I learned more about how the processor works and where it logs warnings currently. I will tidy up what I have tomorrow and submit.

@wilkinsona
Copy link
Member Author

Having seen #28062, we're not sure that this is a good idea. We'd like the explore some alternative approaches such as:

  • improving the default value extraction in the annotation processor
  • using a new annotation that allows a default value to be specific on the configuration properties class rather than it being separated out into the addition metadata json file

@wilkinsona wilkinsona changed the title Fail or generate a warning when a property has a default value that the annotation processor cannot extract, and there's no additional metadata providing a default value Improve ways in which configuration property metadata default values can be provided Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants