djezzzl / database_consistency

The tool to avoid various issues due to inconsistencies and inefficiencies between a database schema and application models.
MIT License
1.02k stars 43 forks source link

"undefined method `null' for nil:NilClass" in column_presence_checker.rb:54 #187

Closed asavageiv closed 1 year ago

asavageiv commented 1 year ago
/Users/asavage/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/database_consistency-1.7.5/lib/database_consistency/checkers/validators_fraction_checkers/column_presence_checker.rb:54:in `analyse': undefined method `null' for nil:NilClass (NoMethodError)

        can_be_null = field.null
                           ^^^^^
        from /Users/asavage/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/database_consistency-1.7.5/lib/database_consistency/checkers/validators_fraction_checkers/column_presence_checker.rb:43:in `check'
        from /Users/asavage/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/database_consistency-1.7.5/lib/database_consistency/checkers/base_checker.rb:25:in `report'
        from /Users/asavage/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/database_consistency-1.7.5/lib/database_consistency/checkers/base_checker.rb:34:in `report_if_enabled?'
        from /Users/asavage/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/database_consistency-1.7.5/lib/database_consistency/processors/validators_fractions_processor.rb:23:in `block (3 levels) in check'

database_consistency_2023_04_09_08_42_23.txt

asavageiv commented 1 year ago

My first guess is it's a failure because the user_id column is missing on the payments table.

#<DatabaseConsistency::Checkers::ColumnPresenceChecker:0x000000010d9a0e58
 @association=
  #<ActiveRecord::Reflection::BelongsToReflection:0x000000010d551910
   @active_record=Payment(id: uuid, fulfillable_type: string, fulfillable_id: uuid, checkout_payment_id: string, checkout_payment_data: jsonb, metadata: jsonb, created_at: datetime, updated_at: datetime),
   @class_name="User",
   @constructable=true,
   @foreign_key="user_id",
   @klass=
    User(id: uuid),
   @name=:user,
   @options={},
   @plural_name="users",
   @scope=nil>,
 @attribute=:user,
 @checker_name="ColumnPresenceChecker",
 @column_or_attribute_name="user",
 @model=Payment(id: uuid, fulfillable_type: string, fulfillable_id: uuid, checkout_payment_id: string, checkout_payment_data: jsonb, metadata: jsonb, created_at: datetime, updated_at: datetime),
 @regular_column=nil,
 @table_or_model_name="Payment",
 @validators=[#<ActiveRecord::Validations::PresenceValidator:0x000000010d548860 @attributes=[:user], @options={:message=>:required}>]>
  create_table "payments", id: :uuid, default: -> { "public.gen_random_uuid()" }, force: :cascade do |t|
    t.string "fulfillable_type"
    t.uuid "fulfillable_id"
    t.string "checkout_payment_id"
    t.jsonb "checkout_payment_data", default: {}, null: false
    t.jsonb "metadata", default: {}, null: false
    t.datetime "created_at", precision: 6, null: false
    t.datetime "updated_at", precision: 6, null: false
    t.index ["fulfillable_type", "fulfillable_id"], name: "index_payments_on_fulfillable"
  end
create_table "users", id: :uuid, default: -> { "public.gen_random_uuid()" }, force: :cascade do |t|
...
end
asavageiv commented 1 year ago

Reproducible with this in column_presence_checker_spec:


  context 'when the column for the `belongs_to` foreign key is missing' do
    let(:attribute) { :user }
    let(:klass) { define_class { |klass| klass.belongs_to :user, foreign_key: 'user_id', **required } }

    before do
      define_database do
        create_table :entities
      end
    end

    specify do
      expect(checker.report(catch_errors: false)).to be_nil
    end
  end
djezzzl commented 1 year ago

Hi @asavageiv,

First, thank you for using the gem and reporting the issue! One more thank you for the contribution with the fixing PR!

As I can see, the root cause of the issue is an invalid association, is it?

Do you think we should just ignore it or report that the association is invalid? Or do you think it should be a separate check instead?

asavageiv commented 1 year ago

Whoops, I thought I responded to this a while back, but I guess I forgot to hit "send"!

I guess it depends on how it would impact configuration. Would we want to include/exclude that check separately? I would guess "yes". Checking for a presence validation is not the same thing as checking for an association column existing at all.

djezzzl commented 1 year ago

I agree with you. We could introduce checkers for finding "dead"/invalid code.

Could you please address issues on your PR so I can merge it? And, by the way, do you want to contribute with a separate checker for the issue we talked about?

asavageiv commented 1 year ago

Actually it already seems to be present in ForeignKeyTypeChecker. I got the following error: ForeignKeyTypeChecker fail Payment user association (user) of class (Payment) relies on field (user_id) of table (payments) but it is missing

djezzzl commented 1 year ago

Oh, you're right. I think it would be good to extract it to a separate checker. Would you agree?