Our team ran into a problem in a Rails application today where someone
unknowingly modified Rails internals leading to bad query generation.
The
developer modified the list of attribute names for a model by reading
and
then mutating the contents of
attribute_nameshttp://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/ClassMethods.html#method-i-attribute_nameson
an
ActiveRecord model class. For a variety of reasons it was not
immediately
obvious that they were modifying the results of this call so it took a
while to track down.
Certain query generation code paths, such as distinct counts, can rely
on
private methods like
aggregate_columnhttps://github.com/rails/rails/blob/c9346322b15c15f51234c33a3db1b3895ffe84ab/activerecord/lib/active_record/relation/calculations.rb#L220-L226that
depend on an accurate list of column names to properly prefix column
names with table names. Because attribute_names happens to expose the
internal array used to track column names any modifications to the
return
value of attribute_names can break query generation.
For example (using the ActiveRecord master bug report demo models):
In our case developers did not intend to modify a core array utilized by
Rails for query generation when they modified the results of the
public-facing method. On the other hand I imagine there are cases where
this level of mutability in Ruby and Rails is helpful to people writing
plugins. My core question is how others recommend my team avoid this
particular class of issue going forward.
A few possibilities immediately come to mind:
- Someone much smarter than me comes up with a better solution than
anything that follows - Via thorough testing, static analysis, code review, and careful
attention try to avoid this class of issue going forward- Interested in any tools or other advice to aid this effort
- Find documentation that identifies the publicly visible Rails
methods
intended to be part of the public API- Perhaps some documentation considers methods like
attribute_names that
can trigger this type of issue to be excluded from the Rails API - Is there a clear line between publicly visible Rails methods and
intended parts of the API?
- Claim this behavior is a bug and submit a patch for it because
mutation of the resulting array has problematic implications and it
arguably breaks encapsulation
- I do not plan to pursue this approach unless there is broad
agreement that the current behavior is incorrect as mutability is
commonplace in Ruby - For the sake of this post I am interested in this particular
method
call but not interested in a theoretical discussion about
mutability and
encapsulation in general - I am happy to have a separate theoretical discussion on those
broader topics via a channel that does not spam the entire
mailing list
- Perhaps some documentation considers methods like