spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.25k stars 40.7k forks source link

Consider reinstating auto-configuration for Spring Session Mongo DB #9552

Closed wilkinsona closed 7 years ago

wilkinsona commented 7 years ago

This commit could, largely, be reverted. The project now lives on separately. A 2.0.0.M1 release is available from repo.spring.io.

vpavic commented 7 years ago

:+1:

Project's coordinates have changes (it's spring-session-data-mongodb now, instead of spring-session-data-mongo) and it will require a separate version property in dependency management.

However note that we're strongly considering splitting the Spring Session into modules by session repository implementation (update: see spring-projects/spring-session#806), meaning there will be a spring-session-core artifact while existing Redis, JDBC and Hazelcast artifacts (which are basically BOMs currently) will include the respective session repository implementation and related code.

This will require some changes in Boot's auto-config support for Spring Session so it might be a good idea to handle that change before reverting old MongoDB related code.

/cc @rwinch

wilkinsona commented 7 years ago

@rwinch Can you please help us to understand the motivation for splitting out the Mongo DB support from Spring Session? We're curious about the benefits of splitting things up. Is jumping straight in and supporting it again in Spring Boot the right thing for us to do? Put another way, is it going to be maintained in the long term?

vpavic commented 7 years ago

@wilkinsona The background of these changes were explained in depth in spring-projects/spring-session#768 and https://spring.io/blog/2017/05/11/spring-session-2-0-0-m1-released.

The new spring-session-data-mongodb should also be considered officially supported since it was picked up by @gregturn.

rwinch commented 7 years ago

@wilkinsona I think this was explained quite well by @vpavic

One thing to add is I view Spring Session as shaping to be a little like Spring Data team in that different data stores are owned by different teams. The data stores supported by the Spring Team are officially supported by their respective team and those that are not are community supported.

gregturn commented 7 years ago

@wilkinsona I'm all in favor of adding the managed version to Boot's listing of versions, yet holding off autoconfiguration until the dust settles. For example, I know Rob is working on core changes that haven't been picked up yet in Spring Session MongoDB. So if he and @vpavic need to hammer out some artifact details, by all means let them proceed.

But in the meantime, let's not hold back people that want access to this officially supported product from picking up the right version a la Boot.

Again, this is akin to Spring Data, so I didn't realize there was yet controversy over divvying up into separate repos.

wilkinsona commented 7 years ago

Thank you, @vpavic and @rwinch

But in the meantime, let's not hold back people that want access to this officially supported product from picking up the right version a la Boot.

Generally speaking, we only provide dependency management for something that Boot's using and won't just provide a version for something.

I've subscribed to https://github.com/spring-projects/spring-session/issues/806. Let's wait for that to land before we tackle this.

snicoll commented 7 years ago

let's not hold back people that want access to this officially supported product from picking up the right version a la Boot.

@rwinch if that module is officially supported, why isn't it part of the bom? We shouldn't have a separate entry in dependency management for this IMO

gregturn commented 7 years ago

@snicoll BOM? Are you referring to the Spring IO platform BOM or to a Spring Session BOM that doesn't exist at this stage?

snicoll commented 7 years ago

There is a reference to a bom in the description of spring-projects/spring-session#806

gregturn commented 7 years ago

There is no BOM right now. We're discussing the subject on Slack right now.

vpavic commented 7 years ago

There is a reference to a bom in the description of spring-projects/spring-session#806

@snicoll The description of that issue refers to previous state of the spring-session-data-redis, spring-session-jdbc and spring-session-hazelcast modules which were basically BOM modules dependency aggregator/starter modules (until referenced issue was resolved) since all of the Spring Session code was in spring-session artifact.

philwebb commented 7 years ago

@rwinch

Now that the Spring Session project has been split up we're noticing an unfortunate side-effect with the Spring Boot support. With Spring Boot 1.5 the user would select "Spring Session" from http://start.spring.io and combine it with the technology that they wanted. For example, "Spring Session" + "Redis" would provide Redis Session support.

This worked because the spring-session JAR contained all the classes and every dependency was optional.

With the new layout, dependencies are no longer optional and the JAR is named after the technology being used. We've got a few options that we can consider, but none of them feel ideal:

Have multiple "Session" starters on start.spring.io

We could explode the existing "Spring Session" checkbox into N options ("Spring Session Redis", "Spring Session JDBC") etc. I'm not a big fan of this idea because it adds a lot of additional noise to the site.

Add transitive excludes

We could create a "Spring Boot Session" starter that pulls in all technology specific JARs but we add <exclude> sections for all the actual technology imports. This would be a pain for us to maintain and I think it's likely to confuse the user since they'll see spring-session-halzelcast JARs even if they're not using Hazelcast

Drop technology specific out-of-the-box support

We could change the "Spring Session Starter" so that it only pulls in spring-session-core. We could then make the core configure fail if no specific technology has been selected. So if you check "Spring Session" on start.spring.io you'll get an app that won't start, but you'll get some guidance as to what you should do:

Application failed to start because you've not included a `spring-session` technology dependency.
Please add the appropriate dependency, for example, if you want Redis you should add `spring-session-redis`.

The last option is probably my favorite so far as it seems the least confusing for users. I'm still not sure that it's as nice as the 1.5 behavior. Do you have any thoughts on the best way to proceed?

vpavic commented 7 years ago

Just to add some references to your comment @philwebb, the Initializr issue was reported in spring-io/initializr#455 while Spring Session starter(s) was proposed a while ago in #6082 (obviously the context of that proposal has changed now).

The direction of Spring Session is shaping into somewhat similar direction to one with Spring Data and Spring Social. Not to repeat the background of the change (if it matters) I'll just point to spring-projects/spring-session#768 and this blog post.

Regarding the multiple Session options on Initializr I'd like to point out one thing - with that option a user that (for example) uses a relational database but would like Redis backed sessions would pick JPA and Session Redis components. This is IMO quite expressive and would also allow Initializr to correctly set the spring.session.store-type to Redis. In contrast, with the current state user would need to select JPA, Redis and Session components with user still required to set spring.session.store-type config prop in generated project.

philwebb commented 7 years ago

The more I think about it, the more I wonder if the multiple options on start.spring.io might actually be the best option. I know that there are some reservations about adding more items, but it does actually make a lot of sense when you consider:

What are the downsides:

wilkinsona commented 7 years ago

IMO, likening Spring Session to Spring Social or Spring Data doesn't really make sense.

With Spring Social or Spring Data you are interacting directly with a store- or social network-specific API. With Spring Session you are interacting with a general purpose API that, almost as an implementation detail, can use different backing stores.

Spring Session feels more like something like Spring Data JPA to me. You interact with a general purpose API that can use different RDBMSes for persistence. We don't have starters or checkboxes on start.spring.io for Data JPA with MySQL, Data JPA with Postgres, etc. I don't think we should have similar checkboxes for Spring Session either.

Unfortunately, from a usability perspective at least, the new arrangement feels like a step backwards.

philwebb commented 7 years ago

There's certainly a lot to consider here. This is one of those unfortunate problems where in isolation everything makes sense, but when we try to pull it back together there are issues:

Once you decide that you want Spring Boot to be in control of data technologies however, things start to clash. The Redis driver pulled in by Spring Session might not be the one you want to use. What if you want to use spring-session-mongodb but you don't want Jackson?

I wonder if we can reach some middle ground where the spring-session-x projects only use optional dependencies. It's perhaps a little annoying for non Boot users that they'd need to include spring-session-redis + some redis driver, but it's better than needing a lot of excludes.

We can then have a spring-boot-starter-session project that just pull in all the Spring Session jars. The only downside is a user might see a spring-session-hazelcast jar when they are only using Redis. But it's only a few KB and about 8 classes so I don't think it's a massive problem.

snicoll commented 7 years ago

What Andy said.

I would have preferred the start.spring.io topic to be brought in https://github.com/spring-io/initializr/issues/455 but since we started here... From my experience, discoverability is far more important than expressiveness (and we want users to be focused). If you want to work with Redis, you add the redis checkbox. If you want to work with Spring Session, you add the spring session checkbox, etc. Sure you may realize later that the Spring Data Redis artifact is useless for your needs but you're 10.000 miles away of the decision that you took when you created the project.

Besides, spring-session-redis brings the Jedis driver. Rather than working with the advanced use case, let's work with a more simple one. You're using redis to store data and your sessions. Why does Spring Session have an opinion about the driver to use? We intend to bring Lettuce as a default so now we have two drivers on the classpath? Surely you can align but that still doesn't help users that want to deviate from the default. To take an extreme example, when you decide to use Jetty rather than Tomcat, you don't have to exclude it at 2 different places.

Worse than that, IMO, there is no way to get the functionality without those opinions. So if I want to use redis today, my only option is to add spring-session-redis and start excluding things.

Regarding Spring Social, there is a "Spring Social Facebook" entry because there is no "Facebook" entry. If there was one, I'd have the exact same argument. We have a "Redis", "JDBC" and "MongoDB" entries so comparing Spring Session to Spring Social from that perspective doesn't make sense.

Back to the actual change now, I most certainly have no opinion about splitting the code in several modules and having a release train. If the Spring Session team decides it's the right thing to do, that's fine by me. But, the new jar structure breaks the use case of 1.5 and that's a concern from our perspective. Ironically enough, those news jars are now far more easier to use if you are not using Spring Boot and much harder if you do (per my argument above about runtime choices taken by starters).

Here is a proposal: what is wrong with having independent modules as you do now and build the same spring-session-core artifact you used to with the "official" stores (or call it something else, whatever). We used to do this for Hibernate in Spring Framework, with several spring-orm-xyz modules than end up in spring-orm.

gregturn commented 7 years ago

I can certainly go with the idea that if you want Redis you'll use it for everything. Same for MongoDB.

Anything more complex should be annotated in Spring Session's reference docs.

Final question, what should default be? If Op picks "Session" on start.spring.io and nothing else, what does it do?

spencergibb commented 7 years ago

I like the JPA comparison. What does it do?

rwinch commented 7 years ago

Thank you for your input @wilkinsona @philwebb and @snicoll

With Spring Social or Spring Data you are interacting directly with a store- or social network-specific API.

This is not strictly true with Spring Data since it provides a common API for various data stores. For example, Spring Data Mongo, JPA, LDAP, Redis, Cassandra, etc all provide different implementations of CrudRepository. Similarly, Spring Session JDBC, Redis, etc provide different implementations of SessionRepository.

Of course there are implementation specific APIs as well. However, there is a core API in spring-data-commons just as there is a core API in spring-session-core. Similarly, both Spring Data and Spring Session provide specific data store implementations of the core APIs in their data store specific jars.

I think it is important to note that there are implementation specific starters for Cloud Cluster (Redis, Hazelcast, and Etcd). Additionally, there are implementation specific starters for Discovery (Eureka, Zookeeper, and Consul). There are plenty of other artifacts in Initializr that only vary by their implementation. I really don't think the idea of having implementation specific starters for Spring Session is much different (if at all) than what is already present in our existing ecosystem.

We don't have starters or checkboxes on start.spring.io for Data JPA with MySQL, Data JPA with Postgres, etc.

The difference is Spring doesn't provide the JPA implementations of MySQL, Postgres, etc. Those are provided by third parties. Therefore, I do not see Spring Session as the same as your analogy.

Spring Data does provide an implementation for JPA, Redis, etc. Similarly Spring Cloud provides discovery with Eureka and Zookeeper. Along the same lines, Spring Session provides implementations for JDBC, Redis, etc. Again, I don't see this much different than what Initializr is doing with other projects.

Unfortunately, from a usability perspective at least, the new arrangement feels like a step backwards.

I'd like to understand why you find this a step backwards. Can you expand on this?

I wonder if we can reach some middle ground where the spring-session-x projects only use optional dependencies. It's perhaps a little annoying for non Boot users that they'd need to include spring-session-redis + some redis driver, but it's better than needing a lot of excludes.

I think this makes a lot of sense. In fact, there is already a ticket in Spring Session to remove the Redis driver being included. See spring-projects/spring-session#802 It would probably be beneficial to take a look at all the transitive dependencies for the spring-session-* projects to ensure they make sense.

