Ensure compatibility with Spring Session module split

[vpavic]

This PR updates Spring Session auto-configuration to ensure compatibility with extraction of SessionRepository implementations into separate Spring Session modules.

See spring-projects/spring-session#806.

[wilkinsona]

Thanks, @vpavic. One thing I don't like about this is the need for our own FlushMode enumeration. However, it rather looks like it can't be avoided. I wonder if we should reconsider the structure of SessionProperties. WDYT, @snicoll?

[snicoll]

Ironically enough, I was about to write the exact same thing. If we look at Spring Data, each store has its own auto-configuration and its own properties object. I can help but think this looks a bit nuclear given the state of the Spring Session project.

Having said that, spring.session.store-type is the only top-level property and I'd like to keep it that way. Perhaps we can remove SessionProperties altogether and create store specific properties object, e.g. something annotated @ConfigurationProperties("spring.session.hazelcast"). That way we can enable the processing of such object when we know that Hazelcast is on the classpath.

That makes each store with a configuration class and a configuration properties class. I think I prefer this option than pretending they are all managed together.

StoreType is less of a concern as it would represent the stores that we officially support. It's quite similar to CacheType in the end.

@vpavic what do you think? If you agree, would you like to get going and rework your PR in that spirit?

[vpavic]

@wilkinsona, @snicoll, thanks for the feedback.

Before putting this PR together I also evaluated the separate @ConfigurationProperties approach. However since @ConfigurationProperties are generally paired with auto-configuration class, and session store specific Spring Session @Configuration classes are not auto-configurations, I went with smallest change that fits the requirement to initiate the discussion and see where that gets us.

So just to clarify, you'd like to remove the existing SessionProperties in favor of separate store specific @ConfigurationProperties while keeping the single auto-configuration class in SessionAutoConfiguration? That looks good and clean to me, especially considering the #9552.

My only concern is how to handel spring.session.store-type in that case - does that mean we would replace the SessionProperties#getStoreType usage with something like PropertyResolver#getProperty?

[snicoll]

So just to clarify, you'd like to remove the existing SessionProperties in favor of separate store specific @ConfigurationProperties while keeping the single auto-configuration class in SessionAutoConfiguration?

Yes. Each @Configuration for each store can add an @EnableConfigurationProperties(XYZ.class) to enable the processing of the store specific config.

My only concern is how to handel spring.session.store-type in that case

What about it? Rather than injecting the properties object, you can inject the environment and resolve the enum value. And then add manual metadata for that entry. It's a bit more work obviously but not that much.

[vpavic]

OK, not a problem, I just wanted to double check if I understood all the bits correctly before getting to work. Thanks for the feedback @snicoll, will update the PR soon.

[vpavic]

@snicoll I've updated the PR with separate @ConfigurationProperties for each store.

Note that I also changed the existing approach, where ServerProperties was injected into SessionProperties - IMO that made sense while there was a single SessionProperties in order to avoid injecting ServerProperties in all store @Configuration classes but with the split properties objects we're injecting it in multiple places anyway. This way I also avoided creating HashMapSessionProperties since the only property HashMapSessionConfiguration depends on is server.session.timeout.

Let me know what you think.

In the meanwhile, @rwinch has resolved spring-projects/spring-session#806.

[rwinch]

Note that our build failed due to Spring IO Checks failing. We will need spring-attic/platform#622 to be resolved before the jars are published

[wilkinsona]

Here's the changes to move us onto Spring Session snapshots: https://github.com/wilkinsona/spring-boot/tree/session-snapshots. I haven't pushed it as it should probably go in at the same time as the changes in this PR.

[snicoll]

Thanks all. @wilkinsona I've cherry-picked that commit

@vpavic looking at your PR it looks like I've mislead you a bit. After having looked at the actual code change, I think keeping SessionProperties is the right call, especially if we have to add another shared property in the future. Sharing that timeout handling in each store is too much work and the change is more involved than it should be. Hopefully my polish commit is more in line with what I expected in the end. Thanks again for the heads up on this!

[vpavic]

Yes, the change of removing SessionProperties got quite involved though I like one aspect of it - the dependency of each store @Configuration class on ServerProperties was more visible and expressive.

Anyhow thanks for addressing this @snicoll!