Inconsistent cycle detection when saving multiple objects causing RuntimeException

shlusiak commented 3 years ago

Parse version: 1.25.0

java.lang.RuntimeException: Unable to save a ParseObject with a relation to a cycle.
        at com.parse.ParseObject$12.then(ParseObject.java:733)
        at com.parse.ParseObject$12.then(ParseObject.java:713)
        at com.parse.boltsinternal.Task$15.run(Task.java:907)
        at com.parse.boltsinternal.BoltsExecutors$ImmediateExecutor.execute(BoltsExecutors.java:113)
        at com.parse.boltsinternal.Task.completeAfterTask(Task.java:898)
        at com.parse.boltsinternal.Task.continueWithTask(Task.java:713)
        at com.parse.boltsinternal.Task.continueWithTask(Task.java:724)
        at com.parse.boltsinternal.Task$13.then(Task.java:816)
        at com.parse.boltsinternal.Task$13.then(Task.java:804)
        at com.parse.boltsinternal.Task$15.run(Task.java:907)
        at com.parse.boltsinternal.BoltsExecutors$ImmediateExecutor.execute(BoltsExecutors.java:113)

Where this is thrown there is a comment that this should never actually happen:

https://github.com/parse-community/Parse-SDK-Android/blob/797786ff42223094c20d2dadd7af9e616020175a/parse/src/main/java/com/parse/ParseObject.java#L728-L732

Sample test case that triggers this

val parent = ParseObject("parent")
parent.save()
// parent now has an objectId

val child1 = ParseObject("child")
child1.put("parent", parent)

val child2 = ParseObject("child")
child2.put("parent", parent)

// both children are new and point to the parent, which points back at each.
// this is a cycle, but not a cycle that cannot be resolved by saving the objects in two batches
parent.put("children", listOf(child1, child2)
parent.save()

Expected behaviour

This should succeed, because child1 and child2 can be persisted first, and once they both have an objectId parent can be persisted to resolve the cycle.

Actual behaviour

java.lang.RuntimeException: Unable to save a ParseObject with a relation to a cycle.

This setup does contain a cycle but one that can be resolved because the parent has an objectId. The logic in collectDirtyChildren only looks for direct cycles involving unsaved objects, which cannot be resolved because neither object can be saved first.

In the scenario above the batching code would be expected to serialise and save child1 and child2 in the first iteration, and then parent in the second iteration, once the objectIds of all children are known.

However the canBeSerialized() method is using ParseTraverser, which does a deep traverse of the given object.

https://github.com/parse-community/Parse-SDK-Android/blob/797786ff42223094c20d2dadd7af9e616020175a/parse/src/main/java/com/parse/ParseObject.java#L2905-L2929

This will return false if any node is found that has no objectId, which makes every object in the above scenario un-serializable, because they all contain a node in the tree without an objectId (parent cannot be saved, because child1 is found, child1 cannot be saved because child2 is found, child2 cannot be saved because child1 is found).

I feel what we'd want here is to set setTraverseParseObjects to false to only traverse this object but not do a deep traversal, but the way the ParseTraverser is written this means that ParseObjects will not even be visited. 🙄

azlekov commented 2 years ago

Hey, @shlusiak

It would be great if you can open a PR for this when you have some spare time. I rely on you on this traversal topic as you shown that you have understanding. Feel free to contact me if you have interest for some general rewrite and restructure of the SDK - I'm looking for some help for the next-gen Parse SDK for the JVM.

PS. If this is something already fixed in some of your previous PRs, please let me know to close this.

Thanks!

Looking forward, Asen

shlusiak commented 2 years ago

@L3K0V It has been a while since I encountered this but I remember I had a crack at it and gave up because the ParseTraverser was reused and I couldn't easily change it's behaviour as I needed it in this different scenario. There are obviously quite a lot of cases to consider and it turned complex enough for me to give up after I understood what was happening, and I resorted in our production code to know when this is happening and manually persist in two stages to avoid this crash. So no, I'm not aware of this being fixed yet.

I might spend some time refreshing the context of all this and look at it again. I'm not finding much free time to contribute to this more than my work required and as I implemented a workaround that driver it is harder for me to dedicate some time to it. But that may change and I'll keep this in mind and might try to pick it up again.

Are you thinking of a gradual rewrite of the existing code base, or of a green field approach of building this up from scatch? I would love a cleaner version that gets rid of the Bolts-Tasks dependency and uses coroutines instead, there are a lot of places where the current design is almost impossible to just "fix" (see local data store performance).

azlekov commented 2 years ago

@shlusiak I have started a discussion here https://github.com/parse-community/Parse-SDK-Android/issues/1100 and define some plan. Next steps for me should be:

Migrate to Kotlin
Remove bolts and replace them with coroutines
- Remove local storage and provide extensible storage interfaces, so everybody can implement their own storage - Room, ObjectBox, Kodein-DB, etc.
Create runtime or api module which can be KMM or just for the JVM without bound to Android APIs.
Normalized cache

I have tried several time to rewrite everything to Kotlin but every time I hit some walls. I tried to start from scratch here https://github.com/L3K0V/ParseKt and again I hit some walls. It's not for a person alone, so I'm looking for someone to work together on next-gen Parse SDK for the JVM or KMM

parse-community / Parse-SDK-Android