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.06k stars 46 forks source link

Unexpected behavior changes for View-based models in v1.7.25 #236

Open wenley opened 2 months ago

wenley commented 2 months ago

When upgrading from v1.7.24 to v1.7.25, my team experienced a bunch of new violations flagged, specifically for our ActiveRecord models that are based on DB Views. This happened both for materialized and not-materialized Views.

Examples of newly flagged violations:

ThreeStateBooleanChecker fail SaleLineItem production boolean column should have NOT NULL constraint
PrimaryKeyTypeChecker fail Sale uid column has int/serial type but recommended to have bigint/bigserial/uuid
ThreeStateBooleanChecker fail Sale production boolean column should have NOT NULL constraint
MissingUniqueIndexChecker fail Sale uid model should have proper unique index in the database
ForeignKeyChecker fail SaleLineItem sale should have foreign key in the database
ForeignKeyChecker fail SaleLineItem order should have foreign key in the database
ForeignKeyChecker fail SaleLineItem supplier_organization should have foreign key in the database
ForeignKeyChecker fail SaleLineItem consumer_organization should have foreign key in the database
ForeignKeyChecker fail Sale order should have foreign key in the database
ForeignKeyChecker fail Sale supplier_organization should have foreign key in the database
ColumnPresenceChecker fail SaleLineItem sale association foreign key column should be required in the database
ColumnPresenceChecker fail SaleLineItem order association foreign key column should be required in the database
ColumnPresenceChecker fail SaleLineItem supplier_organization association foreign key column should be required in the database
ColumnPresenceChecker fail SaleLineItem consumer_organization association foreign key column should be required in the database
ColumnPresenceChecker fail Sale uid column should be required in the database
ColumnPresenceChecker fail Sale order association foreign key column should be required in the database
ColumnPresenceChecker fail Sale supplier_organization association foreign key column should be required in the database

To be clear, I don't think these violations are necessarily incorrect to flag, but given the upgrade was only a patch version, we were surprised to see a bunch of new violations. We can work around this by disabling checks on these models and then fixing the issue incrementally.

As an FYI, we are using the scenic gem to help us manage the DB views, but I don't think that's relevant to the behavior change here.

We are running our Rails application with:

Let me know if there's any other info I can provide to help debug + test!

djezzzl commented 2 months ago

Hi! Thank you for letting us know!

We have changed the way we select models https://github.com/djezzzl/database_consistency/pull/232/files#diff-75ea970610cc52093dc977330caee9fe665e2a18c27de911808990d5d4ae4906R42

Could you please check the validity of the issues raised? If they are not valid, we probably should stop checking views and/or materialized views.

wenley commented 2 months ago

I tried my best, but the following seem not possible to implement for a view (at least for PostgreSQL):

I would not be surprised if those also cannot be implemented for other SQL dialects.

Of the checks I listed above, MissingUniqueIndexChecker correctly flagged an issue (yay and thank you!)

Interestingly, PrimaryKeyTypeChecker was reporting a false positive, or at least an odd error message. We are using the result of an md5(...) computation for the sales.uid column, which has an output type of text and not any numeric type.

djezzzl commented 3 weeks ago

Thank you! So, we should disable some of the checks for views.

Would you like to contribute?