Skip to content

Introduce Spring Session MongoDB. #1901

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

Conversation

gregturn
Copy link
Contributor

  • Migrate the module's code back into this project.
  • Fold the documentation in.
  • Update to current Gradle conventions.
  • Reformat to match styling.

@gregturn gregturn requested a review from eleftherias August 26, 2021 17:05
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 26, 2021
dependency 'com.h2database:h2:1.4.200'
dependency 'com.ibm.db2:jcc:11.5.6.0'
dependency 'com.microsoft.sqlserver:mssql-jdbc:9.4.0.jre8'
dependency 'com.oracle.database.jdbc:ojdbc8:21.1.0.0'
dependency 'com.zaxxer:HikariCP:3.4.5'
dependency 'de.flapdoodle.embed:de.flapdoodle.embed.mongo:2.2.0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be a good idea to use Testcontainers instead of embedded MongoDB for integration testing, for consistency with the rest of the integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -15,11 +16,15 @@ dependencyManagement {
}

dependency 'org.aspectj:aspectjweaver:1.9.7'
dependency 'org.aspectj:aspectjweaver:1.9.6'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems redundant (see one above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

dependency 'org.hsqldb:hsqldb:2.5.1'
dependency 'org.mariadb.jdbc:mariadb-java-client:2.7.4'
dependency 'org.mockito:mockito-core:3.11.2'
dependencySet(group: 'org.mockito', version: '3.10.0') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to actually downgrade Mockito version (see one line above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

settings.gradle Outdated
@@ -17,6 +17,7 @@ include 'spring-session-data-redis'
include 'spring-session-docs'
include 'spring-session-hazelcast'
include 'spring-session-jdbc'
include 'spring-session-mongodb'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be spring-session-data-mongodb, meaning the code should also reallocate to the equally named directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -140,13 +148,11 @@ While convenient, this approach was not sustainable long-term as more features a

Starting with Spring Session 2.0, the project has been split into Spring Session Core module and several other modules that carry `SessionRepository` implementations and functionality related to the specific data store.
Users of Spring Data should find this arrangement familiar, with Spring Session Core module taking a role equivalent to Spring Data Commons and providing core functionalities and APIs, with other modules containing data store specific implementations.
As part of this split, the Spring Session Data MongoDB and Spring Session Data GemFire modules were moved to separate repositories.
As part of this split, the Spring Session Data GemFire module was moved to a separate repository.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the entire Spring Session Modules chapter should be rewritten, and this specific paragraph (that is focused on what happened in 2.0) removed. Changing this sentence alone but not the context around it is both misleading and incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -0,0 +1,986 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this pom.xml isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


optional "org.mongodb:mongodb-driver-core"
testCompile "org.mongodb:mongodb-driver-sync"
testCompile ("de.flapdoodle.embed:de.flapdoodle.embed.mongo") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in one of the earlier comments, IMO it would be a good idea to use Testcontainers instead of embedded MongoDB for integration testing, for consistency with the rest of the integration tests.

Additionally, the tests should also be split to two source sets - (default) one for unit tests and an additional one for integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,139 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably go into spring-session-docs subproject.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@gregturn
Copy link
Contributor Author

gregturn commented Sep 7, 2021

Things @vpavic for checking out my PR. I'll make changes.

@gregturn gregturn force-pushed the issue/spring-session-data-mongodb branch from 7d3fb7d to 94e4163 Compare September 7, 2021 18:26
@gregturn
Copy link
Contributor Author

gregturn commented Sep 7, 2021

I've applied all the changes requested @vpavic. I squashed them together. Feel free to offer more comments.

@vpavic
Copy link
Contributor

vpavic commented Sep 7, 2021

Hey @gregturn, I'll try to take another look at this by the end of the week.

@vpavic vpavic self-requested a review September 7, 2021 21:09
@eleftherias eleftherias added type: enhancement A general enhancement in: mongo-db and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 27, 2021
gregturn and others added 4 commits October 11, 2021 16:21
* Migrate the module's code back into this project.
* Fold the documentation in.
* Update to current Gradle conventions.
* Reformat to match styling.
@eleftherias eleftherias force-pushed the issue/spring-session-data-mongodb branch from 94e4163 to 2ba58f2 Compare October 11, 2021 14:40
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregturn I added commits to rebase on main and update the documentation to use Antora.
I also fixed the samples, I added an inline comment about the change that I made, let me know if you prefer an alternative approach.

compile "org.springframework.boot:spring-boot-starter-webflux"
compile "org.springframework.boot:spring-boot-starter-thymeleaf"
compile "org.springframework.boot:spring-boot-starter-data-mongodb-reactive"
compile "de.flapdoodle.embed:de.flapdoodle.embed.mongo"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an embedded MongoDB to both samples.
The alternative is to expect the user to have a local MongoDB instance on their machine before running the sample. In that case we would need to update the integration tests for the samples to use test containers.

@eleftherias
Copy link
Contributor

Merged via bf139db

@eleftherias eleftherias self-assigned this Oct 20, 2021
@eleftherias eleftherias added this to the 2.6.0-RC2 milestone Oct 20, 2021
eleftherias added a commit that referenced this pull request Nov 26, 2021
eleftherias added a commit that referenced this pull request Jan 14, 2022
@gregturn gregturn deleted the issue/spring-session-data-mongodb branch April 14, 2022 14:28
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.

4 participants