Skip to content

Add @ConfigurationPropertyValue annotation #8385

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add @ConfigurationPropertyValue annotation #8385

wants to merge 2 commits into from

Conversation

thombergs
Copy link
Contributor

@thombergs thombergs commented Feb 22, 2017 •
edited by philwebb
Loading

Adds the @ConfigurationPropertyValue annotation as proposed in issue #7986. Fixes issue #7986 if merged.

The annotation supports defining a fallback property to bind if the original property is not available and defining a default value which is bound if the fallback property also fails (or is not defined).

Changes are covered with unit tests and I added a short example of the annotation to the feature docs.

Looking forward to feedback :).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 22, 2017
@thombergs
Copy link
Contributor Author

...investigating the cause for the build failure...

@snicoll
Copy link
Member

snicoll commented Feb 24, 2017

Thanks for the PR. Unfortunately, we are currently rewriting the binding mechanism from scratch (almost) which would make the core of this PR outdated. I am not sure when that's going to land in master but surely everything else (including the test) should be a very good base.

WDYT @philwebb ?

@thombergs
Copy link
Contributor Author

I'll ask next time before starting on an issue :-). I'll gladly re-implement this feature on top of the new binding mechanism (if it's not included yet) if you give me a trigger here once it's merged.

@thombergs
Copy link
Contributor Author

Will using @ConfigurationProperties on an interface be possible with the new binding mechanism? I just wanted to add a unit test for that case, since the "defaultValue" feature implemented in this PR only really makes sense with an interface and I noticed it's not possible yet, as far as I can see.

@philwebb
Copy link
Member

@thombergs The plan is to support interface based binding. It's turning out to be be quite a chunk of work so it's not yet ready to merge into master.

@philwebb philwebb added the theme: config-data Issues related to the configuration theme label Apr 27, 2017
@snicoll
Copy link
Member

snicoll commented Aug 18, 2017

@thombergs I might give this a try next week with current master. As things stand, we'd be looking at the fallback property for now only (more to come later). Would you be willing to rework your PR with the new binding mechanism?

@thombergs
Copy link
Contributor Author

thombergs commented Aug 19, 2017 •
edited
Loading

@snicoll Sure, just give me a heads-up when you're done.

Just to be sure: only the fallback mechanism for now and leave the default value mechanism for later, correct?

@snicoll
Copy link
Member

snicoll commented Aug 19, 2017

Sure, just give me a heads-up when you're done.

Sorry that wasn't clear. If you don't have time to rework the PR, I'll give it a try as we'd like this feature to go in. So please let me know if you have time to work on that short term.

only the fallback mechanism for now and leave the default value mechanism for later, correct?

Yes (we don't have interface binding yet so that's not necessary for now)

@thombergs
Copy link
Contributor Author

I misunderstood. I thought you meant you were going to do the binding mechanism rework, but that obviously already happened.

I will look into it starting tonight and will probably need a couple nights to grasp the new binding concepts and re-integrate the fallback mechanism. I will submit a new PR then.

@philwebb
Copy link
Member

@thombergs Thanks. If you get anywhere could you please do a force push to this branch, rather than opening a new PR.

@thombergs
Copy link
Contributor Author

Will do.

@thombergs
Copy link
Contributor Author

thombergs commented Aug 20, 2017 •
edited
Loading

I force pushed a re-implementation on top of the new binding mechanism.

The @ConfigurationPropertyValue annotation and the fallback feature are implemented in commit 48eecdc.

For a fallback to function over different property namespaces, I had to remove a check that skipped binding when no property was found in a namespace (see commit 804955f). I couldn't see any side effects, but you probably should look into it.

Implementing this on top the refactored binding mechanism was a lot more straight forward than on the old version, so the refactoring definitely pays off ;).

@thombergs
Copy link
Contributor Author

updated this pull request to work with current master

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 29, 2017
@thombergs
Added ConfigurationPropertyValue annotation and its processing.
857cdf1 
Added onNull() to BindHandler to be able to react to properties that
bind to null to choose a fallback value instead.

ConfigurationPropertyValue annotations (which allow defining a fallback
value for a property) are now collected by
ConfigurationPropertyValueReader and a map of fallback properties is
passed into a FallbackBindHandler. The handler then reacts on a null
value by providing the fallback value instead of the null value.

Removed checking for descendant properties, because otherwise fallback
properties must always be from the same namespace.

Fixes spring-projects#7986
@thombergs
Copy link
Contributor Author

thombergs commented Dec 16, 2017 •
edited
Loading

Updated this PR again on top of current master and squashed it to a single commit.

I added a rather ugly instanceOf FallbackHandler in Binder lacking a better idea. This should probably be discussed.

@philwebb
Copy link
Member

Thanks for the update. I think we'll need to review this as part of the wider push to support interface based configuration properties. Unfortunately, that's unlikely to happen until Boot 2.1 now :(

@philwebb
Copy link
Member

We've been discussing configuration properties again today as a team and we're starting to feel like the supporting fallback options might not be the best idea in the long run. It feels like a feature that could persuade users to create overly complex property structures.

We've decided that for now, we're better off suggesting that people implement fallback scenarios directly in their code.

Thanks very much for your efforts with this, and I'm sorry that we didn't manage to solidify our thoughts sooner.

@philwebb philwebb closed this May 10, 2019
@philwebb philwebb removed theme: config-data Issues related to the configuration theme type: enhancement A general enhancement labels May 10, 2019
@philwebb philwebb added the status: declined A suggestion or change that we don't feel we should currently apply label May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thombergs @snicoll @philwebb @spring-projects-issues