Shopify
Rescue RangeError and return RecordNotFound instead
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
Is this specific for identity cache? If you do the same query without it, which exception would be raised?
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.
I see. So I don't think identity cache should rescue in this case too. This is related to https://github.com/rails/rails/pull/26302 https://github.com/rails/rails/pull/25280 https://github.com/rails/rails/pull/24742 https://github.com/rails/rails/pull/25227.
About https://github.com/Shopify/shopify/issues/81204 specifically, I think you can rescue the exception added here https://github.com/rails/rails/pull/24835 in your controller and show a 404 error.
@sgrif are you fine with merging the still open PRs linked above?
Closing since this PR is proposing to fix this at the wrong level. We don't want to diverge from active record's behaviour.
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!
@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?
Rails is going to change the behavior to rescue RangeError and return nil where appropiate https://github.com/rails/rails/pull/30000. I think we should be doing the same in identity_cache
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.
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.
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.
https://github.com/Shopify/identity_cache/blob/58064226361d3e8991722555a1bbb9455dd3abcd/lib/identity_cache/query_api.rb#L23
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
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.