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.

Sign up for GitHub

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

Jump to bottom

Fix: SQLite causes crash when trying to merge undefined #334

Merged
merged 15 commits into from
Sep 15, 2023
Merged

Fix: SQLite causes crash when trying to merge undefined #334

merged 15 commits into from
Sep 15, 2023

Conversation

chrispader
Copy link
Contributor

@tgolen @marcaaron

Details

When you run Onyx.merge with the value undefined on native platforms, SQLite will crash the app, because we have a NOT NULL constraint for all values in the database.

Proposal

When undefined is passed as the value in Onyx.merge we manually remove the key from storage with Storage.removeItem and remove it from the merge delta.

Related Issues

https://expensify.slack.com/archives/C01GTK53T8Q/p1693404147209609

Automated Tests

Linked PRs

@chrispader chrispader marked this pull request as ready for review September 11, 2023 09:26
@chrispader chrispader requested a review from a team as a code owner September 11, 2023 09:26
@melvin-bot melvin-bot bot requested review from MonilBhavsar and removed request for a team September 11, 2023 09:27
tgolen
tgolen requested changes Sep 11, 2023
View reviewed changes
lib/storage/providers/SQLiteStorage.js Outdated Show resolved Hide resolved
lib/storage/providers/SQLiteStorage.js Outdated Show resolved Hide resolved
@chrispader
Copy link
Contributor Author

@tgolen what's the desired behaviour of Onyx if a key is set to undefined? Previously, setting it to/ merging it with null will remove the key from storage.

Should we also remove the key when setting/merging undefined?

@chrispader chrispader mentioned this pull request Sep 11, 2023
Merged
@chrispader chrispader requested a review from tgolen September 11, 2023 17:22
tgolen
tgolen requested changes Sep 11, 2023
View reviewed changes
lib/Onyx.js Outdated Show resolved Hide resolved
lib/Onyx.js Outdated
if (_.isObject(existingValue) || _.isObject(lastChange)) {
// We want to merge multiple object delates together
// If all of the changes are objects, we can simply merge all of them together
// If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please expand this comment to explain why only the last objects in the batch should be merged? I'm not sure I understand the scenario or what nullish and array values have to do with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1
May be we want to merge the very last value in the batch - irrespective of if it is null or not null

lib/storage/providers/SQLiteStorage.js Outdated Show resolved Hide resolved
lib/utils.js Outdated
@@ -0,0 +1,13 @@
function countObjectsAtEndOfArray(array) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite getting this... what about objects at the beginning or the middle of the array?

@marcaaron
Copy link
Contributor

When undefined is passed as the value in Onyx.merge we manually remove the key from storage with Storage.removeItem and remove it from the merge delta.

I don't think we should do that. Just throw the value out. Probably they wanted to merge something. Not delete it and it just happened to be undefined. I think we should just act like it didn't happen. They will figure out quickly that it's the wrong method to use if they really want to remove a key.

marcaaron
marcaaron requested changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

I think this new behavior is not desirable. I propose we ignore any merging of undefined values. We never said anywhere that you can do this. People might not intentionally be doing this. It is much more likely that a value would be unintentionally undefined than unintentionally null.

@chrispader
Copy link
Contributor Author

I think this new behavior is not desirable. I propose we ignore any merging of undefined values. We never said anywhere that you can do this. People might not intentionally be doing this. It is much more likely that a value would be unintentionally undefined than unintentionally null.

Alright 👍 Makes sense! Just wanted to be on the same page on that. Will instead just set the value to undefined. Setting/merging null will still delete the key though

@greg-schroeder greg-schroeder mentioned this pull request Sep 12, 2023
Closed
6 tasks
@chrispader
Copy link
Contributor Author

Please wait with reviews, fixing tests right now...

@chrispader chrispader mentioned this pull request Sep 12, 2023
Merged
@chrispader
Copy link
Contributor Author

@tgolen @marcaaron i moved the improvements i made to a separate PR, as this became kind of a scope creep already.

#339

@chrispader
Copy link
Contributor Author

This PR now only contains the lines of code needed in SQLiteStorage.js to fix the crash...

tgolen
tgolen previously approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Thanks for decreasing the scope of this!

marcaaron
marcaaron requested changes Sep 12, 2023
View reviewed changes
lib/storage/providers/SQLiteStorage.js Outdated Show resolved Hide resolved
tgolen
tgolen previously approved these changes Sep 13, 2023
View reviewed changes
marcaaron
marcaaron requested changes Sep 13, 2023
View reviewed changes
lib/storage/providers/SQLiteStorage.js Outdated Show resolved Hide resolved
@marcaaron marcaaron merged commit 426177b into Expensify:main Sep 15, 2023
3 checks passed
@ospfranco ospfranco mentioned this pull request Sep 20, 2023
Merged
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@tgolen tgolen tgolen approved these changes

@MonilBhavsar MonilBhavsar Awaiting requested review from MonilBhavsar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrispader @marcaaron @tgolen @MonilBhavsar