Attributes declared after instance created have no attribute methods defined

[shioyama]

Summary

This spec is supposed to check that attributes added after subclasses load are inherited, but that's not what it's doing. The call to the method foo is actually falling through to method missing and no actual method foo is defined, which is why the test I've added here fails.

The problem is I believe actually more serious than just the inheritance case tested here: any situation where you have already created an instance of a class, and then you define a new attribute with attribute, will suffer from the same issue.

So e.g.:

class Post < ApplicationRecord
end

Post.new

Post.attribute :foo

In this situation, no foo method will be defined because define_attribute_methods was already called when the first post was created, so @attribute_methods_generated is true and AR will not re-define them.

So I believe what needs to be done is something like this:

def attribute(name, cast_type = Type::Value.new, **options)
  name = name.to_s
  reload_schema_from_cache
+ undefine_attribute_methods

  self.attributes_to_define_after_schema_loads =
    attributes_to_define_after_schema_loads.merge(
      name => [cast_type, options]
    )
+ define_attribute_methods
end

This passes the spec here, but other stuff fails, and this feels very heavy-handed. (see below)

Any idea where we should go with this? It's quite likely that there are many applications which may be falling through to method missing like this for attributes defined in this way, which would incur serious performance degradation.

[shioyama]

I added a fix which undefines attribute methods on the class and all of its descendants when attribute is called:

def attribute(name, cast_type = Type::Value.new, **options)
  name = name.to_s
+ ([self] + descendants).each(&:undefine_attribute_methods)
  reload_schema_from_cache

  self.attributes_to_define_after_schema_loads =
    attributes_to_define_after_schema_loads.merge(
      name => [cast_type, options]
  )
end

This is similar to what is done in reset_column_information (so maybe this could be refactored out into a common method).

[shioyama]

Related: #31475

[shioyama]

cc/ @matthewd You reviewed #31475 and this one is similar. I know you're probably preparing for RubyKaigi (I'll be there & looking forward to your talk!) but maybe could you have a look when you have a chance... ? 🙂

The problem I'm trying to weed out is that there seem to be a few tests that check attributes are defined by calling them, which then fall through to method missing but pass. Which means that although things look fine, they really aren't. That was also the case for #31475, which was fixed in a similar way to here (undefining attribute methods on self and descendants).

I think the fact that methods need to be undefined like this is really ugly actually, but before trying to improve anything I want to ensure tests are actually testing what they say they are testing.

[shioyama]

The fix here still fails this test:

klass = Class.new(ActiveRecord::Base) do
  self.table_name = "topics"
end

instance = klass.new
klass.attribute(:foo, Type::Value.new)

assert(instance.methods.include?(:foo))

The assertion fails because attribute methods have been undefined and no new instance has been created, so they haven't been redefined yet.

My initial suggestion to undefine, then redefine attribute methods would load the schema, which is a no-go.

I'm aware that attribute is a pretty central method and there will be a lot of eyes on this piece of code, so before I go and try and hack out something better I'd like to discuss the broken test first.

[shioyama]

This is the kind of thing that would pass the test above:

def attribute(name, cast_type = Type::Value.new, **options)
  name = name.to_s
  if methods_redefine_required = @attribute_methods_generated
    undefine_attribute_methods
  end
  reload_schema_from_cache

  self.attributes_to_define_after_schema_loads =
    attributes_to_define_after_schema_loads.merge(
      name => [cast_type, options]
    )
  define_attribute_methods if methods_redefine_required
end

This is really ugly, but basically it checks if we have already defined attribute methods, and if so, undefine and redefines them after adding the new attribute. This works but is really ugly. Maybe the ugliness can be encapsulated in a redefine_attribute_methods which takes a block and undefines/redefines the methods arond the block.

[rafaelfranca]

Why would you define an attribute outside of the model?

[shioyama]

Why would you define an attribute outside of the model?

First off, the test is testing this, so I assume it's considered a valid use case.

But (although I wouldn't do this myself) I do think there are many cases where this could happen.

class ApiObject < ApplicationRecord
end

class ApiResponse
  def method_missing(method_name, *args, &block)
    if (match = /^(?:)(.*)(?:=)$/.match(method_name)) &&
      ApiObject.attribute_names.exclude?(match[1])
      ApiObject.attribute match[1], default: args[0]
    else
      super
    end
  end
end

response = ApiResponse.new

# this is what causes problems
ApiObject.new

response.foo = "fooval"
response.bar = "barval"

object = ApiObject.new
object.attributes
#=> {"id"=>nil, "title"=>nil, "created_at"=>nil, "updated_at"=>nil, "foo"=>"fooval", "bar"=>"barval"}

object.foo
#=> "fooval"

object.method(:foo)
#=> NameErorr (undefined method `foo' ...

I'm not trying to convince you this is a good pattern, just that it is totally a realistic one.

If this is not supported, then attribute should raise an exception if @attribute_methods_generated is true on the model class. Or it should handle it correctly and define the method. But right now, it does neither, so you've got a situation where nobody will notice, but your application is taking way longer to get/set these attributes.

[matthewd]

👋🏻

As you suspected, I can't give this proper attention this week, but my short observation is that this sounds like another instance of a vaguely-known issue. Mostly that involves e.g. class attributes on a superclass after subclassing, but this instance doesn't seem.. out of character, either.

I'm always keen to remove such limitations in principle, though as you've observed here the solutions tend to be some combination of ugly and/or slow... which is a large part of why we tend to just accept them as limitations that most people won't run into in practice.

To the test: I'd say it's most important that it passes as currently written -- that even if it's going a dumb route, it does do the thing. (I'd also be curious to confirm whether the test was mistaken about what it was testing back when it was added in 9deb6ab, or this happened later.)

Overall: I like the idea of making this better, but that enthusiasm is inversely proportional to the amount of code we end up needing to introduce to do so.

Perhaps instead of actually undefining the attribute methods, we could merely mark them as being out of date, and needing to be [undefined and] redefined next time we check... so we don't un-methodize attributes that were already there and working, and then accept that an instance created before the attribute was added will still be operating in method-missing-fallback mode. (Or maybe more extreme, if we identify that attribute methods have already been defined, maybe we could just arrange to immediately define the new attribute in particular, without un/re-defining all the others?)

[shioyama]

Thanks @matthewd for the very thoughtful response.

Perhaps instead of actually undefining the attribute methods, we could merely mark them as being out of date, and needing to be [undefined and] redefined next time we check...

Yes, something along those lines might work I agree.

I think I should say: my hidden agenda here is to narrow the requirement for the method missing handlers in AttributeMethods so that maybe they can be simplified or partially removed.

The only case where they seriously matter (AFAICT) in AR is with this pattern:

post = Post.select("title as foo").first
post.foo # <= hits method_missing since 'foo' is not in the schema

That's an important pattern to be sure. But do you realize that to hit that handler in method missing, we're going through 17 other matchers first? Matchers that check patterns like post.foo_was, post.foo= which make no sense for attributes returned from raw sql like this. The only one that makes any sense is maybe post.foo? but the cost in terms of code complexity and performance (on first hit anyway) is significant enough that I think the whole abstraction should be rethought.

Once upon a time, these matchers were used in a much broader way, but now they're mostly a relic of the past. So my goal here is to weed out the places where code is hitting method_missing for reasons other than the (important) pattern above (and this test is one case of that).

Anyway, maybe we can chat about it at RubyKaigi 😉

[shioyama]

I'm going to close this for now because although I think the test is broken, I don't really like my solution here and would prefer it not be merged. I'll come back to this.