Skip to content
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.

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

fix: Handle exceptions raised when fetching Django request data #758

Merged

Conversation

davidwtbuxton
Copy link
Contributor

The Django helper calls request.build_absolute_uri(), but that may raise a DisallowedHost exception itself. Instead we catch any exception and log None as the URL.

Fixes #757

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: logging Issues related to the googleapis/python-logging API. labels Jun 12, 2023
@davidwtbuxton davidwtbuxton marked this pull request as ready for review June 13, 2023 07:51
@davidwtbuxton davidwtbuxton requested review from a team as code owners June 13, 2023 07:51
@davidwtbuxton
Copy link
Contributor Author

Hi,

I have tested this on the App Engine app that was showing errors, and I no longer see the errors. Which is a good thing.

I'm not sure that my fix of catching any Exception is correct, because it is overly broad. We could catch the specific django.core.exceptions.DisallowedHost exception that is raised by request.build_absolute_uri(), but that would involve importing the exception from Django. Or we could catch Exception and check the doc string. But that then ties us to the docstring. Something like this:

try:
    request_url = request.build_absolute_uri()
except Exception as err:
    if err.__doc__ == "HTTP_HOST header contains invalid value":
        request_url = None
    else:
        raise

Also I figured setting request_url = None and letting the logging request complete with incomplete data was more useful than causing an uncaught exception further up the stack.

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Jul 13, 2023
@parthea parthea added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Aug 9, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 9, 2023
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 9, 2023
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Aug 12, 2023
The Django helper calls `request.build_absolute_uri()`, but that may
raise a DisallowedHost exception itself. Instead we catch any exception
and log `None` as the URL.
@daniel-sanche daniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 21, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 21, 2023
@daniel-sanche daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 21, 2023
Copy link
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 21, 2023
@daniel-sanche daniel-sanche merged commit 5ecf886 into googleapis:main Sep 22, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/python-logging API. size: s Pull request size is small. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging fails with DisallowedHost exception with Django + invalid HTTP_HOST header
4 participants