Skip to content

Refine structured error handler typing#2827

Open
abdulselamadillmohammed wants to merge 2 commits intosnowflakedb:mainfrom
abdulselamadillmohammed:typing-errorhandler-protocol
Open

Refine structured error handler typing#2827
abdulselamadillmohammed wants to merge 2 commits intosnowflakedb:mainfrom
abdulselamadillmohammed:typing-errorhandler-protocol

Conversation

@abdulselamadillmohammed
Copy link
Copy Markdown

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-3246741: Align structured error-handler typing and ready-exception handling #2790

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    This change aligns the connector’s structured error-handling API with the contract already used in errors.py.

    It introduces a shared structured payload type for error details and a shared protocol for error handlers, then applies that contract to the sync connection and cursor errorhandler attributes. It also narrows the structured error-handler flow to Snowflake Error subclasses only.

    In addition, it updates errorhandler_wrapper_from_ready_exception() so generic Python exceptions are re-raised directly instead of being routed into the dict-based structured handler path. This avoids the inconsistent case described in SNOW-3246741: Align structured error-handler typing and ready-exception handling #2790, where non-Snowflake exceptions could enter a handler flow that expects structured error details.

    The PR also adds unit tests covering:
    - default structured error handling
    - forwarding structured payloads to custom handlers
    - normalization of ready Error instances
    - direct re-raise behavior for generic exceptions

  4. (Optional) PR for stored-proc connector:

@abdulselamadillmohammed abdulselamadillmohammed requested a review from a team as a code owner March 25, 2026 11:46
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sfc-gh-turbaszek
Copy link
Copy Markdown
Contributor

Please add changelog entry in DESCRIPTION.md

@abdulselamadillmohammed
Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@abdulselamadillmohammed
Copy link
Copy Markdown
Author

I’ve added a changelog entry in DESCRIPTION.md. Please let me know if anything else is needed or if you have any feedback.

@sfc-gh-turbaszek
Copy link
Copy Markdown
Contributor

Behavioral change in errorhandler_wrapper_from_ready_exception may bypass custom error handlers

The old implementation of errorhandler_wrapper_from_ready_exception routed all exceptions (including non-Error ones) through hand_to_other_handler, which would forward them to any custom errorhandler set on the cursor or connection. The new code immediately re-raises non-Error exceptions, skipping custom handlers entirely:

# New behavior
if not isinstance(error_exc, Error):
    raise error_exc

vs. the old behavior:

# Old behavior
else:
    error_value = error_exc.args

handed_over = Error.hand_to_other_handler(
    connection, cursor, type(error_exc), error_value,
)
if not handed_over:
    raise error_exc

The callers in cursor.py, aio/_cursor.py, and aio/_result_set.py all use isinstance(_next, Exception) as the guard, meaning arbitrary exceptions could flow in. In practice these exceptions come from Error.errorhandler_make_exception(InterfaceError, ...) in result_batch.py, so they should always be Error subclasses. But if any edge case produces a raw Exception, the behavior change would be user-visible:

  1. Custom error handlers on cursor/connection would no longer see the exception
  2. The messages list on cursor/connection wouldn't be appended to
  3. The exception would bubble up raw instead of going through the handler chain

This is probably safe in practice, but it might be worth calling out explicitly as a deliberate behavior change — especially for anyone relying on custom errorhandler callbacks to intercept all exceptions.

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.

SNOW-3246741: Align structured error-handler typing and ready-exception handling

2 participants