-
Notifications
You must be signed in to change notification settings - Fork 573
Refine structured error handler typing - #2827
Conversation
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
sfc-gh-turbaszek
commented
Mar 25, 2026
Please add changelog entry in DESCRIPTION.md
abdulselamadillmohammed
commented
Mar 25, 2026
I have read the CLA Document and I hereby sign the CLA
abdulselamadillmohammed
commented
Mar 25, 2026
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
commented
Mar 27, 2026
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:
- Custom error handlers on cursor/connection would no longer see the exception
- The
messageslist on cursor/connection wouldn't be appended to - 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.
abdulselamadillmohammed
commented
Mar 28, 2026
Thanks for calling this out. This behavior change was intentional.
The goal of this PR was to align the structured error-handler path with the narrower contract used by the rest of the structured error machinery in errors.py: a Snowflake Error subclass together with a structured error payload.
Because of that, I changed errorhandler_wrapper_from_ready_exception() so that only Error instances are normalized and routed through the structured handler flow. Generic Python exceptions are now re-raised directly instead of being passed into the dict-based structured path.
I agree this is a behavioral change if a raw Exception reaches that wrapper. My intent was to make that boundary explicit rather than continue routing arbitrary exceptions through an API built around structured Snowflake error details. In the expected flow, callers reaching this wrapper should already be producing Snowflake Error instances.
Also, while the typing changes in this PR were made on the sync connection/cursor errorhandler attributes, the errorhandler_wrapper_from_ready_exception() change is in shared errors.py, so the runtime behavior change applies to both sync and async internal call sites that invoke that wrapper. The async connector does not expose the same public errorhandler API, so I did not make corresponding async attribute/property changes there.
I can update the PR description and changelog wording to make this behavior change more explicit, and I can also add async-path test coverage around the shared wrapper call sites if you’d like.
sfc-gh-turbaszek
commented
Apr 9, 2026
@abdulselamadillmohammed how crucial is this change? Any breaking change will require a major version bump which we want to avoid for now
abdulselamadillmohammed
commented
Apr 10, 2026
Hi @sfc-gh-turbaszek ,
I’ve updated the logic to address the custom-handler and messages concerns you raised.
Non-Snowflake exceptions no longer bypass the handler chain. I've updated errorhandler_wrapper_from_ready_exception to call hand_to_other_handler for all exceptions, so connection.messages and cursor.messages are populated correctly and custom handlers can intercept the error. If nothing handles the exception, the original exception instance is re-raised rather than constructing a new one.
To keep the StructuredErrorHandler contract consistent, I am normalizing generic Python exceptions into an InterfaceError with a structured payload before passing them to handlers. This restores handler invocation and the messages updates, but it does still change what handlers see for generic exceptions: instead of the original Python exception class and args, handlers receive InterfaceError with a structured dict. I believe this is the cleanest way to preserve the structured handler contract while addressing the issues you pointed out, but please let me know if you’d prefer a closer restore of the old generic-exception behavior.
...-protocol # Conflicts: # DESCRIPTION.md
Please answer these questions before submitting your pull requests. Thanks!
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
Fill out the following pre-review checklist:
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
errorhandlerattributes. It also narrows the structured error-handler flow to SnowflakeErrorsubclasses 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
Errorinstances- direct re-raise behavior for generic exceptions
(Optional) PR for stored-proc connector: