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

Fix MustacheEnvironmentCollector to not ignore native fetcher #21060

Closed
wants to merge 3 commits into from

Conversation

dsyer
Copy link
Member

@dsyer dsyer commented Apr 21, 2020

When the context is a map (as it is in a web View for instance) you can't
assume a non-null fetcher actually contains the property you are searching
for. This change alters the logic so that the native fetcher is always
consulted if it exists, but there is always a fallback.

Fixes gh-21045

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 21, 2020
When the context is a map (as it is in a web View for instance)
you can't assume a non-null fetcher actually contains the property
you are searching for. This change alters the logic so that the
native fetcher is always consulted if it exists, but there is
always a fallback.

Fixes spring-projectsgh-21045
@dsyer dsyer force-pushed the mustache-fetcher branch from d2de937 to b5f1bd4 Compare April 21, 2020 11:29
@dsyer
Copy link
Member Author

dsyer commented Apr 21, 2020

CI failures look unrelated to me?

@wilkinsona
Copy link
Member

CI failures look unrelated to me?

Confirmed. The latest Micrometer 1.5 snapshots have broken our build.

@dsyer
Copy link
Member Author

dsyer commented Apr 21, 2020

UPDATE: this patch is not complete. It fails to resolve _csrf.token in a basic Spring Security app. I'll try and fix that.

@wilkinsona wilkinsona marked this pull request as draft April 21, 2020 13:07
@dsyer dsyer marked this pull request as ready for review April 21, 2020 14:05
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 21, 2020
@philwebb philwebb added this to the 2.3.x milestone Apr 21, 2020
@philwebb
Copy link
Member

I'm tempted to just apply this to 2.3.x rather than the older branches. Any objections @dsyer?

@dsyer
Copy link
Member Author

dsyer commented Apr 21, 2020

No, I don’t think so.

@snicoll snicoll added the for: merge-with-amendments Needs some changes when we merge label May 8, 2020
@snicoll
Copy link
Member

snicoll commented May 8, 2020

There is an unrelated change in .gitignore that should be removed.

@dsyer
Copy link
Member Author

dsyer commented May 8, 2020

I left it as a separate commit so you can leave it out easily if you want.

@philwebb philwebb self-assigned this Jun 8, 2020
philwebb pushed a commit that referenced this pull request Jun 8, 2020
Alter the logic of `MustacheEnvironmentCollector` so that the
native fetcher is always consulted if it exists.

When the context is a map (as it is in a web View for instance) you
can't assume a non-null fetcher actually contains the property you are
searching for.

See gh-21060
philwebb added a commit that referenced this pull request Jun 8, 2020
@philwebb philwebb closed this in 6547ea5 Jun 8, 2020
@philwebb
Copy link
Member

philwebb commented Jun 8, 2020

Thanks @dsyer! I pulled the ignore commit out and applied it to 2.1.x onwards. The Mustache fix is just on 2.3.x+.

@philwebb philwebb modified the milestones: 2.3.x, 2.3.1 Jun 8, 2020
wilkinsona added a commit that referenced this pull request Aug 13, 2020
This reverts commit 6547ea5, reversing
changes made to e9e4a34.

Fixes gh-22039
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate environment fallback for Mustache variable resolution
5 participants