We can then have a spring-boot-starter-session project that just pull in all the Spring Session jars. The only downside is a user might see a spring-session-hazelcast jar when they are only using Redis. But it's only a few KB and about 8 classes so I don't think it's a massive problem.

However, I do not like the idea of an aggregate pom that combines each of the implementations. This makes it less clear what the user's intent is when they want Spring Session. More concretely, it means that we must continue to require the users provide spring.session.store-type because we do not know what their intention is. I'd much prefer the user select which specific session implementation they want, include only the jar we need, then we can check which implementation is on the classpath and create the necessary beans.

If you want to work with Redis, you add the redis checkbox. If you want to work with Spring Session, you add the spring session checkbox, etc.

Unfortunately it isn't that simple. If a user has both MonogoDB and JDBC (or any other permutation) on the classpath, which Spring Session implementation should be used? We don't know and this is why we require spring.session.store-type. If we refine our checkboxes to explicitly state which Spring Session implementation they want, the requirement for spring.session.store-type goes away and we cannot be put in an invalid state.

Sure you may realize later that the Spring Data Redis artifact is useless for your needs but you're 10.000 miles away of the decision that you took when you created the project.

Right. I'd rather users decide what they want up front rather than being 10.000 mi away when they realize they made the wrong decision. Require the user make the right choice up front.

Besides, spring-session-redis brings the Jedis driver. Rather than working with the advanced use case, let's work with a more simple one. You're using redis to store data and your sessions. Why is Spring Session have an opinion about the driver to use? We intend to bring Lettuce as a default so now we have two drivers on the classpath? Surely you can align but that still doesn't help users that want to deviate from the default. To take an extreme example, when you decide to use Jetty rather than Tomcat, you don't have to exclude it at 2 different places.

I think this is a valid point. We plan to address that in spring-projects/spring-session#802

But, the new jar structure breaks the use case of 1.5 and that's a concern from our perspective.

Perhaps we can create a Session category for 1.x and 2.x. For Boot 2.0+ we would have spring-session-* starters listed and spring-session disabled. For Boot < 2.0 we would have spring-session enabled and spring-session-* disabled.

Ironically enough, those news jars are now far more easier to use if you are not using Spring Boot and much harder if you do (per my argument above about runtime choices taken by starters).

I think there are some valid concerns here, but this seems to be mostly with transitive dependencies. Here we agree. I think if we clean up the transitive dependencies of spring-session- it will be a much better experience for our users.

Here is a proposal: what is wrong with having independent modules as you do know and build the same spring-session-core artifact you used to with the "official" stores (or call it something else, whatever). We do this for Hibernate in Spring Framework.

I don't think there is the need for an uber jar.

User Experience

Let's talk about the user experience today vs what it could be with the current arrangement. It appears that we agree we need to sort out transitive dependencies for spring-session-*. Let's assume this gets resolved.

Spring Session 1.x

Spring Session 2.x

Note that if we have an Uber jar or aggregation pom we go back to the state things were in 1.x where we have extra setup from users and can get into invalid setups.

wilkinsona commented 7 years ago

This is not strictly true with Spring Data

I disagree.

When you're using Spring Data, the code that you write is coupled to the datastore that you're using. It provides a familiar and similar API on top of those stores, but it is not an identical API across all of the stores. A developer still cares about @Document or @Entity or the different semantics of the stores (no transactions with Mongo, for example).

Similarly, Spring Session JDBC, Redis, etc provide different implementations of SessionRepository.

IMO, from a user's perspective, this isn't similar. A typical Spring Session user doesn't care about the different implementations of SessionRepository, particularly if they're using Boot where this is (largely) auto-configured for them. Their interaction with Spring Session is done through the Servlet API and a particular implementation of SessionRepository is an implementation detail that doesn't affect their application's logic.

The difference is Spring doesn't provide the JPA implementations of MySQL, Postgres, etc. Those are provided by third parties. Therefore, I do not see Spring Session as the same as your analogy.

No analogy is perfect, but I do think the one I used is closer than a Spring Social or Spring Data analogy. Another analogy could be Spring Framework's support for various view template technologies. All of the code is in spring-webmvc with optional dependencies on the template library. On start.spring.io you choose to build a web app and you choose a template library separately. That makes sense as you may be using the template library with MVC or in some other capacity. Just as you may be using Redis, Mongo, etc with Spring Session or in some other capacity.

