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

Support upserts #668

Merged
merged 19 commits into from
Aug 16, 2022
Merged

Support upserts #668

merged 19 commits into from
Aug 16, 2022

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Jun 13, 2022

Allows to upsert object graphs with Realm.add(x, update:true) (and Realm.addAll([x,y], update:true))

Resolves: #648

@cla-bot cla-bot bot added the cla: yes label Jun 13, 2022
@nielsenko nielsenko marked this pull request as draft June 13, 2022 14:46
@nielsenko nielsenko marked this pull request as ready for review June 14, 2022 06:06
@nielsenko nielsenko self-assigned this Jun 14, 2022
lib/src/realm_class.dart Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
flutter/realm_flutter/tests/test_driver/realm_test.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
lib/src/realm_object.dart Outdated Show resolved Hide resolved
test/add_or_update_test.dart Outdated Show resolved Hide resolved
flutter/realm_flutter/tests/test_driver/realm_test.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Show resolved Hide resolved
lib/src/realm_object.dart Outdated Show resolved Hide resolved
@blagoev
Copy link
Contributor

blagoev commented Jun 17, 2022

There is some work in Core about this realm/realm-core#5516

@nielsenko
Copy link
Contributor Author

There is some work in Core about this realm/realm-core#5516

This is still in draft. I suggest we update our code once it lands.

@nielsenko nielsenko force-pushed the kn/upserts branch 2 times, most recently from c928512 to 14ba9bc Compare August 15, 2022 15:11
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

The code seems fine to me, but I think we could improve the test coverage of that feature.

Off the top of my head, some tests that are missing:

  1. A test that upserting an object with a collection different from the original collection is clearing the old collection before populating it with the new items.
  2. A test that upserting an object with an empty collection clears the old collection.
  3. Tests that mix and match managed and unmanaged objects.
  4. A test that upserting an object with a collection where the items of the collection have changes, those changes get applied to the original objects.

test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
@nielsenko
Copy link
Contributor Author

The code seems fine to me, but I think we could improve the test coverage of that feature.

Off the top of my head, some tests that are missing:

  1. A test that upserting an object with a collection different from the original collection is clearing the old collection before populating it with the new items.

is tested in realm_test.dart#715

  1. A test that upserting an object with an empty collection clears the old collection.

isn't that implied by 1) , ie. [bob, carol] updated to [dan]? But yes, I can add a test for that specifically.

  1. Tests that mix and match managed and unmanaged objects.

Doesn't the realm_test.dart#659-668 cover this? The Friends are updated through a Party (that has no PK).

  1. A test that upserting an object with a collection where the items of the collection have changes, those changes get applied to the original objects.

Isn't this tested in realm_test.dart#664-665?

@nirinchev
Copy link
Member

You're right - I misread some of what was going on with the party test.

Re:
2. It is implied, but I think an empty collection is somewhat special, so it deserves an explicit test.
3. Yes, that covers some of it, but I think it's a bit simple. Looking at the code, I don't think there are bugs, but it's a good idea to guard against refactoring by devising a more complex hierarchy being upserted. Something like:

@RealmModel()
class _ObjWithPK {
  @PrimaryKey()
  late int id;

  late _ObjWithPK linkWithPK;
  late List<_ObjWithPK> listWithPK;

  late _ObjWithoutPK linkWithoutPK;
  late List<_ObjWithoutPK> listWithoutPK;
}

@RealmModel()
class _ObjWithoutPK {
  late _ObjWithPK linkWithPK;
  late List<_ObjWithPK> listWithPK;

  late _ObjWithoutPK linkWithoutPK;
  late List<_ObjWithoutPK> listWithoutPK;
}

And test that a more complex graph with cycles is getting correctly inserted. Again, I don't think there's a bug in the code, but I think I can come up with buggy implementations that pass the Party test.

@nielsenko
Copy link
Contributor Author

I have gathered all test in one test case about friends and parties, with an easy to follow narrative.

nielsenko and others added 2 commits August 16, 2022 14:18
Co-authored-by: Nikola Irinchev <irinchev@me.com>
@nielsenko nielsenko merged commit d67c67b into master Aug 16, 2022
@nielsenko nielsenko deleted the kn/upserts branch August 16, 2022 12:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Realm.add(update:[bool])?
4 participants