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

Copy EmbeddedObject on initializing an unmanaged object #7301

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

pavel-ship-it
Copy link
Contributor

When creating an unmanaged copy of a managed object with the embedded object property we should make a copy of this embedded object instead of reusing it.

This is fix #6921

@pavel-ship-it pavel-ship-it marked this pull request as draft November 30, 2021 16:40
@cla-bot cla-bot bot added the cla: yes label Jan 25, 2022
@pavel-ship-it pavel-ship-it marked this pull request as ready for review February 16, 2022 17:46
Copy link
Contributor

@dianaafanador3 dianaafanador3 left a comment

Choose a reason for hiding this comment

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

Just a minor comments

RealmSwift/Tests/ObjectCreationTests.swift Outdated Show resolved Hide resolved
RealmSwift/Tests/ObjectCreationTests.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
RealmSwift/Tests/ObjectCreationTests.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@dianaafanador3 dianaafanador3 left a comment

Choose a reason for hiding this comment

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

LGTM, only comment It looks like SwiftLint CI Job is failing, because of format issues.

@tgoyne
Copy link
Member

tgoyne commented Feb 24, 2022

We only want to copy the object if it's managed.

Comment on lines 96 to 97
if (([obj isKindOfClass:cls] && ![(id)cls isEmbedded]) ||
([(id)cls isEmbedded] && [obj respondsToSelector:@selector(realm)] && ![obj realm])) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (([obj isKindOfClass:cls] && ![(id)cls isEmbedded]) ||
([(id)cls isEmbedded] && [obj respondsToSelector:@selector(realm)] && ![obj realm])) {
if ([obj isKindOfClass:cls] && (![(id)cls isEmbedded] || ![obj realm])) {

I think what you have would call isEmbedded on classes that don't implement it and would accept objects of any type as long as they're an unmanaged embedded object. This also points at some missing tests for initializing an unmanaged object with a value that has an object of a different type in an embedded object field (which copies that object into an unmanaged object of the correct type if all of the required fields are present and have the correct values, and throws an exception if it can't).

RealmSwift/Tests/ObjectCreationTests.swift Show resolved Hide resolved
@stoneyMDB stoneyMDB marked this pull request as draft May 17, 2022 15:05
@afsmarques
Copy link

so, no more updates on this? 😕

@dianaafanador3 dianaafanador3 force-pushed the py/copy_embedded_on_add branch 2 times, most recently from 27eee84 to 7af1c9c Compare September 20, 2022 15:16
@dianaafanador3 dianaafanador3 force-pushed the py/copy_embedded_on_add branch 2 times, most recently from d793f69 to 62a7f79 Compare October 11, 2022 08:36
@dianaafanador3 dianaafanador3 force-pushed the py/copy_embedded_on_add branch from 62a7f79 to eaca6da Compare February 27, 2023 15:29
@cla-bot cla-bot bot removed the cla: yes label Feb 27, 2023
@dianaafanador3
Copy link
Contributor

@tgoyne checking this, I think the changes regarding the comments were made, looking at the original issue I think some test are missing what the original intention was, something like the follow.

func testCopyEmbeddedObjectFromManagedObjectInSameRealm() {
        let realm = try! Realm()
        try! realm.write {
            let parent = realm.create(EmbeddedPrimaryParentObject.self, value: [
                "object": ["value": 1],
                "array": [[2]]
            ])

            // Copy Managed object
            let copyA = EmbeddedPrimaryParentObject(value: parent)
            realm.add(copyA, update: .modified)
            XCTAssertEqual(realm.objects(EmbeddedPrimaryParentObject.self).count, 1)

            XCTAssertEqual(parent, copyA)
            XCTAssertEqual(copyA.object!.value, 1)
            XCTAssertEqual(copyA.array.count, 1)

            // Explicit copy of object
            let copyB = EmbeddedPrimaryParentObject()
            copyB.object = EmbeddedTreeObject1(value: parent.object!)
            copyB.array.append(EmbeddedTreeObject1(value: parent.array[0]))
            realm.add(copyB, update: .modified)
            XCTAssertEqual(realm.objects(EmbeddedPrimaryParentObject.self).count, 1)

            XCTAssertEqual(parent, copyB)
            XCTAssertEqual(copyB.object!.value, 1)
            XCTAssertEqual(copyB.array.count, 1)

            // Assign of EmbeddedObject
            let copyC = EmbeddedParentObject()
            copyC.object = parent.object
            assertThrows(realm.add(copyC), "Cannot set a link to an existing managed embedded object")

            let parentUnmanaged = EmbeddedPrimaryParentObject(value: [
                "object": ["value": 4],
                "array": [[5]]
            ])

            // Do not copy unmanaged object
            let copyD = EmbeddedPrimaryParentObject(value: parentUnmanaged)
            XCTAssertTrue(copyD.object === parentUnmanaged.object)
            assertThrows(realm.add(parentUnmanaged), "Cannot set a link to an existing managed embedded object")
        }
    }

Let me know if there are more changes needed on this, or If I should change the current text with the text above.

@realm realm deleted a comment from cla-bot bot Feb 27, 2023
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.

Exception on saving of Unmanaged Object with Embedded Object
4 participants