I'd like to understand why you find this a step backwards. Can you expand on this?

This was really just a short way of saying that I agree with all of the cons that have already been listed. My main concern is the effect that the new split will have on start.spring.io. Spring Cloud already has too many checkboxes there (service discovery for example) and I don't want that broken window to influence what we do with Spring Session. Related, it'll also have a negative effect on Spring Boot's starters:

We can then have a spring-boot-starter-session project that just pull in all the Spring Session jars. The only downside is a user might see a spring-session-hazelcast jar when they are only using Redis. But it's only a few KB and about 8 classes so I don't think it's a massive problem.

At this point, would it not be better if Spring Session just grouped everything together in a single jar? Then a user adds a Spring Session dependency and, if they're not already using a datastore, a dependency on that too. Our auto-configuration will then figure out which Spring Session repository type to use. Further configuration is only required if there's more than one Spring Session supported datastore available and we need to know which one to use.

Lastly, I think it's worth noting that I haven't seen a single complaint about the current arrangement that we have for using Spring Session in Boot and on start.spring.io. From a Boot and start.spring.io perspective, the change appears to be trying to solve a problem that doesn't exist and is, I feel, making things worse in the process.

snicoll commented 7 years ago

Right. I'd rather users decide what they want up front rather than being 10.000 mi away when they realize they made the wrong decision. Require the user make the right choice up front.

That's not how users, and especially newcomers, are using start.spring.io in my experience. I actually meant the contrary: when they'll fine tune their projects later they may consider they don't need such and such dependencies and just provide more explicit ones. But that's 10.000 miles away from their need when they go to the site, that is getting started.

I think there are some valid concerns here, but this seems to be mostly with transitive dependencies.

Unfortunately, I don't think so. Let's go forward a bit. The Redis driver is removed from spring-session-redis and we add a "Spring Session Redis" checkbox on start.spring.io. You select that. What is the getting started experience? A user would expect they can start storing stuff to Redis using Spring Session. But they won't since the driver isn't available. In that state they are two outcomes that I see:

A third option would be for you to create a separate artifact in Spring Session that brings the implementation and the driver. That looks very familial to what Spring Boot is doing with the starter, isn't it?

If I only add Spring Session, I have accidentally selected an invalid state. So Spring Session is not going to work because I did not provide an implementation with it and there is no way of knowing which implementation the user wants.

Try to add JPA without any extra dependency and see what happens. Roughly 20% of all generated projects on start.spring.io use JPA and if you don't add a database you'll end up in an invalid state. Given the number of people complaining about this, I think we can safely conclude that this is working for our users.

If I add Spring Session, JDBC, and MongoDB we currently have no way of knowing which Spring Session implementation we need. This is why we require the spring.session.store-type.

What you describe a bug is a feature in my book. The store type has to be explicit. The fact you've split the implementations in separate jars doesn't shield you from having several spring-session-* jars in an application (transitive/conflicting deps, etc).

the change appears to be trying to solve a problem that doesn't exist and is, I feel, making things worse in the process

Andy summarized perfectly how I feel about this, +1 to that!

snicoll commented 7 years ago

I think it is important to note that there are implementation specific starters for Cloud Cluster

Sorry, I forgot to reply to that. I think it's important to note that those fall in the category of things I wish we didn't do that way. And partly the reason why I insist we don't make the same mistake with Spring Session. Those are deprecated as well so I really don't think we can use that as an argument.

philwebb commented 7 years ago

Just to note the outcome of a call we had today on this subject. There's still not 100% agreement on the correct approach, but in order to move forwards this is the plan.

In addition several things will be enhanced in Spring Boot

Additional considerations:

snicoll commented 7 years ago

The Spring Initializr change is available on the dev instance and here is the code https://github.com/snicoll/initializr/commit/a989188aae3532e8715678fd5bf02f5c2a086367

vpavic commented 7 years ago

I thought this and #9625 were on hold due to unwillingness of Boot team to add an additional dependency management property for Spring Session MongoDB?

BTW we are planning to have Spring Session BOM available soon.