@fancyremarker => Modify garnered_find to accept multiple arguments

[mzikherman]

Modifies garnered_find to accept a splat to match the API of Mongoid's find .

You can now call Artist.garnered_find('andy-warhol','pablo-picasso') or Artist.garnered_find(['andy-warhol','pablo-picasso'])

I think this is a straightforward fix except for the following cases:

Artist.find('andy-warhol')  # Returns single object
Artist.find(['andy-warhol'])  # Returns an array of a single object

In these cases, the return type should match what was passed in. I couldn't figure out a great way to figure out from *args what the original arguments were, thus: https://github.com/artsy/garner/pull/63/files#diff-617bcfaf854a9b15e531702bca4eabafR64

That has the effect of properly returning an array of one (containing the object), vs the object itself (matching find).

[dblock]

This looks great. Could you please update CHANGELOG, too (add a Next Release). Thanks.

[fancyremarker]

I think you need to add an additional spec for the array version:

Monger.garnered_find(["m1", "m2"]).should == [ @object, @object2]
@object2.update_attributes!({ :name => "M3" })
Monger.garnered_find(["m1", "m2"]).last.name.should == "M3"

I don't think it'll work with the above code. (See my comment there.)

[fancyremarker]

So I double-checked and the above spec does pass. However, all the new specs pass on the old code, assuming that you change the garnered_find(arg1, arg2) calls to have an arity of 1 (garnered_find([arg1, arg2])). So let's try to construct a spec that will fail on the old code.

[fancyremarker]

Thanks a ton for this! It looks great, with the small exception of the case of cache freshness on Mongoid.garnered_find(["m1", "m2"]), per my comment. Can you verify that with a test?

[fancyremarker]

I might delete all but one of these commented lines.

[mzikherman]

So, the specific issue in #62 failed as a spec here: https://github.com/artsy/garner/pull/63/files#diff-53b60d8911baf05f72df2717fd871d24R115 with my changes.

Digging in it seems that "m1" and ["m1"] was is binding to the same key, so explicitly adding a value when a singular object is passed as an array was the only way I saw to fix (w/o going into how they get generated- which I will do later today if I have time). This fix now seems kind of kludgy. What do you think?

[fancyremarker]

Great work @mzikherman, and excellent spec reproducing the error.

I agree with you that any fix necessitates passing in different keys depending on whether garnered_find was called with array arguments (e.g., ["m1"]) versus single object arguments (e.g., "m1"). One way to make the solution less kludgy is to just use the actual arguments passed in as the key. So, instead of keying both key({ :array => true }) and key({ :source => :garnered_find }), we could just do a single key({ :garnered_find_args => args }). Something like this:

def self.garnered_find(*args)
  identity = Garner::Cache::Identity.new
  args.flatten.each do |arg|
    binding = identify(arg)
    identity = identity.bind(binding)
  end
  identity.key({ :garnered_find_args => args }) do
    find(*args)
  end
end

Would that work here?

[mzikherman]

Great call on the keys @fancyremarker . Rather than including the semi-arbitrary :array => true or :source => :garnered_find , we have the args right there and we might as well include them. That also solves the 'special case' of an array of one item vs an item itself cleanly. I just pushed an update to the PR that incorporates this- I'd love your feedback. I can of course rebase/squash if and when this might be mergeable.

[fancyremarker]

I think you may still want to args.flatten here. You'll get a valid result without flattening, but you likely will not get cacheability.

[fancyremarker]

@mzikherman: This looks great. See my comment; I'd like to hear your thoughts as I may be mistaken. Also, no need to rebase/squash. Better to keep the context for our conversation intact, IMO.

[mzikherman]

I'm still a little bit confused as to the need for flatten. Could you clarify a bit more and I'll try to write a spec? Just pushed a spec that find only gets called once when garnered_find called on an array of one object - is that what you meant?

[fancyremarker]

The need for flatten comes when you have a single array argument with N members, instead of N arguments. Consider the following:

garnered_find("foo", "bar") yields

# args = ["foo", "bar"]

# within args.each loop...
binding = identify("foo")
identity = identity.bind(binding)

binding = identify("bar")
identity = identity.bind(binding)

While garnered_find(["foo", "bar"]) yields

# args = [["foo", "bar"]]

# within args.each loop...
binding = identify(["foo", "bar"])
identity = identity.bind(binding)

identify(["foo", "bar"]) won't resolve to a valid proxy_binding, since the array isn't a valid ID or slug, and thus won't have a garner_cache_key, which means it can't be cached.

Does that help explain things at all?

[mzikherman]

Ah yes, it does. I just pushed a trivial spec that sure enough has find being called twice on foo in your example- indicating not caching when called with an array, and correctly with flatten (which makes total sense now- thanks!).

[mzikherman]

Yea, that seems totally obvious now, dont know how I didn't get that.

[fancyremarker]

Thanks @mzikherman, merging. Want to cut and release a new gem version?

[mzikherman]

AWESOME!! I've never done that before but would love to.