Skip to content

Improve SpEL inline collection caching #25921

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

Improve SpEL inline collection caching #25921

wants to merge 2 commits into from

Conversation

SammyVimes
Copy link
Contributor

This is a fix for #22146

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 16, 2020
@sbrannen
Copy link
Member

For future reference, there was no need to close #25047.

If you need to make changes to a PR, you can push additional commits to the PR branch and rebase on the latest master if necessary.

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Oct 16, 2020
@sbrannen
Copy link
Member

@aclement, do you have time to review the proposal?

@aclement
Copy link
Contributor

It looks reasonable to me @sbrannen . The only concern I'd have is that we do allowable SpEL compilation of inline lists (not inline maps yet):

In InlineList:

	public boolean isCompilable() {
		return isConstant();
	}

So maybe a few tests verifying compiled expressions using inline lists are still OK with the constant computation extended to include OpMinus nodes. I think it'll be ok from browsing the code, but worth testing.

@SammyVimes
Copy link
Contributor Author

SammyVimes commented Nov 19, 2020 •
edited
Loading

@aclement I just added tests verifying compilation, looks like it's still OK

@SammyVimes SammyVimes mentioned this pull request Nov 22, 2020
Closed
@SammyVimes
Copy link
Contributor Author

Hello! I wonder if we can proceed with the review and merge this? :)

@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 30, 2023
@sdeleuze sdeleuze self-assigned this Aug 30, 2023
@sdeleuze sdeleuze added this to the 6.1.0-RC1 milestone Aug 30, 2023
@sdeleuze sdeleuze changed the title SPR-17614 Fix SpEL inline collections with negative keys or values are not cached Improve SpEL inline collection caching Sep 13, 2023
sdeleuze added a commit to sdeleuze/spring-framework that referenced this pull request Sep 13, 2023
@sdeleuze
Polishing
5d45439 
See spring-projectsgh-25921
@sdeleuze sdeleuze closed this in 6f95467 Sep 13, 2023
@sdeleuze
Copy link
Contributor

Rebased and merged, thanks for your contribution and sorry for the delay.

@sbrannen sbrannen self-assigned this Sep 13, 2023
sbrannen added a commit that referenced this pull request Sep 13, 2023
@sbrannen
Revise SpEL inline collection caching improvements
e7cf54d 
This commit revises the contribution for gh-25921 in the following ways.

- Use instanceof pattern matching

- Use List.of() and Map.of()

- Add missing @⁠since tags

- Polish Javadoc

- Rename isNegativeNumber() to isNegativeNumberLiteral()

- Restructure InlineCollectionTests using @⁠Nested, etc.

- Fix testListWithVariableNotCached() test: it previously set a SpEL
  "variable" but tested a "property" in the root context object, which
  effectively did not test anything.

- Introduce additional tests: listWithPropertyAccessIsNotCached(),
  mapWithVariableIsNotCached(), and mapWithPropertyAccessIsNotCached().
@sbrannen
Copy link
Member

Indeed, thanks for the PR, @SammyVimes! 👍

I unfortunately forgot about this PR and so only got around to reviewing it after it had been merged.

You can see the result of my belated review in e7cf54d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sbrannen sbrannen

@sdeleuze sdeleuze

Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Milestone
6.1.0-M5
Development

Successfully merging this pull request may close these issues.
5 participants
@SammyVimes @sbrannen @aclement @sdeleuze @spring-projects-issues