Add auto-configuration for Neo4j driver #22301

meistermeier · 2020-07-10T11:54:53Z

This commit adds the support for creating a managed instance of the Neo4j Java driver.
The low-level support for Neo4j is helpful in situations where the high-level abstraction of Spring Data Neo4j is not needed.

The PR is the foundation for the Spring Data Neo4j Starter update PR #22299 because it provides classes needed in the updated SDN starter.

Also there is the suggested related change for the initializer that enables the Neo4j driver dependency and allows the autoconfiguration to kick in: spring-io/start.spring.io#483

This commit adds the support for creating a managed instance of the Neo4j Java driver. The low-level support for Neo4j is helpful in situations where the high-level abstraction of Spring Data Neo4j is not needed.

snicoll

Thanks a lot for the PR. Getting auto-configuration for the driver that Spring Data bits then reuse is exactly the kind of arrangement we're aiming at so that's great to see it happening for Neo4j too.

I've made a few comments. There are similar formatting and code style as in the other PR.

snicoll · 2020-07-17T09:06:56Z

...onfigure/src/main/java/org/springframework/boot/autoconfigure/neo4j/DriverConfiguration.java

+	@ConditionalOnMissingBean(Driver.class)
+	Driver neo4jDriver(final Neo4jDriverProperties driverProperties) {
+
+		final AuthToken authToken = driverProperties.getAuthentication().asAuthToken();


Style, our codebase doesn't use final (neither in attribute, nor in constructor argument).

snicoll · 2020-07-17T09:08:11Z

...figure/src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverProperties.java

+ * @since 2.4.0
+ */
+@ConfigurationProperties(prefix = "spring.neo4j")
+public class Neo4jDriverProperties {


Given the spring.neo4j namespace, this should be named Neo4jProperties. Usage of this class should also be named consistently. If you think the driver part is important, then it should be reflected in the namespace.

snicoll · 2020-07-17T09:09:28Z

...figure/src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverProperties.java

+			this.kerberosTicket = kerberosTicket;
+		}
+
+		AuthToken asAuthToken() {


Although a number of other properties have the same structure, we are trying to make sure @ConfigurationProperties-annotated type are as simple as possible and do not contain any logic. Please move this to the configuration if possible.

snicoll · 2020-07-17T09:09:58Z

...figure/src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverProperties.java

+		return this.authentication.asAuthToken();
+	}
+
+	public Config asDriverConfig() {


Same remark than asAuthToken

snicoll · 2020-07-17T09:10:53Z

...src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverAutoConfiguration.java

+@ConditionalOnClass(Driver.class)
+@EnableConfigurationProperties(Neo4jDriverProperties.class)
+@Import(DriverConfiguration.class)
+public class Neo4jDriverAutoConfiguration {


For consistency with other driver auto-config, this should be Neo4jAutoConfiguration.

snicoll · 2020-07-17T09:18:48Z

...figure/src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverProperties.java

+
+		AuthToken asAuthToken() {
+
+			boolean hasUsername = StringUtils.hasText(this.username);


The code might a bit messy here but I had hoped if that makes sense to provide a fallback for spring.data.neo4j.usename and spring.data.neo4j.password. Same, a metadata with warning deprecation should indicate the replacement.

...figure/src/main/java/org/springframework/boot/autoconfigure/neo4j/Neo4jSpringJclLogging.java

snicoll · 2020-07-17T09:22:22Z

...e/src/test/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverPropertiesTests.java

+		}
+	}
+
+	@Nested


As I've indicated on the other PR we don't use @Nested anywhere so I'd prefer to leave it out of this PR if possible.

snicoll · 2020-07-17T09:24:25Z

...e/src/test/java/org/springframework/boot/autoconfigure/neo4j/Neo4jDriverPropertiesTests.java

+
+	private Neo4jDriverProperties load(String... properties) {
+
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();


Even such tests use the application context runner. It is a superior option compared to a simple handling of the context as you still need to handle the context otherwise.

Given that the logic for the properties object would be moved elsewhere, this might not be necessary after all.

snicoll · 2020-07-17T09:31:12Z

spring-boot-project/spring-boot-dependencies/build.gradle

@@ -1432,6 +1439,7 @@ bom {
 				"spring-boot-starter-logging",
 				"spring-boot-starter-mail",
 				"spring-boot-starter-mustache",
+				"spring-boot-starter-neo4j",


I think this should be removed as there is no starter for the driver (and there shouldn't be any if all that's needed is the driver.

meistermeier · 2020-07-17T10:13:01Z

(Again) thanks for the valuable feedback. ❤️
I would now focus on this PR to get the foundation for the SDN 6 changes (and compilable code). At least I see a benefit of having these independent changes merged first and base the other PR(s) on this.

snicoll · 2020-07-17T10:15:03Z

Yes! Thank you again.

meistermeier · 2020-07-17T13:24:16Z

I hope that I fixed or clarified the problems you have found.
Unfortunately I have overseen the formatting issues in the first run(s). There was also one in the Neo4jProperties but a gradle format did not touch it and in the next run it wasn't there anymore 🤷‍♀️

meistermeier · 2020-07-20T06:55:08Z

Is there anything left to do for me or do you need any additional information?

snicoll · 2020-07-20T07:13:20Z

I am good for this one, thanks @meistermeier!

This commit adds the support for creating a managed instance of the Neo4j Java driver. The low-level support for Neo4j is helpful in situations where the high-level abstraction of Spring Data Neo4j is not needed. See gh-22301

See gh-22301

snicoll · 2020-07-27T14:52:12Z

Thanks a lot @meistermeier (and @michael-simons). I've polished things quite a bit in 8c418ad and we have enough runway to adapt things if necessary.

I am going to review the actuator stuff that are relevant to the driver next. And finally move to Spring Data 2020.0.0-SNAPSHOT to upgrade to SDN 6.x. If you have time to rebase #22299, that would be much appreciated.

meistermeier · 2020-07-27T15:56:45Z

Thanks a lot @snicoll . Rebasing and ensuring everything still works as expect is on my list next.

Add autoconfiguration for Neo4j driver.

cd2184f

This commit adds the support for creating a managed instance of the Neo4j Java driver. The low-level support for Neo4j is helpful in situations where the high-level abstraction of Spring Data Neo4j is not needed.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 10, 2020

meistermeier mentioned this pull request Jul 10, 2020

Upgrade to Spring Data Neo4j 6 #22299

Closed

wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 14, 2020

wilkinsona added this to the 2.4.x milestone Jul 14, 2020

wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Jul 14, 2020

snicoll changed the title ~~Add autoconfiguration for Neo4j driver.~~ Add auto-configuration for Neo4j driver Jul 17, 2020

snicoll requested changes Jul 17, 2020

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jul 17, 2020

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 17, 2020

snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jul 17, 2020

meistermeier added 4 commits July 17, 2020 14:12

Bring in requested changes.

3a9010b

Fix formatting issues.

7d6c22e

Fix formatting import order issues.

4e82639

Fix formatting import order issues in tests.

3158018

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 17, 2020

snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jul 18, 2020

snicoll self-assigned this Jul 20, 2020

snicoll modified the milestones: 2.4.x, 2.4.0-M2 Jul 27, 2020

snicoll added a commit that referenced this pull request Jul 27, 2020

Polish "Add auto-configuration for Neo4j driver"

8c418ad

See gh-22301

snicoll closed this in e6f1819 Jul 27, 2020

snicoll removed the status: feedback-provided Feedback has been provided label Jul 27, 2020

meistermeier deleted the feature/neo4j-driver-autoconfiguration branch July 27, 2020 15:55

snicoll mentioned this pull request Aug 13, 2020

Provide dedicated Neo4j driver auto-configuration #17610

Closed

spring-projects-issues mentioned this pull request Dec 31, 2020

Add support for java.net.URL attributes. [DATAGRAPH-1356] spring-projects/spring-data-neo4j#1920

Closed


		AuthToken asAuthToken() {

		boolean hasUsername = StringUtils.hasText(this.username);

+              		}
+              	}
+              	@Nested


		private Neo4jDriverProperties load(String... properties) {

		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();

Add auto-configuration for Neo4j driver #22301

Add auto-configuration for Neo4j driver #22301

Uh oh!

Conversation

meistermeier commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meistermeier commented Jul 17, 2020

Uh oh!

snicoll commented Jul 17, 2020

Uh oh!

meistermeier commented Jul 17, 2020

Uh oh!

meistermeier commented Jul 20, 2020

Uh oh!

snicoll commented Jul 20, 2020

Uh oh!

snicoll commented Jul 27, 2020

Uh oh!

meistermeier commented Jul 27, 2020

Uh oh!

Uh oh!

meistermeier commented Jul 10, 2020 •

edited

Loading

snicoll Jul 17, 2020 •

edited

Loading