Skip to content

Issue #361: Add Support for Resolving Property Values #391

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

Closed
wants to merge 2 commits into from
Closed

Issue #361: Add Support for Resolving Property Values #391

wants to merge 2 commits into from

Conversation

jghiloni
Copy link

Add Support for Resolving Property Values in @RequestMapping(value)

Creates extra methods in AnnotationMappingDiscoverer, ControllerLinkBuilder, and ControllerLinkBuilderFactory to take an Environment object that can be used resolve placeholders. Accepts null Environments to allow for backward compatibility.

@jhoeller
Copy link

Could use PropertyResolver instead of Environment, or even a plain StringValueResolver that could be adapted from a PropertyResolver (see PropertySourcesPlaceholderConfigurer's implementation).

@jghiloni
Copy link
Author

I like the idea, but I'm unclear as to where the StringValueResolver would get the values to use for substitution. Are you suggesting passing that in instead of an Environment, or somehow having the resolver as an instance variable?

@jhoeller
Copy link

Indeed, a StringValueResolver would have to be adapted from a PropertyResolver along the lines of PropertySourcesPlaceholderConfigurer, line 170+, so it's not really convenient to use for that purpose. Declaring PropertyResolver itself in such a signature might be the better choice, since Environment implements PropertyResolver and can therefore be passed in directly.

@jghiloni
Copy link
Author

That's a good point. I'll make the necessary changes to reflect that (Passing PropertyResolver instead of Environment)

@jghiloni
Copy link
Author

Any news on this?

@gregturn
Copy link
Contributor

@jghiloni Ollie has been on PTO since SpringOne so things may be slow in coming out right now.

@jghiloni
Copy link
Author

Ah, makes sense! No real rush, just noticed it was still open.

@odrotbohm
Copy link
Member

I don't like that now the ControllerLinkBuilder is now scattered with PropertyResolver parameters, something as it really breaks the abstraction (why would a client have to know about property resolution when calling those methods).

Maybe we can move the changes to the ControllerLinkBuilderFactory, which is an instance based alternative to the static ControllerLinkBuilder, which then can take an Environment as configuration setup to then use it during link resolution without polluting the parameter space?

@odrotbohm odrotbohm force-pushed the master branch 2 times, most recently from 4ebc1be to 266ad50 Compare July 25, 2016 18:32
gregturn pushed a commit that referenced this pull request Jun 15, 2017
@gregturn
Copy link
Contributor

Catching up on @olivergierke 's comments on this feature. I'll try to see how this now operational PR can be retooled to not percolate this change into so many APIs.

gregturn pushed a commit that referenced this pull request Jun 15, 2017
gregturn pushed a commit that referenced this pull request Jun 15, 2017
gregturn pushed a commit that referenced this pull request Jun 15, 2017
gregturn pushed a commit that referenced this pull request Jun 15, 2017
@gregturn
Copy link
Contributor

The related branch now shows a different solution that doesn't require altering any method signatures. It's also more composable.

gregturn pushed a commit that referenced this pull request Jun 16, 2017
gregturn pushed a commit that referenced this pull request Jul 31, 2017
gregturn pushed a commit that referenced this pull request Aug 28, 2017
gregturn pushed a commit that referenced this pull request Feb 6, 2019
gregturn pushed a commit that referenced this pull request Feb 6, 2019
gregturn pushed a commit that referenced this pull request Feb 6, 2019
gregturn pushed a commit that referenced this pull request Feb 6, 2019
@gregturn gregturn changed the base branch from master to main April 7, 2021 18:21
@odrotbohm odrotbohm force-pushed the main branch 3 times, most recently from 2e02d7a to 856b6b9 Compare November 16, 2022 17:53
@odrotbohm odrotbohm force-pushed the main branch 4 times, most recently from 5828e78 to e643c37 Compare November 16, 2023 10:46
@jghiloni jghiloni closed this by deleting the head repository Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants