-
-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a crash when ActiveJob context include a range with TimeWithZone #2548
Fix a crash when ActiveJob context include a range with TimeWithZone #2548
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2548 +/- ##
==========================================
- Coverage 69.25% 64.01% -5.25%
==========================================
Files 126 123 -3
Lines 4759 4707 -52
==========================================
- Hits 3296 3013 -283
- Misses 1463 1694 +231
|
56ebdce
to
5a73d4a
Compare
1bee091
to
6d8f774
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@st0012 we don't but it's the original behavior - is it OK to change it in a patch/bugfix release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah is it because it was entering the Enumerable
branch? In that case, let's leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@st0012 yes exactly :)
d3a6db5
to
e7db3e1
Compare
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
e7db3e1
to
bc76440
Compare
This PR introduced a new challenge where we have specs that now have min Ruby version requirements due to syntax that's used (end-less and begin-less ranges in this case). To solve this, I introduced a new rake task called @sl0thentr0py @st0012 are you OK with such a solution? One remaining bit is to figure out what to do with Rubocop that has Lint/Syntax and target ruby version set to 2.6, so now it fails. |
let's just not use fancy syntax please then |
oh, it's not that - we just need verify that such ranges work with activejob context param serialization 😅 |
I assume a simple |
hah yeah that was my instinct but then I realized that ruby parses entire file first, it won't skip anything due to conditionals so we were ending up with syntax errors. Alright, gonna merge this in, FWIW I think it's gonna be useful for other stuff too. |
ed9cba9
to
a56bbf8
Compare
Our target ruby version is 2.6 so...
a56bbf8
to
1b9344a
Compare
If a range consists for ActiveSupport::TimeWithZone,
it will be serialized as a string.
Previously it would raise an error
Closes #2545