Fix a crash when ActiveJob context include a range with TimeWithZone #2548
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2548 +/- ##
==========================================
+ Coverage 67.90% 68.10% +0.20%
==========================================
Files 118 118
Lines 4483 4480 -3
==========================================
+ Hits 3044 3051 +7
+ Misses 1439 1429 -10
|
If a range consists for ActiveSupport::TimeWithZone, it will be serialized as a string. Previously it would raise an error ``` TypeError: can't iterate from ActiveSupport::TimeWithZone ``` Closes #2545
solnic
force-pushed
the
solnic/2545-activejob-typeerror-cant-iterate-from-activesupporttimewithzone
branch
from
56ebdce to
5a73d4a
Compare
solnic
requested review from
sl0thentr0py and
st0012
and removed request for
sl0thentr0py
| { | ||
| "integer" => 1, | ||
| "post" => "gid://rails-test-app/Post/#{post.id}", | ||
| "range" => [1, 2, 3] |
There was a problem hiding this comment.
it's probably not a good idea to do this, but in a way this is public API so I didn't change it.
There was a problem hiding this comment.
Yeah, this can cause some crazy performance and memory issues I imagine. 😬
| @@ -79,6 +79,12 @@ def sentry_context(job) | |||
|
|
|||
| def sentry_serialize_arguments(argument) | |||
| case argument | |||
| when Range | |||
| if argument.first.is_a?(ActiveSupport::TimeWithZone) | |||
There was a problem hiding this comment.
This would still not support ranges of floats for example. (E.g. (1.1...1.5)) Is that intentional, just to keep this fix small?
There was a problem hiding this comment.
@solnic Also, beware that Range#first raises an exception if it's a beginless range (..1).
(..1).first
'Range#first': cannot get the first element of beginless range (RangeError)
Unlike begin, which would return nil.
There was a problem hiding this comment.
@maxim I just left original behavior untouched, but eventually I'd vote for changing it to always dump ranges to a string "{left}..{right}"
solnic
force-pushed
the
solnic/2545-activejob-typeerror-cant-iterate-from-activesupporttimewithzone
branch
from
1bee091 to
6d8f774
Compare
solnic
changed the title
| if (argument.begin || argument.end).is_a?(ActiveSupport::TimeWithZone) | ||
| argument.to_s | ||
| else | ||
| argument.map { |v| sentry_serialize_arguments(v) } |
There was a problem hiding this comment.
Do we need to serialize individual components in the range? If the range is (1..10000) then it'd probably be pretty slow AND it'd allocate a new array with 10k elements in it.
Maybe in this case we just leave the argument until we find another element type that requires special treatments?
If a range consists for ActiveSupport::TimeWithZone,
it will be serialized as a string.
Previously it would raise an error
Closes #2545