fix(test): fk always valid #290
Conversation
This fixes the test `#test_all_foreign_keys_valid_having_foreign_keys_in_multiple_schemas`.
BuonOmo
force-pushed
the
fix-all-fk-valid
branch
from
f71d878 to
cdcfa78
Compare
- Allow using various schemas for the console, and always start with a clean slate. - If no internet, select latest used Rails version. - Allow to easily show schema loading logs. - Closer CRDB configuration between CI and local dev. - Add editor config file.
BuonOmo
force-pushed
the
fix-all-fk-valid
branch
from
cdcfa78 to
4c31016
Compare
| # referential integrity (e.g: adding a foreign key with invalid data | ||
| # raises). | ||
| # So foreign keys should always be valid for that matter. | ||
| def all_foreign_keys_valid? |
There was a problem hiding this comment.
I noticed that this method may be deprecated: https://github.com/rails/rails/blob/4b8cdef21054aaf6d23a2d2f90418b58e4d0c8ff/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L654-L661
I wonder if it's better to avoid using it. It seems like maybe one other option is to override check_all_foreign_keys_valid instead. What do you think?
Or another thing might be the config.active_record.verify_foreign_keys_for_fixtures option, which looks it will disable these checks in the first place https://github.com/rails/rails/blob/4b8cdef21054aaf6d23a2d2f90418b58e4d0c8ff/guides/source/configuring.md?plain=1#L1286. It's referenced here.
If we do keep this method, I'm not sure I understand this - does this mean that the adapter will always consider all foreign keys to be valid? Could it be changed so that all_foreign_keys_valid? only returns true if disable_referential_integrity was called?
There was a problem hiding this comment.
@rafiss nice find! It seems like shipping with next AR minor.
I'll keep this one for now, and when shipping next AR, update check_all_foreign_keys_valid! to be sure we don't check anything.
If we do keep this method, I'm not sure I understand this - does this mean that the adapter will always consider all foreign keys to be valid? Could it be changed so that all_foreign_keys_valid? only returns true if disable_referential_integrity was called?
The idea is to use the same method as the abstract one, I could make it more obvious by using this bit of code:
def self.included(base)
# Getting rid of PG specific check as in CRDB there are no trigger,
# and thus no such thing as an invalid foreign key.
base.remove_method(:check_all_foreign_keys_valid!)About the option, I think the documentation is correct: this is PG/sqlite specific!
There was a problem hiding this comment.
# Getting rid of PG specific check as in CRDB there are no trigger, # and thus no such thing as an invalid foreign key.
That's not quite accurate - CRDB does have the concept of an invalid foreign key, it's just that they are not implemented using triggers. (I know you are on vacation now, but just leaving this note to keep track of this discussion.)
There was a problem hiding this comment.
@rafiss still available a bit :)
Feel free to change the comment. I extrapolated from what I saw: it is not possible using the same method disable_referential_integrity to generate the same incoherence in the DB.
There was a problem hiding this comment.
ah yes that does seem correct - CRDB doesn't have a way to "turn off" referential integrity checks other than just removing the foreign key constraint entirely. i think we can leave things as they are.
This fixes the test
#test_all_foreign_keys_valid_having_foreign_keys_in_multiple_schemas.@rafiss this makes it that we do not need the query to validate referential integrity (see slack).