Resolve database issues reported by `database_consistency` gem

[kimadactyl]

Description

I wondered if there was any missing indexes that might be slowing down the responses and discovered Database Consistency gem.

This is reporting quite a list to address:

ThreeStateBooleanChecker fail Supporter is_global boolean column should have NOT NULL constraint
ThreeStateBooleanChecker fail Site is_published boolean column should have NOT NULL constraint
PrimaryKeyTypeChecker fail Partner id column has int/serial type but recommended to have bigint/bigserial/uuid
ThreeStateBooleanChecker fail Partner is_a_place boolean column should have NOT NULL constraint
ThreeStateBooleanChecker fail Partner hidden boolean column should have NOT NULL constraint
ThreeStateBooleanChecker fail Partner can_be_assigned_events boolean column should have NOT NULL constraint
NullConstraintChecker fail OrganisationRelationship verb column is required in the database but does not have a validator disallowing nil values
PrimaryKeyTypeChecker fail Event id column has int/serial type but recommended to have bigint/bigserial/uuid
ThreeStateBooleanChecker fail Tag system_tag boolean column should have NOT NULL constraint
PrimaryKeyTypeChecker fail Calendar id column has int/serial type but recommended to have bigint/bigserial/uuid
ThreeStateBooleanChecker fail Article is_draft boolean column should have NOT NULL constraint
PrimaryKeyTypeChecker fail Address id column has int/serial type but recommended to have bigint/bigserial/uuid
PrimaryKeyTypeChecker fail User id column has int/serial type but recommended to have bigint/bigserial/uuid
MissingUniqueIndexChecker fail TagsUser tag_id+user_id model should have proper unique index in the database
MissingUniqueIndexChecker fail SitesSupporter supporter_id+site_id model should have proper unique index in the database
MissingUniqueIndexChecker fail SitesNeighbourhood neighbourhood_id+site_id model should have proper unique index in the database
MissingUniqueIndexChecker fail PartnerTag tag_id+partner_id model should have proper unique index in the database
MissingUniqueIndexChecker fail Partner lower(name) model should have proper unique index in the database
MissingUniqueIndexChecker fail NeighbourhoodsUser neighbourhood_id+user_id model should have proper unique index in the database
MissingUniqueIndexChecker fail Tag name+type model should have proper unique index in the database
MissingUniqueIndexChecker fail Tag slug+type model should have proper unique index in the database
MissingUniqueIndexChecker fail Calendar source model should have proper unique index in the database
MissingUniqueIndexChecker fail ArticleTag tag_id+article_id model should have proper unique index in the database
ForeignKeyChecker fail TagsUser tag should have foreign key in the database
ForeignKeyChecker fail TagsUser user should have foreign key in the database
ForeignKeyChecker fail SitesSupporter supporter should have foreign key in the database
ForeignKeyChecker fail SitesSupporter site should have foreign key in the database
ForeignKeyChecker fail SitesNeighbourhood neighbourhood should have foreign key in the database
ForeignKeyTypeChecker fail SitesNeighbourhood neighbourhood foreign key (neighbourhood_id) with type (integer) doesn't cover primary key (id) with type (bigint). Total grouped offenses: 2
ForeignKeyChecker fail SitesNeighbourhood site should have foreign key in the database
ForeignKeyTypeChecker fail SitesNeighbourhood site foreign key (site_id) with type (integer) doesn't cover primary key (id) with type (bigint). Total grouped offenses: 3
MissingIndexChecker fail Site sites_neighbourhood associated model should have proper unique index in the database
MissingIndexChecker fail Site sites_neighbourhoods associated model should have proper index in the database
ForeignKeyChecker fail PartnerTag partner should have foreign key in the database
ForeignKeyChecker fail PartnerTag tag should have foreign key in the database
MissingIndexChecker fail Partner events associated model should have proper index in the database
ForeignKeyChecker fail NeighbourhoodsUser neighbourhood should have foreign key in the database
ForeignKeyChecker fail NeighbourhoodsUser user should have foreign key in the database
MissingIndexChecker fail Neighbourhood sites_neighbourhoods associated model should have proper index in the database
ForeignKeyCascadeChecker fail Neighbourhood addresses should have foreign key with on_delete: :[:nullify] in the database
ForeignKeyChecker fail ArticleTag article should have foreign key in the database
ForeignKeyChecker fail ArticleTag tag should have foreign key in the database
ForeignKeyChecker fail ArticlePartner article should have foreign key in the database
ForeignKeyChecker fail ArticlePartner partner should have foreign key in the database
MissingIndexChecker fail Address events associated model should have proper index in the database
MissingIndexChecker fail User sites associated model should have proper index in the database
ForeignKeyTypeChecker fail User sites association (sites) of class (User) relies on field (site_admin) of table (sites) but it is missing
ColumnPresenceChecker fail Supporter name column should be required in the database
ColumnPresenceChecker fail SitesTag tag association foreign key column should be required in the database
ColumnPresenceChecker fail SitesTag site association foreign key column should be required in the database
ColumnPresenceChecker fail SitesNeighbourhood neighbourhood association foreign key column should be required in the database
ColumnPresenceChecker fail SitesNeighbourhood site association foreign key column should be required in the database
ColumnPresenceChecker fail Site name column should be required in the database
ColumnPresenceChecker fail Site slug column should be required in the database
ColumnPresenceChecker fail Site url column should be required in the database
ColumnPresenceChecker fail ServiceArea neighbourhood association foreign key column should be required in the database
ColumnPresenceChecker fail ServiceArea partner association foreign key column should be required in the database
ColumnPresenceChecker fail Partner name column should be required in the database
ColumnPresenceChecker fail NeighbourhoodsUser neighbourhood association foreign key column should be required in the database
ColumnPresenceChecker fail NeighbourhoodsUser user association foreign key column should be required in the database
ColumnPresenceChecker fail Event summary column should be required in the database
ColumnPresenceChecker fail Event dtstart column should be required in the database
ColumnPresenceChecker fail Event partner association foreign key column should be required in the database
ColumnPresenceChecker fail Tag name column should be required in the database
ColumnPresenceChecker fail Tag slug column should be required in the database
ColumnPresenceChecker fail Tag type column should be required in the database
ColumnPresenceChecker fail Calendar name column should be required in the database
ColumnPresenceChecker fail Calendar partner association foreign key column should be required in the database
ColumnPresenceChecker fail Calendar source column should be required in the database
ColumnPresenceChecker fail ArticleTag article association foreign key column should be required in the database
ColumnPresenceChecker fail ArticleTag tag association foreign key column should be required in the database
ColumnPresenceChecker fail ArticlePartner article association foreign key column should be required in the database
ColumnPresenceChecker fail ArticlePartner partner association foreign key column should be required in the database
ColumnPresenceChecker fail Article title column should be required in the database
ColumnPresenceChecker fail Article body column should be required in the database
ColumnPresenceChecker fail Article author association foreign key column should be required in the database
ColumnPresenceChecker fail Address street_address column should be required in the database
ColumnPresenceChecker fail Address country_code column should be required in the database
ColumnPresenceChecker fail Address postcode column should be required in the database
ColumnPresenceChecker fail User role column should be required in the database
RedundantIndexChecker fail SitesTag index_sites_tags_on_site_id index is redundant as index_sites_tags_on_site_id_and_tag_id covers it
RedundantIndexChecker fail ServiceArea index_service_areas_on_neighbourhood_id index is redundant as index_service_areas_on_neighbourhood_id_and_partner_id covers it
UniqueIndexChecker fail Partner index_partners_on_slug index is unique in the database but do not have uniqueness validator
RedundantIndexChecker fail OrganisationRelationship index_organisation_relationships_on_partner_subject_id index is redundant as unique_organisation_relationship_row covers it
UniqueIndexChecker fail OrganisationRelationship unique_organisation_relationship_row index is unique in the database but do not have uniqueness validator
RedundantIndexChecker fail ArticlePartner index_article_partners_on_article_id index is redundant as index_article_partners_on_article_id_and_partner_id covers it
UniqueIndexChecker fail Article index_articles_on_slug index is unique in the database but do not have uniqueness validator
UniqueIndexChecker fail User index_users_on_invitation_token index is unique in the database but do not have uniqueness validator
UniqueIndexChecker fail User index_users_on_reset_password_token index is unique in the database but do not have uniqueness validator
MissingTableChecker fail CollectionEvent should have a table in the database

