-
Notifications
You must be signed in to change notification settings - Fork 2
Fix NoMethodError in JobWebhook when error is nil #222
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 NoMethodError in JobWebhook when error is nil #222
Conversation
sebastianMindee
left a comment
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.
First off, thank you very much for taking the time to do this. The vast majority of people don't care enough to help, so this is like a breath of fresh air.
Secondly, there is a couple of issue with the PR I'd like you to fix if possible:
- The error checks don't need to account for
{}. Both in the test and the constructor. - The error type is correct in the docstring but invalid in steep (my bad), can you update it? should be
attr_reader error: ErrorResponse?instead ofattr_reader error: ErrorResponseinsig/mindee/parsing/v2/job_webhook.rbs. - The linting checks are failing for me. Please run
bundle exec rubocop -Abefore committing.
Once done, I can release later today or tomorrow at the latest if all goes well.
Thank you!
spec/v2/parsing/job_webhook_spec.rb
Outdated
| server_response = { | ||
| 'id' => '12345678-1234-1234-1234-123456789012', | ||
| 'status' => 'Processing', | ||
| 'error' => {} |
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.
Should never happen. And I think it would be important for this to break if an unexpected error was loaded.
lib/mindee/parsing/v2/job_webhook.rb
Outdated
| unless server_response['error'].nil? || server_response['error'].empty? | ||
| @error = ErrorResponse.new(server_response['error']) | ||
| end |
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.
| unless server_response['error'].nil? || server_response['error'].empty? | |
| @error = ErrorResponse.new(server_response['error']) | |
| end | |
| unless server_response['error'].nil? | |
| @error = ErrorResponse.new(server_response['error']) | |
| end |
Empty error shouldn't be possible. Only valid objects or null.
sebastianMindee
left a comment
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.
Amazing thanks 🙏
Summary
Fix
NoMethodErrorinJobWebhook#initializewhen theerrorkey is present but value isnil.Problem
When the API returns a response like:
{"id": "12345678-1234-1234-1234-123456789012", "status": "Processing", "error": null} The JobWebhook initializer would call ErrorResponse.new(nil), causing: NoMethodError: undefined method '[]' for nil Solution Updated JobWebhook#initialize to match the existing pattern in Job#initialize: unless server_response['error'].nil? || server_response['error'].empty? @error = ErrorResponse.new(server_response['error']) end Test plan - Added unit tests for JobWebhook covering all edge cases <img width="1460" height="352" alt="PR_description_img" src="https://github.com/user-attachments/assets/64591692-8d35-4d88-9b43-ef0596d717de" />