MONGOID-5789 database_field_name given nil or empty string should raise UnknownAttribute exception

[danhealy]

From github discussion: #5540

We are seeing this behavior in 8.1.4

record = Person.first
record[nil]
undefined method `include?' for nil:NilClass      string.include?(".")
            ^^^^^^^^^
/usr/local/bundle/gems/mongoid-8.1.4/lib/mongoid/attributes.rb:272:in `hash_dot_syntax?': undefined method `include?' for nil:NilClass
      
string.include?(".")
            
^^^^^^^^^ (NoMethodError)
    
from /usr/local/bundle/gems/mongoid-8.1.4/lib/mongoid/attributes.rb:297:in `read_raw_attribute'
    
from /usr/local/bundle/gems/mongoid-8.1.4/lib/mongoid/attributes.rb:88:in `read_attribute'

This PR changes the behavior to raise a more helpful Mongoid::Errors::UnknownAttribute exception instead

[jamis]

@danhealy -- as you noted on the Jira ticket, returning an empty string (instead of raising an exception) is probably more appropriate here. A few reasons:

1. The Mongoid::Errors::UnknownAttribute error is specifically intended for cases where an assignment is detected to an undeclared attribute. You can see this explicitly in the lib/config/locales/en.yml file, under the unknown_attribute key, where it declares "Attempted to set a value for '%{name}' which is not allowed on the model %{klass}." By raising this exception on a simple access, we risk confusing users more because the message is accusing them of attempting to set a value somewhere.
2. In Mongoid 7.5 and earlier, the default behavior was to return an empty string here (because the name argument was previously invoked with to_s before being passed in, and was thus never nil along the path that's relevant to us here). In Mongoid 8.x, the default was changed, but the legacy behavior could still be accessed by setting Mongoid.broken_alias_handling = true. That compatibility flag was removed in Mongoid 9. Given that the legacy behavior was (effectively) to return an empty string, it's probably appropriate to do the same, here.

There may still be collateral damage from such a change, but a quick check shows that at least some of the tests that were failing as a result raising the exception, pass successfully when an empty string is returned. It's worth experimenting with returning the empty string, I think.

Thank you for your work on this issue!

[johnnyshields]

@danhealy I agree with @jamis and would vote against this change. I have code like the following which would break:

MyModel.database_field_name(:foo) || MyModel.database_field_name(:bar)

Nothing stops you from doing a monkey patch like:

module Mongoid::Document
  class_methods do
    def database_field_name!(name)
      database_field_name(name) || raise(ArgumentError.new("#{self.class} field name not found: #{name}"))
    end
  end
end

@jamis could consider this as a bang (!) variant of the method.

[danhealy]

@jamis and @johnnyshields thank you, I appreciate the feedback!

I see 3 paths forward with this issue:

1. Do nothing and allow record[nil] to continue to raise the unhelpful exception mentioned in the description
2. Modify record[nil] to not raise an exception and match the behavior of Mongoid 7 by returning nil
3. Have record[nil] raise a new exception class

After @jamis 's comment I modified the PR so that it attempts to implement option 2. I will need some assistance going through the test failures.

[jamis]

@danhealy -- I agree that option 2 will be ideal. I'll take a look this afternoon/tomorrow and see what I can find regarding the failures, and what options we might have to fix those. Thanks for your work on this!