Skip to content

Conversation

michael-simons
Copy link
Contributor

In the upcoming next release of Neo4j-OGM we provide means to use Neo4j native types (Spatial, Time ec.) in Neo4j-OGM. This native types allow the user to use java.time.*and others in entities without OGM converting them to Strings or longs.

These types are available for Bolt and Embedded modes and a feature requested by our users.

This PR contains the following:

  • Added the modules containing the native type system for Bolt and Embedded Neo4j-OGM to the dependency management
  • A new configuration property to enable them (They are an opt-in feature)
  • A failure analyser when they are enabled but the dependencies aren't on the class path

The dependency of OGM has been set to a released alpha version to be able to discuss this PR.

Neo4j-OGM 3.2 will be the version Spring Data Moore (5.2) requires. It is however not compatible with Spring Data Lovelace (5.1) and as such, this PR should not make it into a Spring Boot version that uses Spring Data Lovelace or before.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 7, 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. I had a quick look and added a few questions.

CypherException cypherException = new CypherException("Error executing Cypher",
"Neo.ClientError.Statement.SyntaxError",
"Unable to execute invalid Cypher");
CypherException cypherException = new CypherException(
Copy link
Member

Choose a reason for hiding this comment

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

I welcome such polish but I'd prefer to see them in a separate commit.

Copy link
Member

Choose a reason for hiding this comment

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

Was this resolved really? I can still see the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split into two separate commits.
I wanted to have this in one PR, as it is a breaking api change in Neo4j 3.2. Can create two PRs from it as well.

Copy link
Member

Choose a reason for hiding this comment

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

There are seven commits right now so it's hard for me to figure out what is supposed to be squashed and what isn't.

Generally speaking, if you have something that's good to go, I very much prefer a quick PR that I can merge right away. If you don't have time to create one, that's fine as well but please squash the current commits to make that a bit more explicit. Thanks!

@michael-simons
Copy link
Contributor Author

I followed your suggestion and replaced the analyzer by some logic throwing an InvalidConfigurationPropertyValueException. I looked through the mechanism handling those and I think it's appropriate for that use case here. Thanks for the hint.

One of the test will fail now as I discovered a bug in our OGM prerelease. So again, thanks for the feedback and ideas.

All other suggestions applied.

@snicoll snicoll added the status: blocked An issue that's blocked on an external project change label Jan 8, 2019
@snicoll
Copy link
Member

snicoll commented Feb 12, 2019

@michael-simons can you please squash what needs to be and give us an update about this one?

This is the version required by SDN 5.2 (Moore) and the one bringing in native types.
It requires some adaption to API changes in a test-class.
This includes tests for the autoconfiguration using that new property.
The test require the native types for Bolt and embedded in the test
scope, so the Neo4j-OGM native types have been added to managed dependencies.

The enhanced autoconfiguration throws an
InvalidConfigurationPropertyValueException when native types cannot be
used due to missing dependencies or wrong transport mode.
@michael-simons
Copy link
Contributor Author

I have extracted the version upgrade into #15937 and squashed the remaining commits here.

Thanks for your feedback and time on this.

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: blocked An issue that's blocked on an external project change status: waiting-for-triage An issue we've not yet triaged labels Feb 13, 2019
@wilkinsona wilkinsona self-assigned this Feb 13, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Feb 13, 2019
wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Feb 13, 2019
This includes tests for the autoconfiguration using that new property.
The test require the native types for Bolt and embedded in the test
scope, so the Neo4j-OGM native types have been added to managed
dependencies.

The enhanced autoconfiguration throws an
InvalidConfigurationPropertyValueException when native types cannot be
used due to missing dependencies or wrong transport mode.

See spring-projectsgh-15637
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Feb 13, 2019
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Feb 13, 2019
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Feb 13, 2019
@wilkinsona wilkinsona changed the title Provide an option to use Neo4j-OGM native types. Add opt-in support for Neo4j-OGM native types Feb 13, 2019
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.0.M1 Feb 13, 2019
@wilkinsona
Copy link
Member

Thanks very much for the PR, @michael-simons. I've merged the proposed changes into master along with some polish. The biggest change there was to remove the exception translation. I think there may be some room for improvement in the exception message that could be done in OGM. If we then still want to do something in Boot, I think it would be better to add some dedicated failure analysers. As @snicoll pointed out to me, that would have the advantage of analysing the failure even when the auto-configuration has backed off.

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

Successfully merging this pull request may close these issues.

4 participants