Adapt to Spring Session Data Redis switching to an unindexed repository by default

[wilkinsona]

See spring-projects/spring-session#1711 for background.

Spring Session's Redis module has switched to a simpler default repository implementation. This broke Boot's build as the simpler repository does not require a cleanup cron. 4a401bf has adapted to the breaking change but means that Boot's still using an indexed repository by default.

We should probably align our default with Spring Session's and use the simpler, more performant repository by default. To do so, we'll need to do something with the spring.session.redis.cleanup-cron as it only applies to the indexed repository. We may also want to provide a configuration property to control the type of repository that's used, making it easy for users to switch back to the indexed repository if they wish to do so.

/cc @eleftherias @vpavic

[bclozel]

This change also broke a smoke test - the session actuator endpoint relies on the indexed variant of the repository. The new default doesn't allow that and the session endpoint is not auto-configured.

This test has been ignored with 21eab01, until a decision is made here.

[philwebb]

We're not going to add a property to switch the implementations. If users want the indexed version then they'll need to configure it themselves.

[bclozel]

@vpavic with SessionRepository being the new default, right now the SessionsEndpoint completely backs off as it's relying on FindByIndexNameSessionRepository for its features.

SessionsEndpoint ships three operations currently:

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/session/SessionsEndpoint.java

Lines 51 to 69 in 7f8e2d8

	@ReadOperation
	public SessionsReport sessionsForUsername(String username) {
	Map<String, ? extends Session> sessions = this.sessionRepository.findByPrincipalName(username);
	return new SessionsReport(sessions);
	}
	@ReadOperation
	public SessionDescriptor getSession(@Selector String sessionId) {
	Session session = this.sessionRepository.findById(sessionId);
	if (session == null) {
	return null;
	}
	return new SessionDescriptor(session);
	}
	@DeleteOperation
	public void deleteSession(@Selector String sessionId) {
	this.sessionRepository.deleteById(sessionId);
	}

We could provide a simple Endpoint variant with getSession and deleteSession, but we're wondering if removing the ability to list sessions for a username doesn't break the main use case for this. What do you think?

[eleftherias]

Adding that users may opt in to using a RedisIndexedSessionRepository by specifying @EnableRedisHttpSession(enableIndexingAndEvents = true).

I don't have background on SessionsEndpoint, but how does it work with other session stores that don't extend FindByIndexNameSessionRepository? I imagine that the new default implementation should work similar to the other stores, if they support such a feature.

[wilkinsona]

I've reinstated the smoke test in 061d86e. The app under test was using @EnableRedisHttpSession which was overriding Spring Boot's current opinion that the indexed repository should be used by default.

[wilkinsona]

Was anything decided about how to handle spring.session.redis.cleanup-cron which is specific to the indexed repository?

[wilkinsona]

We've changed our minds on this one and decided that we'd like to add a property to control enableIndexingAndEvents. It will be false by default, aligning with Spring Session's new default. This will allow the other Redis-related configuration properties to still be used when a user wants to switch indexing back on again. If they haven't switched on indexing and configure cleanup-cron, an exception should be thrown.

[vpavic]

Sorry for failing to address the mentions earlier, I missed this in a pile of unresolved notifications.

I'm not involved in Spring Session maintenance for some time now, so I might be out of the loop a bit - it caught me by surprise a bit to see this already impacting Spring Boot since I don't recall getting any notifications from spring-projects/spring-session#1711 and I see the issue is still open (update: I see the milestone issue was assigned to is closed so this must be an omission).

We could provide a simple Endpoint variant with getSession and deleteSession, but we're wondering if removing the ability to list sessions for a username doesn't break the main use case for this. What do you think?

@bclozel Irregardless of this issue, I quite like your proposal as it increases the usability of sessions endpoint. Admins might obtain the session ids by some other means (not involving Spring Session provided listing) so fetching session details and terminating the session are still valid and quite useful features. If you'd like to go in that direction, I can provide the PR.

We've changed our minds on this one and decided that we'd like to add a property to control enableIndexingAndEvents.

@wilkinsona I'd suggest not to go into that direction, at least not yet. I'm not a fan of enableIndexingAndEvents flag because it is a binary choice and as such is very fragile and will break the moment other Redis backed SessionRepository implementation(s) become available. But that's a discussion for Spring Session side of things so I'll try to go over this with @rwinch during this week.

