Skip to content

Check enums for _all_ defined models in development in specs

Peter Leitzen requested to merge pl-db-schema-spec-fix into master

What does this MR do and why?

Previously, this spec only checked already loaded models (~192 and not 628) and not all available because we did not eager load the application and thus ApplicationRecord.descendants did not return all models.

Note that on CI (or with envvar CI=1) application is always loaded eagerly.

Caught this spec while reviewing a similar issue in https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/2369#note_901643664.

Screenshots or screen recordings

n/a

How to set up and validate locally

Print models loaded:

diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb
index 6761e0f5bd3..1043fbcd6ca 100644
--- a/spec/db/schema_spec.rb
+++ b/spec/db/schema_spec.rb
@@ -185,6 +185,7 @@
     let(:models) { ApplicationRecord.descendants.reject(&:abstract_class?) }
 
     it 'all models use smallint for enums', :aggregate_failures do
+      p models: models.size
       models.each do |model|
         ignored_enums = ignored_limit_enums(model.name)
         enums = model.defined_enums.keys - ignored_enums

and run

$ bin/rspec spec/db/schema_spec.rb -e "for enums"
Test environment set up in 6.273770469 seconds

Database schema
  for enums
{:models=>628}
    uses smallint for enums in all models
Finished in 11.96 seconds (files took 9.2 seconds to load)
1 example, 0 failures

Randomized with seed 60581

Example failure

If a model fails the assertions the RSpec failure looks like:

Failures:

  1) Database schema for enums all models use smallint for enums
     Got 27 failures:

     1.1) Failure/Error: expect(model).to use_smallint_for_enums(enums)

            Expected Ci::Runner enums: access_level to use the smallint type.

            The smallint type is 2 bytes which is more than sufficient for an enum.
            Using the smallint type would help us save space in the database.
            To fix this, please add `limit: 2` in the migration file, for example:

            def change
              add_column :ci_job_artifacts, :file_format, :integer, limit: 2
            end
          # ./spec/db/schema_spec.rb:192:in `block (4 levels) in <top (required)>'
          # ./spec/db/schema_spec.rb:188:in `each'
          # ./spec/db/schema_spec.rb:188:in `block (3 levels) in <top (required)>'
          # ./spec/spec_helper.rb:422:in `block (3 levels) in <top (required)>'
          # ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
          # ./spec/spec_helper.rb:413:in `block (2 levels) in <top (required)>'
          # ./spec/spec_helper.rb:409:in `block (3 levels) in <top (required)>'
          # ./lib/gitlab/application_context.rb:48:in `with_raw_context'
          # ./spec/spec_helper.rb:409:in `block (2 levels) in <top (required)>'
          # ./spec/spec_helper.rb:267:in `block (2 levels) in <top (required)>'
          # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
          # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
          # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
          # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Peter Leitzen

Merge request reports