$ bundle exec database_consistency | wc -l
91

Motivation

Database inconsistencies slow down responses and make our database less robust.

Acceptance criteria

• [ ] All migrations are created and are reversible
• [ ] CI has a step to run the checker

[kimadactyl]

It has a tool to generate migrations to fix this (bundle exec database_consistency -f) and creates 81 migrations. Running these migrations tho is bringing up errors with the data in the database. This is the first one it hits:

== 20240829173419 ChangePartnersIsAPlaceNullConstraint: migrating =============
-- change_column_null(:partners, :is_a_place, false)
bin/rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::NotNullViolation: ERROR:  column "is_a_place" contains null values
/Users/kim/git/PlaceCal/db/migrate/20240829173419_change_partners_is_a_place_null_constraint.rb:3:in `change'

Caused by:
ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR:  column "is_a_place" contains null values
/Users/kim/git/PlaceCal/db/migrate/20240829173419_change_partners_is_a_place_null_constraint.rb:3:in `change'

Caused by:
PG::NotNullViolation: ERROR:  column "is_a_place" contains null values

Do you think it's important to resolve this @katjam?

It looks like about 90% of migrations run - we could potentially just merge those ones for now?

[katjam]

I think yes, we should fix this. Running the good ones sounds like a great idea.

If we leave it, the problem will get more and more difficult to fix. I wonder if we also want to do some kind of manual audit of the production DB and clean up stuff but I appreciate this is a big can of worms.

[katjam]

I think there have been a lot of manual commands run on the DB over the years which has probably left it in a bit of a state.

[kimadactyl]

So the migrations that are failing seem to be a lot of ones where the column state is currently nil and its a boolean. I think if we fix that it'll fix the underlying db - but migrating all the nils to false on the given example is making half the tests fail 😢

Chucked it on the next milestone anyway. I think we can add a CI step to stop it getting any worse as well.

[kimadactyl]

Only 5 not running actually. Not bad at all.

/Users/kim/git/PlaceCal/db/migrate/20240903131833_change_partners_is_a_place_null_constraint.rb
/Users/kim/git/PlaceCal/db/migrate/20240903131860_add_sites_neighbourhoods_site_id_index.rb
/Users/kim/git/PlaceCal/db/migrate/20240903131872_add_sites_site_admin_index.rb
/Users/kim/git/PlaceCal/db/migrate/20240903131876_change_sites_neighbourhoods_neighbourhood_id_null_constraint.rb
/Users/kim/git/PlaceCal/db/migrate/20240903131905_change_users_role_null_constraint.rb

Rubocop is complaning about some of the migrations being irreversable, though so I can't check it in:

db/migrate/20240903131832_change_partners_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :partners, :id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131836_change_events_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :events, :id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131838_change_calendars_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :calendars, :id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131840_change_addresses_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :addresses, :id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131841_change_users_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :users, :id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131857_change_sites_neighbourhoods_neighbourhood_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :sites_neighbourhoods, :neighbourhood_id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131859_change_sites_neighbourhoods_site_id_to_bigint.rb:5:5: C: Rails/ReversibleMigration: change_column is not reversible.
    change_column :sites_neighbourhoods, :site_id, :bigint
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131906_remove_index_sites_tags_on_site_id_index.rb:5:5: C: Rails/ReversibleMigration: remove_index(without column) is not reversible.
    remove_index nil, name: 'index_sites_tags_on_site_id'
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131907_remove_index_service_areas_on_neighbourhood_id_index.rb:5:5: C: Rails/ReversibleMigration: remove_index(without column) is not reversible.
    remove_index nil, name: 'index_service_areas_on_neighbourhood_id'
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131908_remove_index_organisation_relationships_on_partner_subject_id_index.rb:5:5: C: Rails/ReversibleMigration: remove_index(without column) is not reversible.
    remove_index nil, name: 'index_organisation_relationships_on_partner_subject_id'
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20240903131909_remove_index_article_partners_on_article_id_index.rb:5:5: C: Rails/ReversibleMigration: remove_index(without column) is not reversible.
    remove_index nil, name: 'index_article_partners_on_article_id'
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Do we care about this @katjam? I can rewrite them to be reversible but i dont know what a good use of my time that is? My pref would prob be to just have rubocop ignore the database migrations directory, its autogenerated for the most part and feels like one library fighting another.

[katjam]

I do tend to make reversible migrations so they can be tested, deployed and stepped back incrementally. It's the only way to maintain integrity if something unexpected happens, especially when working across environs with different data sets populating them. Not providing down method relies on assumption that nothing will go wrong and that probably is not a sensible assumption.

Having said that - under our current circumstances with very few users actually depending on the service and nothing critical, it may be a use of our time right now. Though I think increasingly, we do have a responsibility to Trans Dimension and Tipping Point audiences.

[kimadactyl]

Totally fair enough. It prob won't take that long, prob just being lazy here. And you're right if it fucks up when deploying fixing it would be an even bigger headache.

[kimadactyl]

We have about 1,000 visitors a month atm across transdim+placecal so i don't think it's fair to say we have very few users any more as well!