Skip to content
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

NPE while attempting to rollback with version 4.1.2 #351

Closed
alankstewart opened this issue Aug 1, 2016 · 17 comments
Closed

NPE while attempting to rollback with version 4.1.2 #351

alankstewart opened this issue Aug 1, 2016 · 17 comments
Assignees
Labels
status: needs-investigation An issue that has been triaged but needs further investigation

Comments

@alankstewart
Copy link

alankstewart commented Aug 1, 2016

Without any changes to my code, I'm getting a NullPointerException when I update Spring Data Neo4j from 4.1.1 to 4.1.2:

java.lang.NullPointerException
    at org.neo4j.ogm.annotations.DefaultEntityAccessStrategy.getIdentityPropertyReader(DefaultEntityAccessStrategy.java:486)
    at org.neo4j.ogm.context.MappingContext.purge(MappingContext.java:307)
    at org.neo4j.ogm.context.MappingContext.clear(MappingContext.java:266)
    at org.neo4j.ogm.context.MappingContext.reset(MappingContext.java:282)
    at org.neo4j.ogm.session.transaction.DefaultTransactionManager.rollback(DefaultTransactionManager.java:82)
    at org.neo4j.ogm.transaction.AbstractTransaction.rollback(AbstractTransaction.java:65)
    at org.neo4j.ogm.drivers.embedded.transaction.EmbeddedTransaction.rollback(EmbeddedTransaction.java:60)
    at org.springframework.data.neo4j.transaction.Neo4jTransactionManager.rollback(Neo4jTransactionManager.java:60)

The code calls org.springframework.transaction.PlatformTransactionManager#rollback and the NPE is thrown. I've also tried various versions of Neo4j 3.0.x and neo4j-ogm 2.0.x as well.
I couldn't see anything in the release notes for 4.1.2 to suggest I need to change client code. Any help would be appreciated.

@mangrish mangrish added the status: needs-investigation An issue that has been triaged but needs further investigation label Aug 2, 2016
vince-bickers pushed a commit to neo4j/neo4j-ogm that referenced this issue Aug 19, 2016
@vince-bickers
Copy link

@alankstewart Please could you retry using the latest snapshot version of the OGM?

@alankstewart
Copy link
Author

alankstewart commented Aug 20, 2016

@vince-bickers Thanks for looking at this. With SDN 4.1.2, neo4j 3.0.4, and neo4j-ogm 2.0.5-SNAPSHOT I still get:

 java.lang.NullPointerException:
  at org.neo4j.ogm.entity.io.DefaultEntityAccessStrategy.getIdentityPropertyReader(DefaultEntityAccessStrategy.java:484)
  at org.neo4j.ogm.context.MappingContext.purge(MappingContext.java:341)
  at org.neo4j.ogm.context.MappingContext.clear(MappingContext.java:273)
  at org.neo4j.ogm.context.MappingContext.reset(MappingContext.java:289)
  at org.neo4j.ogm.session.transaction.DefaultTransactionManager.rollback(DefaultTransactionManager.java:82)
  at org.neo4j.ogm.transaction.AbstractTransaction.rollback(AbstractTransaction.java:65)
  at org.neo4j.ogm.drivers.embedded.transaction.EmbeddedTransaction.rollback(EmbeddedTransaction.java:60)
  at org.springframework.data.neo4j.transaction.Neo4jTransactionManager.rollback(Neo4jTransactionManager.java:60)
  at com.atomist.util.NeoTestTransactions$class.rollbackTx(NeoTestTransactions.scala:22)
  at com.atomist.contract.mapping.NeoRepositoryTest.rollbackTx(NeoRepositoryTest.scala:31)

The line numbers are a bit different than the original error with 2.0.3 but it appears to be failing in the same method

@vince-bickers vince-bickers self-assigned this Aug 21, 2016
@vince-bickers
Copy link

Please could you post the relevant code of the failing test method here?

@alankstewart
Copy link
Author

alankstewart commented Aug 22, 2016

Unfortunately there are too many dependencies to post any code that would work and it's also written in Scala. I set a breakpoint in DefaultEntityAccessStrategy.getIdentityPropertyReader when classInfo is null and when it is, classInfo.identityField() thus fails. Perhaps for now, could you put in a null check for classInfo, and at least then I can see what happens afterwards? Thanks

@fbiville
Copy link
Contributor

@alankstewart you can actually fork SDN, check it out at the version you're using, patch it and see for yourself ;-) Also, maybe create a minimal project to reproduce the issue if you cannot post the code as is.

@vince-bickers
Copy link

@alankstewart I'm on holiday at the moment, but I'll take a look at this on Monday, unless another member of the team is able to pick it up beforehand. The primary question in my mind is: why is classinfo null? This is an indication there's something wrong with the project config, but it is baffling that you get the error simply by upgrading.

@alankstewart
Copy link
Author

alankstewart commented Aug 25, 2016

