Optionally include rollback exception in closed transaction invalid request error message when possible #11297
Conversation
…rich InvalidRequestError message
| @@ -97,6 +97,15 @@ def _trans_ctx_check(cls, subject: _TConsSubject) -> None: | |||
| trans_context = subject._trans_context_manager | |||
| if trans_context: | |||
| if not trans_context._transaction_is_active(): | |||
| transaction = getattr(subject, "transaction", None) | |||
There was a problem hiding this comment.
@zzzeek is using getattr in this context acceptable, or would you prefer that the _TConsSubject protocol be extended to also require subjects passed to _trans_ctx_check have a transaction attribute?
There was a problem hiding this comment.
the protocol should be expanded and I would stick to underscored names in all cases
| @@ -1031,6 +1031,10 @@ def is_active(self) -> bool: | |||
| def _is_transaction_boundary(self) -> bool: | |||
| return self.nested or not self._parent | |||
|
|
|||
| @property | |||
| def rollback_exception(self): | |||
There was a problem hiding this comment.
let's keep this underscore, dont want to add more public accessors, note TransactionalContext uses underscored method names as its API
| @@ -97,6 +97,15 @@ def _trans_ctx_check(cls, subject: _TConsSubject) -> None: | |||
| trans_context = subject._trans_context_manager | |||
| if trans_context: | |||
| if not trans_context._transaction_is_active(): | |||
| transaction = getattr(subject, "transaction", None) | |||
There was a problem hiding this comment.
the protocol should be expanded and I would stick to underscored names in all cases
| @@ -97,6 +97,15 @@ def _trans_ctx_check(cls, subject: _TConsSubject) -> None: | |||
| trans_context = subject._trans_context_manager | |||
| if trans_context: | |||
| if not trans_context._transaction_is_active(): | |||
| transaction = getattr(subject, "transaction", None) | |||
| if transaction is not None: | |||
| transaction = getattr(subject, "transaction", None) | ||
| if transaction is not None: | ||
| if transaction.rollback_exception is not None: | ||
| raise exc.InvalidRequestError( |
There was a problem hiding this comment.
these two raises that look identical should be just the one raise
|
there's changes requested on this PR if you're still around |
Following discussion #11243
Description
SessionTransaction._rollback_exceptionusing public propertyrollback_exceptionTransactionalContext._trans_ctx_check()when a closed transaction is detected. first checking a transaction'srollback_exceptionis accessible and if it is, raising an InvalidRequestError which includes it in the error messageChecklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!