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

Halfbaked implementation of access to deleted object keys #5081

Closed
wants to merge 1 commit into from

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Nov 7, 2022

What, How & Why?

This is an attempt to show how the issue explained in realm-core#5220 could be solved. My knowledge on how things are handled in cachedCollectio.ts is zero, so please bare with me.

To be used as a basis for further discussion.

This closes # ???

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated

@cla-bot cla-bot bot added the cla: yes label Nov 7, 2022
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, there will now be an additional deletedObjects array sent to the change listener, which contains all the objectKeys which have been removed from the current collection.

I think this would solve the current problem, but it's a bit, as you put it, halfbaked. I think it would be a cleaner interface if the arguments to the listener were:
(listenerCollection, changesByIndex, changesByObjectKey)

The changesByObjectKey would then adhere to the same structure as changesByIndex (with records: insertions, newModifications, deletions). This would increase flexibility of the listeners.

Another possibility would be a breaking change to changes which had a different structure:

{ 
changes:{
  byIndex: {
    insertions: [...], // array of indexes
    newModifications: [...],
    deletions: [...]
  },
  byObjectKey: {
    insertions: [...], //array of objectKeys
    newModifications: [...],
    deletions: [...]
  },
//deprecated
insertions: [...],
newModifications: [...],
deletions: [...]
}

@jedelbo
Copy link
Contributor Author

jedelbo commented Nov 8, 2022

I am not really sure what the interface to the application is. My assumption was that this was more or less defined by the cache. So you should only transfer information to the listenerCallback function than what it required to keep the cache up to date. I am not really sure if the key to the cache is an ObjKey or the primary key value (currently an ObjectId, but potentially a String, Int, UUID). Whatever it is, that should be the information transferred. My PR here just demonstrates how you can get that information in the before callback.

@takameyer
Copy link
Contributor

We are currently using the ObjectKey. I can expand on this PR to shape the user facing interface into what makes sense. Thanks for taking the first steps!

@leon-wbr
Copy link

How would I go about merging this with the most recent version of realm-js? I'm not good with C++, but I desperately need access to the deleted object keys (or the deleted objects).

It seems like implementing this would solve my problem, so I would like to work on it but don't know where to start.

I'm a bit lost as to where the CallbackWrapper is now. It seems that there needs to be changes to realm/object-store? I don't quite understand how the collection data (listenerCollection) is passed to cachedCollection and where I would add the additional attributes. Is it in CollectionChangeCallback?

@jedelbo
Copy link
Contributor Author

jedelbo commented Sep 20, 2023

@leon-wbr Actually we found out that this was not needed after all. Unfortunately I can't remember the details.

@jedelbo jedelbo closed this Sep 20, 2023
@jedelbo jedelbo deleted the je/deleted_object-keys branch September 20, 2023 13:23
@leon-wbr
Copy link

That is unfortunate, because it would help solve #783, which is my problem. I'd like to implement a change listener like onDelete for every time an object is deleted, similar to what is possible in Ruby on Rails' ORM. It seems this is one of the only limited factors.

@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.

3 participants