@vince-bickers @fbiville I'm less convinced now that it is an issue with SDN 4.1.2 but with how I've declared dependencies and with the version of neo4j-ogm-core.
When all was working with my tests using SDN 4.1.1, I declared my dependencies like this:

        <dependency>
            <groupId>org.springframework.data</groupId>
            <artifactId>spring-data-neo4j</artifactId>
            <version>4.1.1.RELEASE</version>
        </dependency>
        <dependency>
            <groupId>org.neo4j</groupId>
            <artifactId>neo4j</artifactId>
            <version>3.0.4</version>
        </dependency>
        <dependency>
            <groupId>org.neo4j.test</groupId>
            <artifactId>neo4j-harness</artifactId>
            <version>3.0.4</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.neo4j</groupId>
            <artifactId>neo4j-ogm-test</artifactId>
            <version>2.0.3</version>
            <scope>test</scope>
        </dependency>

neo4j-ogm-core was being brought in transitively by SDN 4.1.1 as 2.0.1. All my code and tests worked fine. When I updated to SDN 4.1.2, the neo4j-ogm-core version became 2.0.3 and the tests failed with the NPE above. To verify this I declared neo4j-ogm-core as a separate dependency with version 2.0.1 and changed the neo4j-ogm-test to 2.0.1 as well, and the tests pass fine with both SDN 4.1.1 and 4.1.2, but fail if I change the neo4j-ogm-core and neo4j-ogm-test versions manually to 2.0.3 with either 4.1.1 or 4.1.2. I tried also neo4j-ogm-core 2.0.2 with the same failures.
Does this information help in any way?

@alankstewart
Copy link
Author

alankstewart commented Aug 25, 2016

I just tried neo4j-ogm-core (along with neo4j-ogm-test) 2.0.2, 2.0.3, 2.0.4, and your latest 2.0.5-SNAPSHOT (with either SDN 4.1.1 or 4.1.2) and all fail with the same NPE. It appears a change happened from 2.0.1 to 2.0.2

@alankstewart
Copy link
Author

Any ideas? Thanks

@mangrish
Copy link
Contributor

mangrish commented Sep 2, 2016

I know @vince-bickers is getting closer to a fix on this @alankstewart. It might take a wee bit longer to work out.

@alankstewart
Copy link
Author

That's great. Thanks

@vince-bickers
Copy link

@alankstewart I've pushed some more fixes to the repository after analysis showed we could have potential bugs, but so far I've not been able to reproduce your issue. With only a stack trace to go on, it's not simple to diagnose.

However, from the stack trace I'm pretty sure the following is happening:

  1. One or more NodeEntities have been saved but not committed in a test
  2. One or more of these NodeEntities has a property mapped to a RelationshipEntity
  3. A rollback has been triggered, either manually, or by Spring's test framework
  4. As part of the rollback process all the ids of the persisted but not committed objects are reset
    and the objects are removed from the mapping context
  5. The NPE is occurring when the mapping context attempts to remove a RelationshipEntity a) as
    a consequence of removing a NodeEntity b) that maintains a property reference to a)
  6. The NPE is caused because the type of this property (the RelationshipEntity) is not known.

At this point, I can't offer much more help without having at sight of some code. I have tested a number of scenarios where I tried to reproduce the NPE according to the above analysis, but have not been able to. This does look like a mapping problem, but I'm not sure it's in our code. One possibility is that your code contains a subtle mapping bug that the change from 2.0.1 to later versions of the OGM exposed.

To take this forward, I'd need to see some code. If you can give me temporary access to your repo and the failing test, or create a minimal project that reproduces the issue and give us access to that, that would be great. Otherwise, I don't think there's much more I can do at the moment.

@alankstewart
Copy link
Author

@vince-bickers I added a stripped down repo under my user called sdn351 and added you as a collaborator. If you run mvn clean install, the tests all pass. Change the neo4j-ogm version to 2.0.2 or above in the properties section at the top of the pom and you will see the tests fail. The test class is NeoRepositoryTest under src/test/scala.

@vince-bickers
Copy link

vince-bickers commented Sep 3, 2016

Thanks, I've found the issue immediately.

Take a look at mappedCodebaseInfo.scala. You declare a relationship here:

@JsonProperty("build_dependencies")
  @Relationship(`type` = "BUILD_DEPENDENCIES")
  private var _buildDependencies: JList[MappedBuildDependency] = _

But the declared type of MappedBuildDependency is this:

@RelationshipEntity(`type` = "BUILD_DEPENDS_ON")
class MappedBuildDependency private() extends GraphRelationship with BuildDependency {

The type names need to be the same. By changing the declared name of MappedBuildDependency to BUILD_DEPENDENCIES, the tests all pass.

In 2.0.1 we looked up classes in ClassInfo by their Java types, rather than their declared relationship names But this was changed to use the explicitly declared relationship name in 2.0.2.

I'm closing this issue now, but please feel free to re-open if you're still having problems.

@vince-bickers
Copy link

Actually, I'm re-opening this for further investigation, because I think it is a bug. You should be able to declare different relationship names using the same underlying type.

@vince-bickers vince-bickers reopened this Sep 3, 2016
@vince-bickers
Copy link

Confirmed as a bug in the rollback code, and fixed. Please try again with the latest snapshot release: 2.0.5-SNAPSHOT.

@alankstewart
Copy link
Author

Thanks @vince-bickers . I can confirm the latest 2.0.5 snapshot works and I also fixed the mapping as directed. I'll use 2.0.4 until I see a released version of 2.0.5. Cheers and thanks again for your time in this.
Alan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs-investigation An issue that has been triaged but needs further investigation
Projects
None yet
Development

No branches or pull requests

4 participants