Skip to content

Missing TransactionManager when user provides a custom Neo4j SessionFactory. #17662

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 1 commit into from
Closed

Missing TransactionManager when user provides a custom Neo4j SessionFactory. #17662

wants to merge 1 commit into from

Conversation

michael-simons
Copy link
Contributor

Thise commit pushes @ConditionalOnMissingBean(SessionFactory.class) from the main Neo4j autoconfiguration class down to the bean declaration level. In addition, the OGM configuration bean won’t be created when there’s already a SessionFactory.

Thus, the Neo4jWebConfiguration will still be applied, even when user decides to run their own SessionFactory.

This change is needed to provide the mechanism proposed in #17610 in an external starter, which would provide the SessionFactory. As we like to provide the starter for both Boot 2.2.0 and 2.1.x, I'd like to see this change in both branches if possible.

Thanks for your ongoing support in this case.

@snicoll snicoll changed the title Make Neo4j autoconfiguration more flexibile. Make Neo4j autoconfiguration more flexibile Jul 29, 2019
@snicoll snicoll changed the title Make Neo4j autoconfiguration more flexibile Make Neo4j auto-configuration more flexible Jul 29, 2019
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 29, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the follow-up on our conversation. I've added one question and one suggestion. I'd like also that we exercise the feature you're trying to build in your custom starter (i.e. a test that provides a custom SessionFactory and validates the necessary bits are still auto-configured.

@@ -94,6 +94,7 @@ public void customSessionFactory() {
this.contextRunner.withUserConfiguration(CustomSessionFactory.class).run((context) -> {
assertThat(context).doesNotHaveBean(org.neo4j.ogm.config.Configuration.class);
assertThat(context).hasSingleBean(SessionFactory.class);
assertThat(context).hasSingleBean(OpenSessionInViewInterceptor.class);
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to commit this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I first had a dedicated test customSessionShouldNotDisableOSiV, my interpretation of

(i.e. a test that provides a custom SessionFactory and validates the necessary bits are still auto-configured.

Thise commit pushes `@ConditionalOnMissingBean(SessionFactory.class)` from the main Neo4j autoconfiguration class down to the bean declaration level. In addition, the OGM configuration bean won’t be created when there’s already a `SessionFactory`.

Thus, the `Neo4jWebConfiguration` will still be applied, even when user decides to run their own `SessionFactory`.
@michael-simons michael-simons changed the title Make Neo4j auto-configuration more flexible Missing TransactionManager when user provides a custom Neo4j SessionFactory. Jul 30, 2019
@michael-simons
Copy link
Contributor Author

To make things explicit: I think not providing a TransactionManger and also not a OpenSessionInViewInterceptor is a bug. My PR fixes that.

This is the actual test for the desired behaviour and included in the PR

@Test
	public void customSessionFactoryShouldNotDisableOtherDefaults() {
		this.contextRunner.withUserConfiguration(CustomSessionFactory.class).run((context) -> {
			assertThat(context).hasSingleBean(Neo4jTransactionManager.class);
			assertThat(context).hasSingleBean(OpenSessionInViewInterceptor.class);
		});
	}

@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 30, 2019
@philwebb philwebb added this to the 2.1.x milestone Jul 30, 2019
snicoll pushed a commit that referenced this pull request Aug 2, 2019
This commit separates the auto-configuration of the `SessionFactory` in
an isolated class so that the rest of the auto-configuration is still
applied if the user provides a custom `SessionFactory` bean.

See gh-17662
@snicoll snicoll self-assigned this Aug 2, 2019
@snicoll snicoll modified the milestones: 2.1.x, 2.1.7 Aug 2, 2019
@snicoll snicoll closed this in dd5ec49 Aug 2, 2019
@michael-simons
Copy link
Contributor Author

Fantastic. Much appreciated.

@michael-simons
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants