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.

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

Jump to bottom

Bug: _TemplateResponse is still relying on request key from context which maybe different from request argument to TemplateResponse. #2531

Closed
apiwat-chantawibul opened this issue Mar 1, 2024 · 4 comments
Closed

Bug: _TemplateResponse is still relying on request key from context which maybe different from request argument to TemplateResponse. #2531

apiwat-chantawibul opened this issue Mar 1, 2024 · 4 comments
Assignees
@alex-oleshkevich
Labels
need confirmation This issue needs confirmation.

Comments

@apiwat-chantawibul
Copy link

apiwat-chantawibul commented Mar 1, 2024 •
edited
Loading

Situation

  • Since Add request argument to TemplateResponse #2191 and the resulting release 0.29.0, request required argument has been added to TemplateResponse constructor to become a replacement for passing request as key-value pair into context argument.
  • In theory, this should open up previously reserved request keyword as a variable name that coder can use in Jinja template.

Reproduction

For example, one should be able to run FastAPI server:

from fastapi import FastAPI, Request
from fastapi.responses import HTMLResponse
from fastapi.templating import Jinja2Templates

app = FastAPI()
templates = Jinja2Templates(directory="templates")

@app.get("/items/{id}", response_class=HTMLResponse)
async def read_item(request: Request, id: str):
    return templates.TemplateResponse(
        request = request,
        name = "item.html",
        context = {
            "id": id,
            "request": "value the user wants to template into request variable",
        }
    )

with templates/item.html

<html>
<body>
id is {{ id }} and request is {{ request }}
</body>
</html>

However, this will raise:

  File "/usr/local/lib/python3.12/site-packages/starlette/templating.py", line 45, in __call__
    extensions = request.get("extensions", {})
                 ^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'get'

when /items/whateverid is accessed.

Cause

When all arguments to TemplateResponse are keyword arguments, request: Request is obtained from one of the keyword arguments:

starlette/starlette/templating.py

Line 218 in 39dccd9

request = kwargs.get("request", context.get("request"))

However, this request: Request is never passed to _TemplateResponse which will still try to find request: Request from context.

starlette/starlette/templating.py

Line 44 in 39dccd9

request = self.context.get("request", {})

  • In this case, it will find request: str instead leading to the previously mentioned error.
  • Or in the case that user didn't supply any request key in context, it will set request to {} which should also be undesirable.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Kludex Kludex added the need confirmation This issue needs confirmation. label Mar 1, 2024
@alex-oleshkevich
Copy link
Member

@apiwat-chantawibul while I agree that having it in the context is not required anymore, removing request from the context is a breaking change as projects expect this variable to be automatically available in templates.

We can have request as constructor argument of _TemplateResponse and it is an option. However I am not sure if it is a good idea. Many developers are used to have request variable in the template context (so does Django, so do we) and I would prefer to keep this convention further.

What do you think @Kludex ?

@Kludex
Copy link
Member

Kludex commented Mar 21, 2024

The changes that we propose ourselves on 1.0 solve this issue, right?

@alex-oleshkevich
Copy link
Member

Yes, it would be possible to override request context variable in v1.0.
See https://github.com/encode/starlette/pull/2384/files#diff-e36747b146f2f9c644d1ae076c13ee41df4a81132f25f14ebe9c6ad4facb1352L225

cc @apiwat-chantawibul

@alex-oleshkevich
Copy link
Member

Feel free to reopen this ticket in case if you still have questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alex-oleshkevich alex-oleshkevich

Labels
need confirmation This issue needs confirmation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@alex-oleshkevich @Kludex @apiwat-chantawibul