Skip to content

fix: core data crash with inverse attribute - #519

Merged
volodymyr-chekyrta merged 3 commits into
openedx:developfrom
edx:upstream-develop-crashfix-3
Sep 30, 2024
Merged

fix: core data crash with inverse attribute#519
volodymyr-chekyrta merged 3 commits into
openedx:developfrom
edx:upstream-develop-crashfix-3

Conversation

@rnr

@rnr rnr commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

This PR fixes crash:

IMG_7707

@volodymyr-chekyrta

Copy link
Copy Markdown
Contributor

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

@IvanStepanok

Copy link
Copy Markdown
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

rnr commented Sep 12, 2024

Copy link
Copy Markdown
Contributor Author

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

forgotvas commented Sep 18, 2024

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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

rnr commented Sep 25, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Please resolve git conflicts

@rnr

rnr commented Sep 30, 2024

Copy link
Copy Markdown
Contributor Author

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

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Copy link
Copy Markdown
Contributor

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
@rnr
rnr deleted the upstream-develop-crashfix-3 branch October 7, 2024 10:21
IvanStepanok pushed a commit to raccoongang/openedx-app-ios that referenced this pull request Sep 1, 2025
* fix: core data crash with inverse attribute (openedx#69)

* chore: set deletetionRule as Cascade

---------

Co-authored-by: Vadim Kuznetsov <vadim@touchappmedia.com>
Co-authored-by: Anton Yarmolenko <37253+rnr@users.noreply.github.com>
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