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

fix: core data crash with inverse attribute #519

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

rnr
Copy link
Contributor

@rnr rnr commented Sep 6, 2024

This PR fixes crash:

IMG_7707

@volodymyr-chekyrta
Copy link
Contributor

@IvanStepanok will review this, because deletionRule=“Cascade” was done intentionally to fix a different crash case.

@IvanStepanok
Copy link
Contributor

Hi @rnr, great that you hit on this topic. This problem sometimes haunts the project and I think I have found a solution. We used cascading deletion of the database so that no garbage is left behind when deleting, but we didn't take into account that we need to clearly define the relationships between the database objects. I set up the relationships and it works great! Now when I delete a database due to a conflict, the application does not crash. To test it, I tried to write a function that I run right at the start of the application when creating a context:

      private func createContext() -> NSManagedObjectContext {
        let context = persistentContainer.newBackgroundContext()
        context.automaticallyMergesChangesFromParent = true
        createErrorCausingEntities(context) // <-- CALL HERE
        return context
    }

   func createErrorCausingEntities(_ context: NSManagedObjectContext) {
           
           context.perform {
               context.mergePolicy = NSMergePolicy.mergeByPropertyObjectTrump
               // Create CDPrimaryCourse
               let primaryCourse = CDPrimaryCourse(context: context)
               primaryCourse.name = "Test Course"
               
               // Create CDAssignment without setting up the inverse relationship
               let assignment = CDAssignment(context: context)
               assignment.title = "Test Assignment"
               
               // Attempt to add the assignment to futureAssignments without proper setup
               primaryCourse.futureAssignments = NSSet(array: [assignment])
               
               do {
                   try context.save()
               } catch {
                   print("Error saving context: \(error)")
               }
           }
       }

There is screenshots with all relationships settings. You can try it:

Screenshot 2024-09-10 at 08 58 41 Screenshot 2024-09-10 at 08 58 48 Screenshot 2024-09-10 at 08 58 21 Screenshot 2024-09-10 at 08 58 29

I hope this solution will fix this crash🙏

@rnr
Copy link
Contributor Author

rnr commented Sep 12, 2024

Thank you @IvanStepanok !
I tried this and looks good. Lets wait opinion from @forgotvas (he had fixed this crash)

@rnr rnr requested a review from forgotvas September 12, 2024 17:32
@forgotvas
Copy link
Contributor

forgotvas commented Sep 18, 2024

hi @IvanStepanok, yes main point is to add relationship between CDPrimaryCourse and CDAssignment, Nulify rule was committed by my fault, so our fix should work fine with Cascade rule too. Like you suggested in your snapshot, but you did use new fields for relationship.

Can you provide example what crash fixed by Cascade rule?

@IvanStepanok
Copy link
Contributor

Hi @forgotvas

We added a cascading deletion rule to fix an issue where reloading CDPrimaryCourse did not overwrite nested objects properly. By using "Cascade," related entities (like CDAssignment) are now deleted alongside their parent, ensuring that when reloading, new data can be saved without conflicts from residual old data.

@forgotvas
Copy link
Contributor

Hi @forgotvas

We added a cascading deletion rule to fix an issue where reloading CDPrimaryCourse did not overwrite nested objects properly. By using "Cascade," related entities (like CDAssignment) are now deleted alongside their parent, ensuring that when reloading, new data can be saved without conflicts from residual old data.

do you have steps to reproduce that crash, want to try to repeat it in my branch with my fix.

@IvanStepanok
Copy link
Contributor

do you have steps to reproduce that crash, want to try to repeat it in my branch with my fix.

As I remember, you need a really large course to reproduce this error. On the first attempt, caching works fine, but on the second time, there's a cascade of core data errors. Nothing special, the app still works, but the cache is not updated.

@rnr
Copy link
Contributor Author

rnr commented Sep 25, 2024

So @IvanStepanok @forgotvas
As I see there is no difference between your fixes - just Ivan creates separate Relationships to relates entities but Vadim adds inverseEntity field. Is this right? Then lets select one of these approaches and move forward. WDYT?

@volodymyr-chekyrta
Copy link
Contributor

Please resolve git conflicts

@rnr
Copy link
Contributor Author

rnr commented Sep 30, 2024

Conflict is resolved
Also changed deletionRules to 'Cascade' as @IvanStepanok suggested

Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

LGTM

@volodymyr-chekyrta volodymyr-chekyrta merged commit 2433b22 into openedx:develop Sep 30, 2024
6 checks passed
@rnr rnr deleted the upstream-develop-crashfix-3 branch October 7, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants