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

Fixed a bug where == was used to compare a value with nil #89

Closed
wants to merge 1 commit into from

Conversation

jechol
Copy link
Contributor

@jechol jechol commented Jan 20, 2025

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

@zachdaniel
Copy link
Contributor

So, this actually needs to be a bit more complex. We have to check if the identity considers nulls distinct, and if it does then we can do the is_nil() check, otherwise we cannot use that identity for this. Because there can be duplicates in that case.

@jechol
Copy link
Contributor Author

jechol commented Jan 20, 2025

I hadn't considered that case. The reason I became interested in this bug is that ever since filter_for_records was introduced, I kept encountering the message, Comparing values with nil will always return false. Use is_nil/1 instead. In: #{inspect(call)} in certain parts of my application. Please take that into account.

@zachdaniel
Copy link
Contributor

I understand, but this fix will essentially have the same bug just without the warning. Do you have any resources without primary keys?

@jechol
Copy link
Contributor Author

jechol commented Jan 20, 2025

No, all my resources have primary keys.

@zachdaniel
Copy link
Contributor

That represents a more serious issue. Primary keys aren't supposed to be able to be nil. So this code is very likely not working properly for you.

@zachdaniel
Copy link
Contributor

Can you think of a way that you could end up passing records w/o their primary key into the data layer?

@jechol
Copy link
Contributor Author

jechol commented Feb 10, 2025

It looks like it was fixed by a311224, so I'm closing this PR.

@jechol jechol closed this Feb 10, 2025
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