When Constants Aren't

I was recently working on some unfamiliar code and ran across a confusing bug. I’ve changed the names and nature of the code below, but the essence of the bug remains.

One of the model classes defines a constant that is an array containing some symbols. Other methods in the class take an optional parameter containing a subset of these symbols; if the parameter isn’t specified, then it defaults to the constant.

class Bedrock < ActiveRecord::Base
  has_many :flintstones
  has_many :rubbles
  has_many :slates
 
  FAMILIES = [:flintstones, :rubbles, :slates]
 
  #...
 
  def all_characters(options={})
    families = options[:only] || FAMILIES
    families.flat_map { |family| all_family_members(family) }
  end
 
  private
 
  def all_family_members(family)
    case family
      when :flintstones then flintstones
      when :rubbles then rubbles
      when :slates then slates
    end
  end
end

Using this code, you can get a list of characters for a subset of the families:

main_characters = client.all_characters(only: [:flintstones, :rubbles])

The problem came when some other code used the FAMILIES constant to compute the subset to pass to this method. There are a number of layers of method calls involved, so the bug was obscure, but essentially there was some code doing this:

def some_request
  only = Bedrock::FAMILIES
  #...
  if only.include?(:slates)
    only << :rockheads
    only.delete(:slates)
  end
  @bedrock.all_characters(only: only)
end

It looks like there is a rename from :slates to :rockheads happening for some reason. All of the other evidence in the code suggests that there was a rename the other way at some point. We’re not yet sure what to make of this.

Even if we assume that this rename makes sense, the problem is that the code is mutating an array that we thought was a constant. There is nothing in Ruby that stops anyone from modifying the contents of the array as long as they don’t try to reassign the constant itself. The same issue would arise if the constant was an instance of any other class that had methods allowing the internal state of the object to be changed.

There are several code smells here:

• The replacing of :slates with :rockheads is questionable. This codebase is new to us, so we haven’t yet figured out if it makes sense to do this. There’s a comment nearby that suggests that it is intentional; we’re taking that at face value until we learn more.

• Mutating the array in-place is likely not the best idea; sometimes we want to avoid creating extra garbage, though. Either way, it’s a good idea to know where the array came from before mutating like this.

• Direct access to the FAMILIES constant breaks encapsulation.

We could freeze the array when creating it. That would stop any in-place mutation, but we’d have be sure that our tests exercise every code path that might try such a mutation. Since the code is still pretty new to us, we’re not sure how thorough the tests are.

We could have changed the code that was mutating the array; using freeze would have forced us to do this. There are at least two places where some kind of mutation is going on; there might be more, and we can’t be sure that no one else will ever implement something similar, so we rejected this approach.

We ultimately decided that the broken encapsulation was the most important problem to address. This fixes the problem at the source so that no other code accesses the constant directly. Any indirect accesses get a copy of the array, not the original.

We also decided to assume (for now) that the rename from :slates to :rockheads is legitimate, so we added a clause to our case statement until we better understand the reason for it.

class Bedrock < ActiveRecord::Base
  # ... as before ...
 
  def self.all_families
    FAMILIES.dup
  end
 
  # ... as before ...
 
  def all_family_members(family)
    case family
      when :flintstones then flintstones
      when :rubbles then rubbles
      when :slates then slates
      when :rockheads then slates  # Protect against questionable rename
    end
  end
end
 
#...
 
def some_request
  only = Bedrock::all_families
 
  # rest as before...
end

This fixes the immediate bug, creating a safer world for us to continue to learn the codebase and make improvements. We could go one step further and make the constant private; we haven’t (yet) done that in this case.

In general, I like to find a way to fix bugs like this by making it so that it is impossible for the bug to occur. I don’t want to have to trust other programmers, including my future self, to remember the intimate details of how my code needs to be used.

Instead, my code should only allow proper usage. If I can’t quite reach that ideal, then it should communicate proper usage as best it can and, if necessary, protect itself from improper usage.