Skip to content

JdbcOperationsSessionRepository doesn't resolve delta to correct SQL statement #1070

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 4 commits into from

Conversation

candrews
Copy link
Contributor

When using JDBC, setting a new attribute multiple times results in an SQL UPDATE not an INSERT. Because there is no row to update, the session attribute is not actually saved to the table.

For example, in a request handler (a Spring MVC controller, servlet, filter, etc):

Assert.isTrue(request.getSession().getAttribute("test")==null);
request.getSession().setAttribute("test", "someValue");
request.getSession().setAttribute("test", "someValue");

I would expect a row to be added to the SPRING_SESSION_ATTRIBUTES table with the ATTRIBUTE_NAME of "test" and the ATTRIBUTE_VALUE of "someValue"

However, what actually happens is that an UPDATE statement is executed resulted in no row being added to SPRING_SESSION_ATTRIBUTES.

The problem appears to be at https://github.com/spring-projects/spring-session/blob/2.0.3.RELEASE/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java#L738 - I believe the line should be:

else if (this.delta.get(attributeName) != DeltaValue.ADDED && this.delegate.getAttribute(attributeName) != null) {

@vpavic
Copy link
Contributor

vpavic commented May 10, 2018

Thanks for the PR @candrews - could you please add a test that reproduces the problem i.e. fails before the change you're proposing?

@candrews candrews force-pushed the patch-1 branch 2 times, most recently from 3df124a to c33aef5 Compare May 10, 2018 19:42
If an attribute is added multiple times, it's delta value should still be ADDED - not UPDATED.
@candrews
Copy link
Contributor Author

@vpavic I've added a test that passes with this change and fails without it.

@candrews
Copy link
Contributor Author

This issue is big problem for me; is there anything I can do to help get it merged so it appears in the next snapshot?

@vpavic
Copy link
Contributor

vpavic commented May 11, 2018

Thanks for the update @candrews - I'll try to get a closer look at this soon.

@candrews
Copy link
Contributor Author

I found a few additional, imho related, issues:

  • If an attribute exists, then is remove and re-added, that results in an INSERT. This ends up with an exception (key constraint violation). The test for this that I've added is called saveUpdatedRemoveThenModifySingleAttribute
  • If an attribute is added then removed, that should not result in any query being executed. Currently that sequence results in a DELETE. The test I've added for this situation is saveUpdatedAddThenRemoveSingleAttribute
  • Finally, I added saveUpdatedModifyThenRemoveSingleAttribute - it just seemed like a good test to have :)

I've added commits to this PR with tests and fixes for those issues.

@vpavic
Copy link
Contributor

vpavic commented May 14, 2018

This was merged to master in 60581c6 and polished in b634873. Thanks again @candrews! Snapshot should we available in half an hour tops, I'd appreciate if you could give it a spin.

@candrews
Copy link
Contributor Author

I've been testing it with my use case and so far so good - thank you for efforts, I greatly appreciate it 👍

@vpavic vpavic changed the title Don't set delta to updated if already set to added JdbcOperationsSessionRepository doesn't resolve delta to correct SQL statement May 15, 2018
@blacelle
Copy link

I encoutered the same kind of issue while using Spring OAuth2, to persist the authorizationRequest attribute. It was written twice (first in HandlerAdapter.handle, second in DispatcherServlet.processDispatchResult). The issue is also fixed on my side.

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

Successfully merging this pull request may close these issues.

3 participants