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

Protect against undefined values #228

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Protect against undefined values #228

merged 2 commits into from
Jan 12, 2023

Conversation

tgolen
Copy link
Collaborator

@tgolen tgolen commented Jan 12, 2023

cc @marcaaron @chrispader

Details

Explained in this thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1673550288930829

This caused a crash in the app when clearing Onyx due to one of the values passed to multiSet() being undefined (which is normal behavior for a value that we are trying to clear out).

image

Related Issues

None

Automated Tests

None

Linked PRs

This will be tested and deployed with Expensify/App#13886

@tgolen tgolen self-assigned this Jan 12, 2023
@tgolen tgolen marked this pull request as ready for review January 12, 2023 19:21
@tgolen tgolen requested a review from a team as a code owner January 12, 2023 19:21
@melvin-bot melvin-bot bot requested review from dangrous and removed request for a team January 12, 2023 19:21
marcaaron
marcaaron requested changes Jan 12, 2023
View reviewed changes
lib/storage/providers/SQLiteStorage.js Outdated
@@ -66,7 +66,7 @@ const provider = {
multiSet(pairs) {
const stringifiedPairs = _.map(pairs, pair => [
pair[0],
JSON.stringify(pair[1]),
JSON.stringify(pair[1] || null),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only short circuit to null in the case of an explicitly undefined value? Anything falsy will default to null with what you have here. It might be fun for a while, but will probably lose it's charm quickly 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good call.

@tgolen
Copy link
Collaborator Author

tgolen commented Jan 12, 2023

OK, updated. Here is the output of stringifiedPairs:

[["session", "{\"loading\":false}"], ["account", "{\"errors\":null,\"success\":\"\",\"isLoading\":false}"], ["network", "{\"isOffline\":false}"], ["iou", "{\"loading\":false,\"error\":false,\"creatingIOUTransaction\":false,\"isRetrievingCurrency\":false}"], ["isSidebarLoaded", "false"], ["shouldShowComposeInput", "true"], ["preferredLocale", "\"es\""], ["activeClients", "null"]]

dangrous
dangrous approved these changes Jan 12, 2023
View reviewed changes
Copy link

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

Code makes sense to me! Will leave to @marcaaron to push the button

marcaaron
marcaaron approved these changes Jan 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.

LGTM 👍 thanks for catching this one!

@marcaaron marcaaron merged commit c8736b6 into main Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dangrous dangrous dangrous approved these changes

@marcaaron marcaaron marcaaron approved these changes

Assignees

@tgolen tgolen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tgolen @dangrous @marcaaron