-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Update Neo4j support to require Neo4j Java Driver 6.0.0 #47381
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
base: main
Are you sure you want to change the base?
Update Neo4j support to require Neo4j Java Driver 6.0.0 #47381
Conversation
Signed-off-by: Gerrit Meier <[email protected]>
Thanks for the PR. Is that a mandatory requirement for the next RC of Spring Data?
FTR we can't use the BOM as it isn't one. It provides dependency management for Netty and it should not as a BOM should only provide dependency management for the dependencies of the project itself. |
Signed-off-by: Gerrit Meier <[email protected]>
We merged the 6.0 driver upgrade in SDN to the main branch (next RC) this morning and as a supportive downstream developer, I raised this timely ;)
Thanks for looking into this, missed the definition there. |
Hey @snicoll see neo4j/neo4j-java-driver#1707 from our colleague. We had been unsure while adding it, the Maven manually is not 100% clear on this, at least there's nothing saying "you must not include dependencies" We would love to hear your experience on that topic, though. Please let us know if the declaration @meistermeier created in the Gradle file is enough or if we must change this before the PR can go in. Thank you. |
Signed-off-by: Gerrit Meier <[email protected]>
For sure I ran the wrong build locally, should be better now.
fails now but I assume that is due to Spring Boot checking Spring Data Neo4j's version definition from a milestone branch instead of latest main. |
Very cool, thank you!
I bet it isn't. They can't make such a strong stance but we can. Or rather, we can decide not to include a BOM that does not match that expectation. The main idea is that if you have two sources of dependency management for the same artifact, it can lead to various issues (inconsistencies, warning in Maven itself, etc). Neo4J isn't at the application layer, it is a driver/library. As such, it should not have an opinion on the exact version of Netty that users should consume. If that is necessary, then shading is the right option. |
Yes. I think you should migrate to Spring Data snapshot in your PR. It's a bit odd that you migrate to the new driver whilst Spring Data Neo4J isn't upgraded. Actually, I am surprised that it even works... |
Thanks @snicoll This makes sense and we will fix that in the next release. The intention behind was this: The latest driver can use native transport, such as epoll, io_uring etc. We wanted to help application builders and picking the right versions of the than required additional dependencies without having to search for them. But on the other hand, I think it's fair to assume quite technical users in that area that are aware that there's a bom for that. We also update our manual on that topic. Cheers. |
See spring-projectsgh-47381 Signed-off-by: Gerrit Meier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tanks for the PR. I've started to polish in https://github.com/snicoll/spring-boot/tree/pr/47381
There's no need to push any further change. I've added a few comments and could use your insights when you have a min.
Driver neo4jDriver(Neo4jProperties properties, Environment environment, Neo4jConnectionDetails connectionDetails, | ||
ObjectProvider<ConfigBuilderCustomizer> configBuilderCustomizers) { | ||
ObjectProvider<ConfigBuilderCustomizer> configBuilderCustomizers, | ||
ObjectProvider<ObservationRegistry> observationRegistryProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do this here. If we did then we could not configure a Neo4j driver without the observation registry on the classpath.
builder.withMaxConnectionLifetime(pool.getMaxConnectionLifetime().toMillis(), TimeUnit.MILLISECONDS); | ||
builder.withConnectionAcquisitionTimeout(pool.getConnectionAcquisitionTimeout().toMillis(), | ||
TimeUnit.MILLISECONDS); | ||
if (pool.isMetricsEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I like that change. If metrics vs. no metrics is no longer needed, then I think the metricsEnabled
flag should go away and have metrics to be exported if an ObservationRegistry
is available. That's what the other components are doing at the moment.
Is there a reason to keep having a flag for this or was it just an attempt at migrating what we had before?
Scheme.validateScheme(lowerCaseScheme); | ||
} | ||
catch (IllegalArgumentException ex) { | ||
if (!ServiceLoader.load(BoltConnectionProviderFactory.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite a bit of ceremony here. Do we really need to invoke "any" factory this way? Also this adds an import on BoltConnectionProviderFactory
...
I've also switched this PR to an enhancement as it isn't "just" a dependency upgrade. |
Upgrading the Neo4j Java driver to the next major version requires some changes:
My build setup (and general Gradle) knowledge is pretty limited and I am not sure how to make use of the Java driver BOM (https://central.sonatype.com/artifact/org.neo4j.driver/neo4j-java-driver-bom) here. Given the fact that there is already the alignment definition for Spring Data Neo4j in the dependency definition.
I couldn't find any micrometer tests with Neo4j and was not sure if this is needed and/or where to place them correctly.