samvera
find_with_ids is undocumented, untested, confused
Protected method find_with_ids is only invoked once in this codebase and its method signature and behavior are confused:
https://github.com/samvera/active_fedora/blob/master/lib/active_fedora/relation/finder_methods.rb#L224-L239
def find_with_ids(ids, cast)
expects_array = ids.first.is_a?(Array)
return ids.first if expects_array && ids.first.empty?
ids = ids.flatten.compact.uniq
case ids.size
when 0
raise ArgumentError, "Couldn't find #{@klass.name} without an ID"
when 1
result = find_one(ids.first, cast)
expects_array ? [result] : result
else
find_some(ids, cast)
end
end
No documentation and no tests.
I might expect:
@param [Array<String>] ids
@param [Boolean] cast
@return [Array<Object>] objects that match the id(s) provided
But then, those first two lines of the method anticipate an empty Array as one of the "ids". This doesn't seem to make any sense comparing these invocations::
Image.all.send(:find_with_ids, ['xyz', '123', '345'], false)
=> [#<Image id:'xyz' ...> , #<Image id:'123' ...>, #<Image id:'345' ...>]
Image.all.send(:find_with_ids, ['xyz', [], '345'], false)
=> [#<Image id:'xyz' ...> , #<Image id:'345' ...>]
Image.all.send(:find_with_ids, [[], 'xyz', '345'], false)
=> []
Same elements, different order: radically divergent results.
The only method that invokes find_with_ids is find. The argument it passes unmodified to to find_with_ids is documented as:
@param [String, Hash] args either an id or a hash of conditions
But then it does:
raise ArgumentError, "#{self}.find() expects an id. You provided `#{args.inspect}'" unless args.is_a? Array
Since find assigns args via splat-param, it is literally impossible to ever trigger that exception, so it accomplishes nothing towards validating the params. What a mess!
Like much (all?) of ActiveFedora::FinderMethods, that method comes nearly wholesale from Rails' ActiveRecord::FinderMethods:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/finder_methods.rb#L437
In ActiveRecord's case, the a public find method only accepts a specific id (1), a list of ids (1, 5, 6), or an array of ids ([5, 6, 10]).
Our find's params have the problems I mentioned: documented as @param[String, Hash], variable assigned by splat, meaningless check for Array. @cbeer: In short, we seem to have combined find and find_by, confusingly.
More pointedly, rails doesn't throw exception on exists? like we do (#1276).
In rails 3 find took a hash of conditions, that has since been deprecated and removed. That deprecation hasn't happened in AF yet
On Mon, Aug 21, 2017 at 4:36 PM Joe Atzberger [email protected] wrote:
In ActiveRecord's case, the a public find method only accepts a specific id (1), a list of ids (1, 5, 6), or an array of ids ([5, 6, 10]).
Our find's params have the problems I mentioned: documented as @param[String, Hash], variable assigned by splat, meaningless check for Array. @cbeer https://github.com/cbeer: In short, we seem to have combined find and find_by, confusingly.
More pointedly, rails doesn't throw exception on exists? like we do (#1276 https://github.com/samvera/active_fedora/issues/1276).
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/samvera/active_fedora/issues/1278#issuecomment-323859588, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFnjJ31PcMTOfcJwVhCCR3OEJO6fXMwks5safhQgaJpZM4O8OnB .
