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

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

Open
wenley opened this issue Aug 27, 2024 · 3 comments
Open

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

wenley opened this issue Aug 27, 2024 · 3 comments

Comments

@wenley
Copy link

wenley commented Aug 27, 2024

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:

  • Rails v7.1.3.4
  • PostgreSQL v14
  • scenic v1.8.0

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

@djezzzl
Copy link
Owner

djezzzl commented Aug 27, 2024

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
Copy link
Author

wenley commented Aug 27, 2024

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

  • ThreeStateBooleanChecker, since a DB View cannot structurally guarantee a value is not null
  • ColumnPresenceChecker, for the same reason
  • ForeignKeyChecker, since views are not tables, and therefore cannot have Foreign Key checks implemented on them

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
Copy link
Owner

djezzzl commented Oct 10, 2024

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

Would you like to contribute?

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

No branches or pull requests

2 participants