Skip to content

Refactor invalid scope

Joe Woodward requested to merge fix/invalid-scope-lfs_object into master

What does this MR do and why?

This change converts LfsObject::for_oid_and_size(oid, size) from a scope to a class method to prevent us from raising NoMethodError further down the stack

Rails scopes should return an ActiveRecord::Relation or nil to allow chaining scopes together. When scopes return a single record we can no longer chain them. Returning a single record from a scope usually won't have any undesirable side-effects, however, if the scope returns nil Rails will treat the scope as if you called Klass.all which causes the app to load all the records in the table and will also return NoMethodError when you try to use a method on the object you expected.

For example:

class LfsObject
  scope :subset, ->(ids) { where(id: ids) }
  scope :invalid_with_correct_result, -> { find_by(id: first.id) }
  scope :invalid_with_unexptected_result, -> { find_by(id: nil) }

Calling LfsObject.subset(first_three_ids) will return the first 3 records in and ActiveRecord::Relation (similar to an Array)

However, if you are expecting to return a single record and then calling lfs_object.do_something, using invalid_with_correct_result and invalid_with_unexptected_result behave very differently

#> lfs_object = LfsObject.invalid_with_correct_result
#> lfs_object === LfsObject.first
=> true
#> lfs_object.do_something
=> did something

Whereas when no record is found and nil is returned inside the scope, the scope returns all records so we can continue chaining scopes

invalid_with_unexptected_result
#> lfs_object = LfsObject.invalid_with_unexptected_result
#> lfs_object === nil
=> false
#> lfs_object === LfsObject.all
=> true
#> lfs_object.do_something
=> NoMethodError: undefined method `do_something' for #<LfsObject::ActiveRecord_Relation:0x000000012a3c5520>

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Joe Woodward

Merge request reports

Loading