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

Fix crashes if the query has undefined as value #707

Closed
wants to merge 1 commit into from

Conversation

leninlin
Copy link
Contributor

Hi!

For example, I have lines like these

const enhance = withObservables(['noteId'], ({ noteId }) => ({
  images: database.collections.get('note_images').query(Q.where('note_id', noteId)),
}));

noteId may be undefined at some points in time. I understand that this is bad and I fixed it, but in this case I get an unpleasant crash.
Fetching from the database itself works well with undefined, but when you change any record in this table, the application dies. The action is in one place, but the error is thrown from another place. This moment is hard to find and debug.
Therefore, I propose to do the same behavior for the fetch and for observe, removing the throwing error.
For example, in the sqlite adapter, undefined is set to null. And on observe, undefined == null // => true

@@ -30,7 +30,7 @@ const encodeWhereDescription: WhereDescription => Matcher<*> = description => ra
} else if (compRight.column) {
right = (rawRecord: Object)[compRight.column]
} else {
throw new Error('Invalid comparisonRight')
right = undefined
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 ok with allowing undefined, but this is not the correct solution. compRight.value !== undefined ought to be modified so that it checks for a property's presence, and not value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I will correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leninlin Hey, you there? do you want to finish up this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. I forgot. I will finish this.

@radex
Copy link
Collaborator

radex commented Jun 1, 2020

please also add a CHANGELOG note

@radex
Copy link
Collaborator

radex commented Oct 29, 2021

Made redundant by #1198

@radex radex closed this Oct 29, 2021
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.

2 participants