JdbcOperationsSessionRepository: deserialize lazily

[candrews]

Instead of deserializing all of the session attributes as they are read from the database, deserialize as getAttribute(String) requests them.
For applications which store a lot of data in the session but only use some of it for each requests, this change results in a lot less time being spent doing deserialization of objects that won't be used.
For other applications, no harm is done.

[candrews]

The same solution could be applied to RedisOperationsSessionRepository too (I can do that if this approach is deemed acceptable)

[candrews]

@vpavic what do you think?

[vpavic]

Thanks for the PR @candrews, we'll review and consider this for 2.1.x.

[vpavic]

This is something we'd like to get merged in one of the 2.1.0 milestones. I've left one comment on what I believe could be simplified.

Other than that, could you please:

• rebase on the current master
• provide some unit tests that verified the new behavior

[vpavic]

Is this really necessary? I think we could set attribute here as is, define our own Supplier and use it in SessionResultSetExtractor. JdbcSession#getAttribute would then test if the attribute is instance of the supplier we defined and if yes execute #get on it. That would reduce the impact and simplify things.

[candrews]

I initially thought of that approach as well, but decided not to go with it. My reasons are:

• I believe instanceof checking is generally discouraged in favor of polymorphism (which is what I ended up doing)
• That would prohibit the storage of instances of that special class (as doing so would result in unintended behavior)

Would you still like me to go ahead with that approach?

[candrews]

rebase on the current master

✔️

provide some unit tests that verified the new behavior

I'm having a bit of trouble with that. SessionResultSetExtractor seems like it would be the part to test (it's currently not executed by any of the unit tests). However, it's a private class so it can't be accessed directly, and jdbcOperations.query is always mocked (running the actual thing would result in a real database connection being made, which is not unit-testy and wouldn't work). Do you have any suggestions on what you'd like to see?

[candrews]

Also, it occurs to me that the existing integration tests (which pass) show that this change doesn't break anything.

[vpavic]

Thanks for the update @candrews.

I'm having a bit of trouble with that.

You could look into supplying a custom ConversionService and then verify its invocations. Does that make sense?

Also, it occurs to me that the existing integration tests (which pass) show that this change doesn't break anything.

We are introducing a new behavior here, and we should verify that in our tests.

[candrews]

You could look into supplying a custom ConversionService and then verify its invocations. Does that make sense?

That was my first thought, too, but I can't figure out a way to make that happen.

conversionService is used in org.springframework.session.jdbc.JdbcOperationsSessionRepository.deserialize(byte[]), which is called from org.springframework.session.jdbc.JdbcOperationsSessionRepository.SessionResultSetExtractor.extractData(ResultSet) - and as far as I can tell, there's no way to get SessionResultSetExtractor.extractData(ResultSet) to execute (see #1133 (comment)).

[vpavic]

This is now merged to master in c523fb5 and polished in b61937d.
Thanks for the PR @candrews - please review the polish commit and the approach taken to verify the new behavior.