Add fetch_multi_by_* support for cache_index with a single field

[shwetabhgarg]

@dylanahsmith Are these change ready?

[dylanahsmith]

They need tests still

[shwetabhgarg]

@dylanahsmith are these changes ready?

[shwetabhgarg]

@dylanahsmith could you finish these changes please?

[Matt888]

Updates: added test for happy paths. Some discussion needed about ideal return values (why is it an array, should it be a hash?). Maybe should add some failure scenario tests as well.

[exterm]

I wonder if it would be more consistent to return [[@bob], [@bertha]] here.

[dylanahsmith]

fetch_multi_by_title would always have a predictable return type. Wrapping each value in an array seems unnecessarily inconvenient from the perspective of the caller of that method. Consistency would only make sense if the caller was something generic that had to work with any of these indexes, since that seems like the only type of caller would have to special case single column return values.

Also, this behaviour is consistent with how active record's pluck method works, which similarly asks for a fixed set of columns.

[dylanahsmith]

oh wait, this is supposed to be a non-unique index. I guess the problem is that the caller would have to associate the given values back to the given arguments.

[exterm]

fetch_multi_by_#{key} has the same return type whether the cache is unique: true or not.

All other fetch methods, including fetch_multi_#{alias}_by_#{key}, have different return types depending on the uniqueness of the cache.

[dylanahsmith]

If we use nested arrays for fetch_multi_by_#{key}, then it seems like that would imply that it would always return an array of the same size as the given keys, such that it could be be used with ziped together with the input keys to get a hash of results (e.g. Hash[Item.fetch_multi_by_title(titles).zip(titles)]).

However, I worry that might set the expectation that fetch_multi_by_#{key} and fetch_multi will work similarly. However, fetch_multi returns a compacted array, which means it wouldn't work properly with zip using the input keys array.

[dylanahsmith]

Some discussion needed about ideal return values (why is it an array, should it be a hash?).

I think I chose to try to keep fetch_multi_by consistent with fetch_multi which returns a compacted array.

If a hash is desired, then index_by(&column_name) or group_by(&column_name) could be used to turn it back into a hash if the column is unique or non-unique respectively.

[Matt888]

I think in general, it's a bit weird that the caller can call the same type of method but get different results depending on some internal configuration:

Product.fetch_multi_title_by_title(["hello"])
=> { "hello" => ["hello"] }

Product.fetch_multi_title_by_description(["world"])
=> { "world" => "hello" }

How does the caller know if something is unique or not?

[dylanahsmith]

@Matt888 cache_attribute already works that way for the singular methods. E.g.

Product.fetch_title_by_title("hello")
=> ["hello"]

Product.fetch_title_by_description("world")
=> "hello"

Also, that seems similar to how has_many and has_one differ in behaviour. The caller would be expected to know which type of association it is before using it or they could just discover it by using it in the console and seeing whether it returns an array or not.

[Matt888]

has_many / has_one differ because they use a different method name depending on the configuration. So yes you need to know the association, but once you do, you can refer to it with the appropriate method, which gives you an expectation about what you'll get back

product.variants => [variant, variant] # has_many uses plural name
product.image => image # has_one uses single name

[dylanahsmith]

I'm still unclear on what you are proposing, since it seems like you are criticizing the existing API, which I am trying to make this feature consistent with. Breaking changes should be out of scope for a feature addition PR like this.