Note that when I opened spring-projects/spring-session#1711, the idea was that configuration support would be refined in a way that it's easier for users to use a non-default SessionRepository implementation (regardless of it being Spring Session or even 3rd party provided) without losing all of auto-configuration support from Spring Boot.

[mbhave]

Thanks for the input, @vpavic. We discussed the option to provide getSession and deleteSession for non-indexed repositories on our last team call and decided we would leave things as they are for now and reconsider if we see demand for it.

Regarding enableIndexingAndEvents, we'll hold off on that for now until you've had a chance to discuss things with Rob.

[vpavic]

We discussed the option to provide getSession and deleteSession for non-indexed repositories on our last team call and decided we would leave things as they are for now and reconsider if we see demand for it.

The new Redis SessionRepository implementation is more performant thanks to significantly reduced number of operations against Redis with the primary tradeoff being lack of support for events. I'd recommend everyone to stick with this implementation (now also being default one) unless they really depend on support for session events. The tradeoff becomes larger if there's no Actuator support at all for the new repository so I'd naturally look to close that gap.

Note that not tying sessions endpoint on presence of indexed repository also opens up the ability to provide the endpoint in its basic form for the reactive stack, where there are no indexed repositories yet (see #10827).

[rwinch]

Regarding enableIndexingAndEvents, we'll hold off on that for now until you've had a chance to discuss things with Rob.

I'd avoid adding this property since we are leaning against doing this and it is very simple for a user to expose the indexed version as a Bean already.

In the long term it may be good to collaborate with the Data team to see if there is a better way to support looking up by attributes.

[wilkinsona]

Thanks, Rob.

since we are leaning against doing this

Does this mean that you're considering reverting the introduction of the attribute? FWIW, I think it's important that users can restore the old behavior easily if they wish to do so. By easily, I mean something that's a single property or a minimal amount of code.

it is very simple for a user to expose the indexed version as a Bean already

If they define the bean themselves, the spring.session and spring.session.redis properties won't work any more. Our opinion is that this makes things quite complex. It wouldn't be as easy as we would like to restore the previous behavior should a user wish to do so.

[rwinch]

Does this mean that you're considering reverting the introduction of the attribute?

Sorry this was not very clear. What I meant is that, I'd avoid adding the property to Boot because we are wanting to steer users toward the non-indexed versions due to the overhead problems.

We will need to revisit how to implement indexing in a way that scales better in a future release.

Our opinion is that this makes things quite complex. It wouldn't be as easy as we would like to restore the previous behavior should a user wish to do so.

Can you help me understand what specific properties not-being available would make it too complex?

[wilkinsona]

we are wanting to steer users toward the non-indexed versions due to the overhead problems.

IMO, changing the default already does that.

We will need to revisit how to implement indexing in a way that scales better in a future release

Until that's happened, I think it's important that users who need indexing and are happy with the trade-offs that it entails can easily re-enable it.

Can you help me understand what specific properties not-being available would make it too complex?

Focusing on Redis, when a user defines their own SessionRepository bean, the following properties will no longer work:

• spring.session.redis.flush-mode
• spring.session.redis.namespace
• spring.session.redis.save-mode
• spring.session.timeout

While these attributes are configurable via attributes on @EnableRedisHttpSession, they cannot be configured externally when using the annotation. Users who require the settings to be externally configurable would have to extend RedisIndexedHttpSessionConfiguration and implement their own mechanism.

In other words, a user who is using Redis with indexing and configuring the session timeout can do this today:

spring.session.timeout=60s

If we add a property to switch indexing back on, they can do this:

spring.session.timeout=60s
spring.session.redis.indexed=true

If we don't add the property, they'll have to do something like this:

@Configuration(proxyBeanMethods = false)
@EnableConfigurationProperties(MyRedisSessionProperties.class)
public class SessionRedisConfiguration extends RedisIndexedHttpSessionConfiguration {

	@Autowired
	void customize(MyRedisSessionProperties properties) {
		setMaxInactiveIntervalInSeconds((int) properties.getMaxInactiveInterval().getSeconds());
	}

	@ConfigurationProperties("my.redis.session")
	static class MyRedisSessionProperties {

		private Duration maxInactiveInterval;

		public Duration getMaxInactiveInterval() {
			return this.maxInactiveInterval;
		}

		public void setMaxInactiveInterval(Duration maxInactiveInterval) {
			this.maxInactiveInterval = maxInactiveInterval;
		}

	}

}

[rwinch]

I don't think many applications are actually needing externalized configuration for theses properties. If they do, I don't view this as a large amount of code.

Users can reuse Spring Boot's SessionProperties which reduces the necessary code even further:

@Configuration
public class SessionConfig extends RedisIndexedHttpSessionConfiguration {

	@Autowired
	public void customize(SessionProperties sessionProperties, RedisSessionProperties redisSessionProperties, ServerProperties serverProperties) {
		Duration timeout = sessionProperties.determineTimeout(() -> {
			return serverProperties.getServlet().getSession().getTimeout();
		});
		if (timeout != null) {
			this.setMaxInactiveIntervalInSeconds((int)timeout.getSeconds());
		}

		this.setRedisNamespace(redisSessionProperties.getNamespace());
		this.setFlushMode(redisSessionProperties.getFlushMode());
		this.setSaveMode(redisSessionProperties.getSaveMode());
		this.setCleanupCron(redisSessionProperties.getCleanupCron());
	}
}

If Spring Boot exposed SessionRepositoryCustomizer<RedisIndexedSessionRepository>, then this could be cleaned up even more. However, it is debatable if a user creating their own Bean would want Spring Boot properties applied. Perhaps the user would need to create an instance of the SessionRepositoryCustomizer<RedisIndexedSessionRepository> and manually apply it to their Bean.

Ultimately, I think it is up to the Boot team to decide if this is too much work but I think this is a limited amount of work for a user that is moving to a major release. Additionally, our users expect to configure a Bean when they decide to go outside of the recommendations (i.e. using the index repository that is not scaling).

[wilkinsona]

Users can reuse Spring Boot's SessionProperties which reduces the necessary code even further

They shouldn't do that. We document that the getters and setters are not public API:

The properties that map to @ConfigurationProperties classes available in Spring Boot, which are configured through properties files, YAML files, environment variables, and other mechanisms, are public API but the accessors (getters/setters) of the class itself are not meant to be used directly.

This is important in allowing us to evolve configuration properties to add features and fix bugs. For example, we've had to change boolean to Boolean in the past which is binary incompatible.

Ultimately, I think it is up to the Boot team to decide if this is too much work but I think this is a limited amount of work for a user that is moving to a major release.

Viewed in isolation, this may be a small amount of work. However, small amounts of work quickly add up to a significant amount of work that impedes adoption. We're already requiring users to move to Jakarta EE 9 which is a pretty big ask and one that we can't do much about. Given this, it's our opinion that we should do all we can to minimise the upgrade effort in other areas where we do have some control. I think this is one such area. I'll flag this for discussion at a future team meeting to see if we are in agreement.

[vpavic]

@rwinch and I discussed this today, and concluded that enableIndexingAndEvents attribute of @EnableRedisHttpSession is very likely going away in favor of something more flexible and future proof (possibly an enum attribute).

We also plan to review and potentially restructure some of the existing Spring Session configuration infrastructure, which shouldn't (at least too much) impact Spring Boot's auto-configuration support.

I'll start looking into these changes next week and will share the progress here. When things are done on Spring Session side, I can also help with adopting Spring Boot to these changes.

[wilkinsona]

Thanks, @vpavic. While you're doing that, can you please keep in mind that, from Spring Boot's perspective, it's important that it's as straightforward as possible for a user to switch back to the current behaviour where an indexed repository is used.

[vpavic]

Sure. Being able to go back to previous defaults with least amount of friction for users is among goals for this, regardless of the way they configure Spring Session (meaning, Session's own configuration support or Boot's auto-configuration).

[vpavic]

To provide some updates here on the direction Spring Session will likely go in. On top of the existing @EnableRedisHttpSession that will going forward configure RedisSessionRepository and expose a set of attributes that are relevant to that session repository implementation, Spring Session will also offer @EnableRedisIndexedHttpSession that can will be used to configure RedisIndexedSessionRepository and will also expose relevant attributes. So users that rely on Spring Session's configuration facilities would simply change the @Enable... annotation they use in order to opt into the previous defaults.

For Spring Boot side of things, I'd suggest introducing a configuration property like spring.session.redis.repository, that would allow opting into a non-default session repository implementation. So that users could switch back to the previous defaults by declaring spring.session.redis.repository=indexed property.

I've sketched something along those lines in gh-30673 branch of my fork.

[bclozel]

Thanks @vpavic - this looks like the right approach from our perspective. Would you like to turn that into pull request?

[vpavic]

Sure, that was the plan anyway for that branch - I was just waiting for some initial feedback from the team.

[scottfrederick]

Closing in favor of #32205