Skip to content

Generated objects onSave are not considered as already processed. #2223

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
Wayne-P opened this issue Apr 12, 2021 · 12 comments
Closed

Generated objects onSave are not considered as already processed. #2223

Wayne-P opened this issue Apr 12, 2021 · 12 comments
Assignees
Labels
type: bug A general bug

Comments

@Wayne-P
Copy link

Wayne-P commented Apr 12, 2021

So looking for a little clarification here on a possible issue.

As first referenced by https://community.neo4j.com/t/kotlin-neo4j-graph-saving/36378/1

Kotlin 1.4.31
Spring Framework Boot 2.4.4
Desktop 4.2.4
also see GitHub - https://github.com/Wayne-P/graph/tree/master

So we have three classes imaginatively labelled A, B & C

@Node ("NODE_A")
data class A constructor(
    @Id @GeneratedValue val id: Long? = null, val aName: String, @Relationship(type = "A_TO_B_REL") val b: B, @Relationship(type = "A_TO_C_REL") val c: C
)
@Node ("NODE_B")
data class B constructor(
    @Id @GeneratedValue val id: Long? = null, val bName:String, @Relationship(type = "B_TO_C_REL") val cList: List<C>
)
@Node("NODE_C")
data class C constructor(
    @Id @GeneratedValue val id: Long? = null, val cName: String
)

So, roughly,
A has a B and a C.
B has a list of C .
So far, so good.

The application code ...

@Transactional
@SpringBootApplication
class GraphApplication {
    @Bean
    fun init(aRepository: ARepository, bRepository: BRepository, cRepository: CRepository): CommandLineRunner {
        return CommandLineRunner {
            val c1 = C(cName = "c1")
            val c2 = C(cName = "c2")
            val b1 = B(bName = "b1", cList = listOf(c1, c2))
            val a1 = A(aName = "a1", b = b1, c = c1)

            aRepository.deleteAll()
            bRepository.deleteAll()
            cRepository.deleteAll()

            aRepository.save<A>(a1)
        }
    }
}

fun main(args: Array<String>) {
    runApplication<GraphApplication>(*args)
}

Now curiously, repeated efforts to execute this code give different results.
Sometimes, an IllegalStateExcexception from the save attempt of

java.lang.IllegalStateException: Failed to execute CommandLineRunner
	at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:809) ~[spring-boot-2.4.4.jar:2.4.4]
	at org.springframework.boot.SpringApplication.callRunners(SpringApplication.java:790) ~[spring-boot-2.4.4.jar:2.4.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:333) ~[spring-boot-2.4.4.jar:2.4.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1313) ~[spring-boot-2.4.4.jar:2.4.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1302) ~[spring-boot-2.4.4.jar:2.4.4]
	at aoni.graph.GraphApplicationKt.main(GraphApplication.kt:39) ~[main/:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:566) ~[na:na]
	at org.springframework.boot.devtools.restart.RestartLauncher.run(RestartLauncher.java:49) ~[spring-boot-devtools-2.4.4.jar:2.4.4]
Caused by: java.util.NoSuchElementException: No value present
	at java.base/java.util.Optional.get(Optional.java:148) ~[na:na]

Other times, the code runs without exception to produce two c1 s with different ids.
I would have expected to see a graph with just one c1 ,as opposed to a tree.

dual c1

I appreciate that work has been done in
https://github.com/spring-projects/spring-data-neo4j/issues/2148
to ensure that ids of subcomponents are correctly returned in response to a save, but will this work lead to one c1 being created here ?

Many thanks for considering this.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 12, 2021
@meistermeier meistermeier self-assigned this Apr 14, 2021
@meistermeier meistermeier added status: needs-investigation An issue that has been triaged but needs further investigation and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 14, 2021
meistermeier added a commit that referenced this issue Apr 14, 2021
@meistermeier
Copy link
Collaborator

Thanks for reporting this.
There is now a issue/gh-2223 branch in place that should produce a 6.0.9-GH-2223-SNAPSHOT version in a few minutes.
It would be good to get your feedback on this. It looks, at least to me, that the problems, either exception or too much nodes, is gone with this. The change is still a WIP (only imperative, needs some cleaning) but I wanted to get you into the feedback loop as fast as possible.
To use this, you would have to add

repositories {
  mavenCentral()
  maven { url = uri("https://repo.spring.io/milestone") }
  maven { url = uri("https://repo.spring.io/snapshot") }
}

to your repositories and

implementation("org.springframework.data:spring-data-neo4j:6.0.9-GH-2223-SNAPSHOT")

@meistermeier meistermeier added blocked: awaiting feedback and removed status: needs-investigation An issue that has been triaged but needs further investigation labels Apr 14, 2021
@Wayne-P
Copy link
Author

Wayne-P commented Apr 14, 2021

Thanks, I have run this 7 times now and on each occasion, it provided me with better results !

6 0 9-GH-2223-SNAPSHOT

@Wayne-P
Copy link
Author

Wayne-P commented Apr 14, 2021

I can feed this snapshot into a more challenging graph this evening and will let you know tonight whether that works.

@meistermeier
Copy link
Collaborator

Thanks you very much for the fast feedback.
It might be still rough on the edges but yes, go ahead and stress it ;)

@Wayne-P
Copy link
Author

Wayne-P commented Apr 14, 2021

Okay, I gave this a slightly more extreme test and (a) the saves did not throw an exception and (b) the resulting node displays looked fine in the neo4j desktop application.
I assume that the saves are still not expected to return all of the hydrated ids, as of this snapshot ?

@meistermeier
Copy link
Collaborator

(yet another) great feedback, thanks.
Yes, the id hydration is not yet finished for this fix but at least now I am sure that the direction is the right one and I have not misinterpreted the initial cause for the bug.

@meistermeier
Copy link
Collaborator

I noticed that we had the id hydration or, to be correct, the possible re-creation of objects and collections already in 6.1 completed. For me this issue was now a good reason to port it back into the 6.0 line.
I think that this test case represents what you are expecting, right? a0adf09#diff-632a7e8c36411ebb61de39aafeab742126002813a4db0bcbd942e3e24b333ca7R314
I would be very happy if you could checkout the latest snapshot of this branch and give us feedback (again).

@Wayne-P
Copy link
Author

Wayne-P commented Apr 15, 2021

Sure, did you mean the latest snapshot of 6.0.9-GH-2223

@meistermeier
Copy link
Collaborator

Exactly. Still nothing I would rate as production ready yet but at least should give the desired ids back.

@Wayne-P
Copy link
Author

Wayne-P commented Apr 15, 2021

Well on the positive front, I can see the ids, so that is good.
On the negative front, I have come across an occasional exception.
In this case, I suspect the exception is occasional, as on one of the tests I use random data.
I will need to investigate further and I will get back to you later this evening, as I am not available tomorrow.

@Wayne-P
Copy link
Author

Wayne-P commented Apr 15, 2021

Okay, I think it may be struggling with empty lists.
I have a Larder node.
N.B. Serving is also a node.

@Node(node_larder)
data class Larder constructor(
    @Id @GeneratedValue override val id: Long? = null,
    val localDate: LocalDate,
    @Relationship(type = rel_larder_to_serving) var servingList: List<Serving>,
    override val timestamp: Instant = Instant.now(),
    override val userId: String
) : UserEntityAbst<Larder>

I am genuinely not sure why I have set the servingList as a var here, as opposed to a val.
Anyway, my best guess is that when I try to save a Larder with an empty servingList, I get a

Caused by: java.lang.NullPointerException: Parameter specified as non-null is null: 

against the servingList, when the code attempts to instantiate the Larder.

Apologies, as I do not have the time today for a more detailed, or for that matter confident, analysis.
If you believe that this case should work, I can resume reviewing the matter over the weekend.

@meistermeier
Copy link
Collaborator

Thanks. I will probably also look at this Monday again, so no hurry and thanks for the feedback.

meistermeier added a commit that referenced this issue Apr 19, 2021
@meistermeier meistermeier changed the title Kotlin save anomaly - crashes or tree generated when graph expected Generated objects onSave are not considered as already processed. Apr 19, 2021
@meistermeier meistermeier added this to the 6.0.9 (2020.0.9) milestone Apr 19, 2021
meistermeier added a commit that referenced this issue Apr 19, 2021
onSave immutable entities will get re-created if they have generated
ids. In those cases if a node gets referenced more than once (e.g.
A->B->C and A->C) the resulting entities will not be considered as the
same because one has already an identifier set, the other not yet.
In thoses cases the logic either persisted a duplicate or it could not
find the already processed entity because the identifier of the entity
in question is still `null`.

Closes #2223
@michael-simons michael-simons added type: bug A general bug and removed bug labels Apr 20, 2021
meistermeier added a commit that referenced this issue Apr 20, 2021
onSave immutable entities will get re-created if they have generated
ids. In those cases if a node gets referenced more than once (e.g.
A->B->C and A->C) the resulting entities will not be considered as the
same because one has already an identifier set, the other not yet.
In thoses cases the logic either persisted a duplicate or it could not
find the already processed entity because the identifier of the entity
in question is still `null`.

Closes #2223
meistermeier added a commit that referenced this issue Apr 20, 2021
onSave immutable entities will get re-created if they have generated
ids. In those cases if a node gets referenced more than once (e.g.
A->B->C and A->C) the resulting entities will not be considered as the
same because one has already an identifier set, the other not yet.
In thoses cases the logic either persisted a duplicate or it could not
find the already processed entity because the identifier of the entity
in question is still `null`.

Closes #2223
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

No branches or pull requests

4 participants