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

feat!: template exception #3561

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

Murtagy
Copy link
Contributor

@Murtagy Murtagy commented Jun 11, 2024

Description

Added a wrapper around exceptions coming from templates.
The exceptions can be rather cryptic, so having a little more context around them is helpful.

E.g. I have an endpoint which uses SQLAlchemy to get some data and then passes it into template context to render.
The SQLAlchemy models have relations, which I have to load using selectinload for optimization and proper use of async.
So an untimely access to model.<relationship_name> caused an unwanted sync call to db from template context and raised an exception with little context around it. It has the endpoint name, but it takes some time to judge wether the error comes from endpoint code or template code, and doesn't help to debug template itself. See full stacktrace https://gist.github.com/Murtagy/9808d91ff497d37f40673516d2a8a373

So the core benefit of this PR is the change of

2024-06-11T09:21:45.120672Z [error    ] Uncaught Exception             connection_type=http path=/c/icard/abaf1066-cecb-424d-b7bf-07313e075608 traceback=  File "<string>", line 2, in _connection_for_bind
  File "/Users/murtagy/Dev/digital_card/.venv/lib/python3.12/site-packages/sqlalchemy/orm/state_changes.py", line 139, in _go
    ret_value = fn(self, *arg, **kw)

into

Traceback (most recent call last):
  File "/Users/murtagy/Dev/project/.venv/lib/python3.12/site-packages/litestar/response/template.py", line 150, in to_asgi_response
    body = template.render(**context).encode(self.encoding)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/murtagy/Dev/project/.venv/lib/python3.12/site-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/Users/murtagy/Dev/project/.venv/lib/python3.12/site-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/Users/murtagy/Dev/project/src/templates/card.html.jinja2", line 1, in top-level template code
    {% extends "base.html.jinja2" %}
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/murtagy/Dev/project/src/templates/base.html.jinja2", line 143, in top-level template code
    {% block content %}
  File "/Users/murtagy/Dev/project/src/templates/card.html.jinja2", line 28, in block 'content'
    {% set interactions = card.interactions %}
    ^^^^^^^^^^^^^^^^^^^^^^^^^ 

Which now points to exact template location. Woohoo!

I did not add any tests for these 🤷

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.24%. Comparing base (883ca0a) to head (21583eb).

Current head 21583eb differs from pull request most recent head 34cccf6

Please upload reports for the commit 34cccf6 to get more accurate results.

Files Patch % Lines
litestar/exceptions/http_exceptions.py 60.00% 1 Missing and 1 partial ⚠️
litestar/response/template.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3561      +/-   ##
==========================================
- Coverage   98.25%   98.24%   -0.02%     
==========================================
  Files         328      328              
  Lines       14870    14868       -2     
  Branches     2366     2364       -2     
==========================================
- Hits        14611    14607       -4     
- Misses        117      119       +2     
  Partials      142      142              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3561

@Murtagy Murtagy marked this pull request as ready for review June 18, 2024 10:32
@Murtagy Murtagy changed the title Feat/template exception feat/template exception Jun 18, 2024
@Murtagy Murtagy changed the title feat/template exception feat: template exception Jun 18, 2024
@Murtagy
Copy link
Contributor Author

Murtagy commented Jun 21, 2024

@JacobCoffee how could I ask for a review? I can not click and change "Reviewers"

Copy link
Member

@provinzkraut provinzkraut 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 the PR @Murtagy!

I'm afraid this will have to wait until 3.0, as it is a breaking change because it changes the exception type, which some users might rely on.

If you want, you can retarget this at the v3.0 branch

@provinzkraut provinzkraut changed the title feat: template exception feat!: template exception Jun 21, 2024
@Murtagy
Copy link
Contributor Author

Murtagy commented Jul 1, 2024

Thanks for the PR @Murtagy!

I'm afraid this will have to wait until 3.0, as it is a breaking change because it changes the exception type, which some users might rely on.

If you want, you can retarget this at the v3.0 branch

What is the exception that users could already handle?
In my understanding, this was a completely unhandled exception. Could this be a minor version change? I would be surprised that adding a handled exception is major version change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants