Skip to content

Conversation

@wooooodward
Copy link

@wooooodward wooooodward commented May 6, 2025

Describe the solution

Switches from direct key access to .get() for optional fields (error, request, cause) in the Onfleet API error payload. This prevents a KeyError when those fields are missing and ensures the intended error (e.g., HttpError, ValidationError, etc.) is raised instead.


Fixed

Fixes #30 – prevents crashes caused by missing fields in Onfleet error responses. Now the correct exception is raised with partial error data instead of blowing up with a KeyError.


Important

Switches to .get() for optional fields in onfleet/request.py to prevent KeyError and ensure correct exceptions are raised.

  • Error Handling:
    • In onfleet/request.py, switch from direct key access to .get() for error, request, and cause fields in error payloads.
    • Prevents KeyError when fields are missing, ensuring correct exceptions are raised.
  • Fixed:

This description was created by Ellipsis for ffbb6e1. You can customize this summary. It will automatically update as commits are pushed.

Avoid KeyError when optional fields are missing in Onfleet error response
@ellipsis-dev
Copy link

ellipsis-dev bot commented May 6, 2025

PR Summary

This pull request improves error handling in the request.py file by replacing direct dictionary access with the safer .get() method for optional fields in the Onfleet API error payload.

Changes

  • Modified error handling to use .get() method instead of direct key access for optional fields (error, request, cause)
  • Prevents KeyError exceptions when these fields are missing in the API response

Impact

  • Fixes issue [BUG] KeyError when batchCreate results in errors #30 where the code would crash with a KeyError when certain fields were missing
  • Ensures the intended error (e.g., HttpError, ValidationError) is raised instead of an unrelated KeyError
  • Improves robustness of the error handling mechanism by gracefully handling incomplete error payloads

This is a small but important change that enhances the reliability of the library when dealing with unexpected API responses.

@ellipsis-dev
Copy link

ellipsis-dev bot commented May 6, 2025

⚠️ This PR modifies application logic but lacks adequate unit test coverage. Please ensure all new logic is tested.

While this is a small change to error handling, it's addressing an important bug where the wrong type of error was being raised. Adding tests would help verify that the fix works as expected and prevent regression in the future.

Consider adding tests that:

  1. Verify behavior when error fields are missing from the response
  2. Ensure the correct exception type is raised with partial error data
  3. Confirm the fix resolves the issue described in [BUG] KeyError when batchCreate results in errors #30

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to ffbb6e1 in 2 minutes and 12 seconds. Click for details.
  • Reviewed 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. onfleet/request.py:68
  • Draft comment:
    Using .get() avoids KeyError for optional fields, but error_code is later used in numeric comparisons (e.g., line 75). If the 'error' key is missing, error_code becomes None, which will raise a TypeError. Consider providing a default numeric value, e.g. error['message'].get('error', 0), or adding a check before comparisons.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_R2AgANdeGVOuPNhw

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Providing a default numeric value (e.g. .get('error', 0)) to ensure safe comparisons.

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] KeyError when batchCreate results in errors

1 participant