Avoid false positives on case-insensitive unique indexes when using CITEXT fields #168
Conversation
|
Please, make sure, that commits are squashed into one. |
Hi @fatkodima - apologies if I'm misunderstanding -- but won't you merge my PR in via a squash once it's ready? I don't know yet if this PR needs any more fixes or amendments either. |
test/active_record_doctor/detectors/missing_unique_indexes_test.rb
Outdated
Show resolved
Hide resolved
test/active_record_doctor/detectors/missing_unique_indexes_test.rb
Outdated
Show resolved
Hide resolved
| @@ -66,6 +66,7 @@ def validations_without_indexes | |||
| columns[-1] = "lower(#{columns[-1]})" unless case_sensitive | |||
|
|
|||
| next if unique_index?(model.table_name, columns) | |||
| next if model.columns_hash[attribute.to_s]&.type == :citext # citext is case-insensitive by default | |||
There was a problem hiding this comment.
Two questions:
- Why is
&.needed here? - If we change
&.to just., will the test suite fail?
There was a problem hiding this comment.
Hey @gregnavis - It failed on a few tests so I added it in.
Do you think there's possibly a less inelegant way to handle this? I'm not a super fan of the safe-navigation operator myself, and it's normally indicative of a code smell of some sort, but I didn't see a way around it without making the code really contort itself to avoid it.
Let me know if you've got a preferred way to not use &.
There was a problem hiding this comment.
hey @gregnavis - checking in to see if you're happy with my response on ☝🏻 and if there's any other changes you'd like to see?
This should resolve #167
Hi there @gregnavis and @fatkodima
Please take a look at this PR and let me know if you need any changes. I wasn't able to run tests locally, so I took my best stab at getting them to work. I'll fix them if they break on CI.
Thanks so much for this great gem - I'm happy to have been able to contribute, even in this tiny way :)