Rescue RangeError and return RecordNotFound instead

[aud]

This rescues the RangeErrors that keep happening all over Shopify (customers, checkout, etc) and instead raises the more appropriate RecordNotFound error instead. Example issue: https://github.com/Shopify/shopify/issues/81204

cc @PhilibertDugas @nikixiaoyou @dylanahsmith

[rafaelfranca]

Is this specific for identity cache? If you do the same query without it, which exception would be raised?

[aud]

The identity cache fetch_by_id method doesn't rescue. The comment for the method says this behaves like ActiveRecord::Base.where(id: id).first, which raises RangeError. So it's not specific to identity cache.

[rafaelfranca]

I see. So I don't think identity cache should rescue in this case too. This is related to rails/rails#26302 rails/rails#25280 rails/rails#24742 rails/rails#25227.

About https://github.com/Shopify/shopify/issues/81204 specifically, I think you can rescue the exception added here rails/rails#24835 in your controller and show a 404 error.

@sgrif are you fine with merging the still open PRs linked above?

[dylanahsmith]

Closing since this PR is proposing to fix this at the wrong level. We don't want to diverge from active record's behaviour.

[aud]

About Shopify/shopify#81204 specifically, I think you can rescue the exception added here rails/rails#24835 in your controller and show a 404 error.

Yeah, I did open a PR for this but it wasn't exclusive to one controller, it was happening all over the place. Would love if we could get those PR's merged!

[aud]

@sgrif @rafaelfranca this is happening more and more frequently in Shopify, in a bunch of different places. Is there anything I can do to help get those above PRs merged?

[rafaelfranca]

Rails is going to change the behavior to rescue RangeError and return nil where appropiate rails/rails#30000. I think we should be doing the same in identity_cache

[dylanahsmith]

Once the behaviour changes in Rails, then won't it no longer be necessary to rescue the range error? E.g. would Topic.where(id: 9223372036854775808).to_a return []?

I would think that by addressing this in identity cache we would be adding more inconsistencies, especially if it we are just adding it to this one specific place in identity cache.

[dylanahsmith]

This method shouldn't raise. The exception throwing variant is below and already does a raise ActiveRecord::RecordNotFound if this method returns nil.

If we want to workaround this issue in identity cache, then it should probably be done in fetch_by_id and dynamic_attribute_cache_miss for unique_index. That way the fix would at least be more consistently applied across identity cache.

[dylanahsmith]

There should be another test to make sure Item.fetch_by_title_and_id("title", "1111111111111111111111111111111") doesn't raise

[dylanahsmith]

Note that any fix to identity cache will be limited to singular fetch APIs. E.g. fetch_multi takes multiple ids, so there isn't an easy way to rescue the RangeError and return something appropriate.

[rafaelfranca]

Once the behaviour changes in Rails, then won't it no longer be necessary to rescue the range error?
I think we still need to rescue here because we are raising error inside identity cache.

identity_cache/lib/identity_cache/query_api.rb

Line 23 in 5806422

id = type_for_attribute(primary_key).cast(id)

BTW, I think there is the correct place to handle this problem.

E.g. would Topic.where(id: 9223372036854775808).to_a return []?
Yes

I would think that by addressing this in identity cache we would be adding more inconsistencies, especially if it we are just adding it to this one specific place in identity cache.

We should be handling this case in all the places where a RangeError can be raised in the library

[dylanahsmith]

If we could check for the type being out of range, then that would allow us to easily filter out ids that are out of range in a method like fetch_multi.

It looks like the cast method on the type doesn't raise the RangeError exception. This means that the exception isn't coming from identity cache, so we would need to change the code to make sure it raises the exception before trying to perform the query in rails to handle it in identity cache.

It looks like we need to use the serialize method on the integer type to get the exception, since the range itself is private. We don't otherwise want to serialize column values in identity cache though so we would be left with code that it sounds like we wouldn't need for a newer version of rails.