thrown ActiveRecord::AssociationTypeMismatch when assigning a wrong value for a namespaced association

[dcrec1]

fixes #20541

[matthewd]

safe_constantize

[dcrec1]

Sorry, do you mean this?:

record.is_a?(reflection.class_name.safe_constantize)

This breaks the tests because #safe_constantize returns nil when couldn't find a match and record.is_a?(nil) will thrown TypeError: class or module required.

[senny]

@dcrec1 you have to split up the one-liner. Using safe_constantize is preferred to rescue false as it doesn't hide programming errors:

tpyo rescue false # => false

Something along the lines of:

if klass = reflection.class_name.constantize
  record.is_a?(klass)
end

[dcrec1]

@rafaelfranca

[senny]

Do we need to create new global models for a single test-case? We generally try to avoid extending those further. Would be nice if we can reproduce the problem with models defined in the test-case itself.

[dcrec1]

The test should be done with a class that only exists in a namespace, but maybe a possibility is to undefine Account and use Admin::Account. Do you think is better?

Also, I added a new validation to the test to ensure that it is really testing what it should.

[senny]

wouldn't defining the class inside the test-case fit that description?

class NamespacedBelongsToAssociationsTest < ActiveRecord::TestCase
  class Region < ActiveRecord::Base
  end
end

[dcrec1]

Yes, defining Admin::Region in the test works, I'm updating the code, thanks :)

[dcrec1]

@senny do you think this is a valid method name? wasn't so sure about how to call this.

[senny]

@dcrec1 I think it's better to inline that into raise_on_type_mismatch!

[dcrec1]

Like this?:

unless record.is_a?(reflection.klass) ||  (record.is_a?(klass) if klass = reflection.class_name.safe_constantize)

Note that this conditional is almost never evaluated, so doing something like this would be less efficient:

valid_reloaded_class = record.is_a?(klass) if klass = reflection.class_name.safe_constantize
unless record.is_a?(reflection.klass) ||  valid_reloaded_class

Which way should I go?

[senny]

what about nesting the conditionals?

unless record.is_a?(reflection.klass)
  fresh_class =  reflection.class_name.safe_constantize
  unless record.is_a?(fresh_class)
    raise ActiveRecord::AssociationTypeMismatch
  end
end

[dcrec1]

This way works, will change the code.

[senny]

please create the table in the test-case as well.

[dcrec1]

@senny do you think another change is needed?

[senny]

You should be able to define everything inline. The classes for example:

  def test_type_mismatch_with_namespaced_class
    Admin.const_set "RegionalUser", (Class.new(Admin::User) {
                                       belongs_to(:region)
                                     })
    Admin.const_set "Region", Class.new(Admin::User)
    e = assert_raise(ActiveRecord::AssociationTypeMismatch) {
      Admin::RegionalUser.new(region: 'wrong value')
    }
    assert_match(/^Region\([^)]+\) expected, got String\([^)]+\)$/, e.message)
  ensure
    Admin.send :remove_const, "Region" if Admin.const_defined?("Region")
    Admin.send :remove_const, "RegionalUser" if Admin.const_defined?("RegionalUser")
  end

Some goes with the schema modifications that you currently do in setup and teardown. No need to do that for every test.

[senny]

@dcrec1 this also needs an entry in the changelog.

[senny]

I merged a slightly modified version of this PR. The changes are contained in the merge commit (8e27fd9).

@dcrec1 thank you 💛

[dcrec1]

thanks @senny