Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make error deserialization more resilient #127

Merged
merged 2 commits into from
Jan 10, 2022
Merged

Make error deserialization more resilient #127

merged 2 commits into from
Jan 10, 2022

Conversation

nagl-stripe
Copy link
Contributor

Description

#103 lets temporal-ruby respond gracefully when it can't construct the correct exception from serialized data. This PR makes that logic kick in for any type of deserialization failure, not just ArgumentError

Testing

I added a test to this unit: bundle exec rspec ./spec/unit/lib/temporal/workflow/errors_spec.rb

Integration, to check for regressions:

cd examples
./bin/worker &
USE_ENCRYPTION=1 ./bin/worker &
bundle exec rspec spec/integration

antstorm
antstorm reviewed Dec 23, 2021
View reviewed changes
Copy link
Contributor

@antstorm antstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! A tiny comment, but otherwise looks good.

CC @drewhoskins-stripe in case you've got thoughts on this

spec/unit/lib/temporal/workflow/errors_spec.rb Outdated
def initialize(message)
# This class simulates an error class that has bugs in its initialize method, or where
# the arg isn't a string. It raises TypeError (String can't be coerced into Integer)
1 + message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be clearer to just explicitly raise the error here

@nagl-stripe nagl-stripe requested a review from antstorm January 5, 2022 18:18
antstorm
antstorm approved these changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@antstorm antstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@antstorm antstorm merged commit f764e21 into coinbase:master Jan 10, 2022
@antstorm antstorm added the sync pending Needs to be ported to cadence-ruby label Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antstorm antstorm antstorm approved these changes

Assignees
No one assigned
Labels
sync pending Needs to be ported to cadence-ruby
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nagl-stripe @antstorm