Skip to content

SV-246: Add access to request in handle_errors #73

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.

Sign up for GitHub

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

Open
aalekseev wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SV-246: Add access to request in handle_errors #73

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec2514e
SV-246: Add on_failure hook for side-effects
aalekseev Dec 23, 2025
0167ac6
fix: code-review fixes, rolling back a bit of implementation
aalekseev Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions esteid/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import json
import logging
from http import HTTPStatus
from typing import Callable, TYPE_CHECKING
from typing import Callable, Literal, TYPE_CHECKING

from django.core.exceptions import ValidationError as DjangoValidationError
from django.http import Http404, HttpRequest, JsonResponse, QueryDict
Expand Down Expand Up @@ -30,6 +30,9 @@ class RequestType(HttpRequest):
logger = logging.getLogger(__name__)


SessionStageType = Literal["start", "finish"]


class SessionViewMixin:
"""
Provides POST and PATCH method handlers for auth/signing session management.
Expand All @@ -52,7 +55,16 @@ def report_error(self, e: EsteidError):
def handle_user_cancel(self):
pass

def handle_errors(self, e: Exception, stage="start"):
def on_failure(self, e: Exception, stage: SessionStageType, request: HttpRequest):

@iharthi iharthi Dec 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here we change the signature of handle_errors, which is not backwards compatible. I am not 100% sure how bad it is (I don't think it is used outside a certain project you need the change for), but it is not great (especially not great since we don't have any comprehensive change-log that I would be aware of to empathize the fact there is a breaking change).

To work around this, we could create a context variable (per view instance) and then set that in dispatch to hold request. Then we could read it from context variable, if necessary, in on_failure / handle_errors.

Why context variable? There is a concern in another MR that needs checking, if setting an instance property on view would break in django async. If the concern is correct (we didn't check it, but it probably is) then context variable is better; if the concern isn't correct then it can just be an instance property like self._request.

@aalekseev aalekseev Dec 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

handle_errors is backward compatibly since the request is optional param there and it is passed in post and patch calls 🤔 also it is the last arg so optional..

@iharthi iharthi Dec 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Argument is optional, but if the method is overridden by a subclass in the project using the library the signature should match

@aalekseev aalekseev Dec 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

you are right here, tried contextvar approach now

"""
Hook called when exception occurs during authentication/signing.
It runs before "handle_errors" and allows to perform logging, auditing, or other side effects.
"""
pass

def handle_errors(self, e: Exception, stage: SessionStageType = "start", request: HttpRequest = None):

@iharthi iharthi Dec 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why do we need to have a separate on_failure if we can just override handle_errors and do

def handle_errors(...):
    do_whatever_on_failure_needs_to_do()
    return super().handle_errors(...)

@aalekseev aalekseev Dec 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes I tihnk it would make sense to remove separate on_failure and do it with super

self.on_failure(e, stage, request)

@aalekseev aalekseev Dec 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Perhaps we can get away with just passing request to handle_errors and the developer would be responsible to add side effects and calling super on that overwritten method


if isinstance(e, EsteidError):
if isinstance(e, CanceledByUser):
self.handle_user_cancel()
Expand Down Expand Up @@ -84,7 +96,7 @@ def post(self, request, *args, **kwargs):
try:
return self.start_session(request, *args, **kwargs)
except Exception as e:
return self.handle_errors(e, stage="start")
return self.handle_errors(e, stage="start", request=request)

def patch(self, request, *args, **kwargs):
"""
Expand All @@ -93,7 +105,7 @@ def patch(self, request, *args, **kwargs):
try:
return self.finish_session(request, *args, **kwargs)
except Exception as e:
return self.handle_errors(e, stage="finish")
return self.handle_errors(e, stage="finish", request=request)


class DjangoRestCompatibilityMixin(SessionViewMixin):
Expand Down
Loading