search

djezzzl / database_consistency

The tool to avoid various issues due to inconsistencies and inefficiencies between a database schema and application models.
MIT License
1.02k stars 43 forks source link

ColumnPresenceChecker is incompatible with Rails' new default `belongs_to` validation behavior #215

Closed agrobbin closed 9 months ago

agrobbin commented 10 months ago

Starting in the next major release of Rails, thanks to rails/rails#46522, the validation behavior of belongs_to has changed to include an :if, to only validate if the underlying foreign key attribute has changed. This does not play well with the way ColumnPresenceChecker analyzes whether a particular column is consistent with its AR model:

https://github.com/djezzzl/database_consistency/blob/18e4aec9bb33f71f7de1d61ee906774a10c987b3/lib/database_consistency/checkers/validators_fraction_checkers/column_presence_checker.rb#L56-L68 https://github.com/djezzzl/database_consistency/blob/18e4aec9bb33f71f7de1d61ee906774a10c987b3/lib/database_consistency/checkers/validators_fraction_checkers/column_presence_checker.rb#L36-L38 https://github.com/djezzzl/database_consistency/blob/18e4aec9bb33f71f7de1d61ee906774a10c987b3/lib/database_consistency/checkers/validators_fraction_checkers/column_presence_checker.rb#L7

I'm not sure off-hand what the ideal change is here to make this continue to work, but wanted to raise the issue now while the next major release of Rails is still in active development!

chaadow commented 9 months ago

HI @djezzzl Unfortunately this checker fails in rails 7.1 ( which was released about a week ago )

chaadow commented 9 months ago

@djezzzl @agrobbin I made this little "quick fix" which made all my belongs_to pass with rails 7.1 defaults https://github.com/chaadow/database_consistency/commit/22707383848851005e910b25dcc18e22262374af

haven't tested backward compatibility. but if you can take a look maybe I can create a PR with this change

agrobbin commented 9 months ago

At first glance it seems like a reasonable guess, but I'll defer to @djezzzl here, as I'm not sure if that's "safe enough".

djezzzl commented 9 months ago

Hi there 👋

First, thank you both for using the gem and reporting the issue!

The idea that @chaadow has suggested seems reasonable here. Even though it may bring false positives (in cases with custom if: :something), at least we will have less noise now. Also, it's worth adding that covering this case is "nice to have" rather than a critical issue (like missing unique indexes).

There is also a way to patch ActiveRecord to "mark" those presence validators with this optimized "if" condition. As the database_consistency runner is expected to run as a separate process on demand, we can do that without affecting other environments (development/production/test, etc). However, maintaining it could be slightly more complicated and unwelcome.

With that said, I would go with this proposal by chaadow.

Would you mind creating a PR? I would be happy to merge it and release it ASAP.

P.S. It would be good to add one more check to your condition that the ActiveRecord's version is 7.1+

P.P.S. And again, thank you, @chaadow and @agrobbin! Sorry for being late.

djezzzl commented 9 months ago

Released in 1.7.21!

Thank you again! Have a great week!