Skip to content

Comments

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

Open
aalekseev wants to merge 2 commits intomasterfrom
feat/onfailurehook
Open

SV-246: Add access to request in handle_errors#73
aalekseev wants to merge 2 commits intomasterfrom
feat/onfailurehook

Conversation

@aalekseev
Copy link

@aalekseev aalekseev commented Dec 23, 2025

For audit side-effects on errors added context var and public getter for the current request

@aalekseev aalekseev requested a review from iharthi December 23, 2025 09:13
@aalekseev aalekseev self-assigned this Dec 23, 2025
@aalekseev aalekseev requested review from Jyrno42 and removed request for iharthi December 23, 2025 09:31
esteid/mixins.py Outdated
Comment on lines 65 to 66
def handle_errors(self, e: Exception, stage: SessionStageType = "start", request: HttpRequest = None):
self.on_failure(e, stage, request)
Copy link
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

@aalekseev
Copy link
Author

CI is broken due to oscrypto not working with newer ubuntu versions. Last release that it worked with is Bullseye

esteid/mixins.py Outdated
pass

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

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.

Copy link
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..

Copy link

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

Copy link
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

esteid/mixins.py Outdated
"""
pass

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

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(...)

Copy link
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

@iharthi
Copy link

iharthi commented Dec 23, 2025

Regards the CI issue, I suppose we'll have to move over to cryptography from oscrypto. But this is out of scope of this change.

@aalekseev aalekseev changed the title SV-246: Add on_failure hook for side-effects SV-246: Add access to request in handle_errors Dec 23, 2025
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.

2 participants