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

Simple solution to provide the environment. #1411

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lyca
Copy link
Contributor

@lyca lyca commented Dec 10, 2020

- will work in most Spring Boot applications
- too simple, if nested contexts are used
@gregturn
Copy link
Contributor

I'm not sure I see the benefit of moving to this bean-based approach of getting the Environment compared to simply pulling it out of the WebApplicationContext if needed.

@lyca
Copy link
Contributor Author

lyca commented Mar 15, 2021

I currently do not see any method, that simply returns the WebApplicationContext for this to work. The current implementation does not work in Spring Boot as pointed out on SO over here https://stackoverflow.com/questions/22167912/contextloader-getcurrentwebapplicationcontext-always-returns-null and in issue #1406 by @adriendengreville

@gregturn gregturn changed the base branch from master to main April 7, 2021 18:20
@ShishkinDmitriy
Copy link

Hi guys, when it will be merged?
Faced exactly the same problem in spring boot.

@romanpierson
Copy link

Hi, is this ever to be merged? From reading #1328 I understood it was solved 2 years ago by @lyca and @odrotbohm stated its merged but for me its not working and can also not see those code changes in the latest source (using 2.6.9). Thanks for the clarification!

@odrotbohm
Copy link
Member

They don't need to be in there, as the fix for #1328 is actually a better solution than what's been suggested here. Would you mind creating a ticket and leave a few details of what exactly doesn't work, ideally with an accompanying reproducer.

@romanpierson
Copy link

Sure I created a small reproducer - thanks for reaching out and let me know if that's not the intended way of how this should be used and work.

https://github.com/romanpierson/spring-hateoas-reproducer

@ckdrunix
Copy link

ckdrunix commented Nov 9, 2023

They don't need to be in there, as the fix for #1328 is actually a better solution than what's been suggested here. Would you mind creating a ticket and leave a few details of what exactly doesn't work, ideally with an accompanying reproducer.

As lyca pointed out #1328 did not resolve the issue for spring boot apps, since ContextLoader.getCurrentWebApplicationContext() is null in this case. Therefore PropertyResolvingMappingDiscovery.resolveProperties does not resolve the properties contained in the mapping (verified on spring boot 3.1.5, spring-hateoas 2.1.2). Therefore I think this PR is justified.

@bcnapelinckx
Copy link

They don't need to be in there, as the fix for #1328 is actually a better solution than what's been suggested here. Would you mind creating a ticket and leave a few details of what exactly doesn't work, ideally with an accompanying reproducer.

As lyca pointed out #1328 did not resolve the issue for spring boot apps, since ContextLoader.getCurrentWebApplicationContext() is null in this case. Therefore PropertyResolvingMappingDiscovery.resolveProperties does not resolve the properties contained in the mapping (verified on spring boot 3.1.5, spring-hateoas 2.1.2). Therefore I think this PR is justified.

Completely correct... When will this fix be merged?

@romanpierson
Copy link

Not sure what's the point of requesting a reproducer to be honest .......

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to resolve place holders in @RequestMapping
7 participants