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

New template middleware causing an issue? #5034

Open
mlissner opened this issue Feb 3, 2025 · 10 comments
Open

New template middleware causing an issue? #5034

mlissner opened this issue Feb 3, 2025 · 10 comments
Assignees

Comments

@mlissner
Copy link
Member

mlissner commented Feb 3, 2025

In #4956, @v-anne reports that they're having a problem with the new template middleware. They're working on adding pagination, but when they do, they say:

The buttons for the pagination show up fine, but when I try clicking them this shows up:

Environment:


Request Method: GET
Request URL: http://0.0.0.0:8000/prayers/top/?page=2

Django Version: 5.1.5
Python Version: 3.13.1
Installed Applications:
['daphne',
 'pghistory.admin',
 'django.contrib.admin',
 'django.contrib.admindocs',
 'django.contrib.contenttypes',
 'django.contrib.auth',
 'django.contrib.humanize',
 'django.contrib.messages',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.sitemaps',
 'django.contrib.staticfiles',
 'corsheaders',
 'hcaptcha',
 'markdown_deux',
 'mathfilters',
 'rest_framework',
 'rest_framework.authtoken',
 'django_filters',
 'storages',
 'waffle',
 'admin_cursor_paginator',
 'django_elasticsearch_dsl',
 'pghistory',
 'pgtrigger',
 'cl.alerts',
 'cl.audio',
 'cl.api',
 'cl.citations',
 'cl.corpus_importer',
 'cl.custom_filters',
 'cl.disclosures',
 'cl.donate',
 'cl.favorites',
 'cl.people_db',
 'cl.lasc',
 'cl.lib',
 'cl.opinion_page',
 'cl.recap',
 'cl.recap_rss',
 'cl.scrapers',
 'cl.search',
 'cl.simple_pages',
 'cl.stats',
 'cl.users',
 'cl.visualizations',
 'tailwind',
 'django_extensions',
 'django_browser_reload',
 'debug_toolbar']
Installed Middleware:
['django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.security.SecurityMiddleware',
 'django_permissions_policy.PermissionsPolicyMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'corsheaders.middleware.CorsMiddleware',
 'csp.middleware.CSPMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django_ratelimit.middleware.RatelimitMiddleware',
 'waffle.middleware.WaffleMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'cl.lib.middleware.RobotsHeaderMiddleware',
 'cl.lib.middleware.IncrementalNewTemplateMiddleware',
 'pghistory.middleware.HistoryMiddleware',
 'django_browser_reload.middleware.BrowserReloadMiddleware',
 'debug_toolbar.middleware.DebugToolbarMiddleware']



Traceback (most recent call last):
  File "/usr/local/lib/python3.13/site-packages/django/core/handlers/exception.py", line 42, in inner
    response = await get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/django/core/handlers/base.py", line 271, in _get_response_async
    response = await middleware_method(request, response)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/courtlistener/cl/lib/middleware.py", line 86, in process_template_response
    old_template = response.template_name
                   ^^^^^^^^^^^^^^^^^^^^^^

Exception Type: AttributeError at /prayers/top/
Exception Value: 'TemplateResponse' object has no attribute 'template_name'

Originally posted by @v-anne in #4956 (comment)

@mlissner
Copy link
Member Author

mlissner commented Feb 3, 2025

Sorry, Elisa, this one has to go to you. I set it for the next sprint as a high priority, but if something jumps out at you, maybe an opinion now would be helpful to @v-anne.

@v-anne
Copy link
Contributor

v-anne commented Feb 3, 2025

If it helps with debugging, there was a ~50/50 chance that clicking the page buttons worked. Sometimes, they would go to the correct page, but other times, they would throw up the error shown above.

@mlissner
Copy link
Member Author

mlissner commented Feb 4, 2025

Literally 50/50 or roughly 50/50?

@v-anne
Copy link
Contributor

v-anne commented Feb 4, 2025

Roughly. I tested approximately ten times, I think it broke approximately six of them? I will rerun it if necessary.

@elisa-a-v
Copy link
Contributor

I noticed the view is an async one, and I don't have much experience with them, but I wonder if it's possible for the middleware to pick up on the request before the TemplateResponse object has been initialized? It's really weird to me that the TemplateResponse doesn't have the template_name attribute since it's literally the first thing its __init__ sets:

Image

@mlissner
Copy link
Member Author

mlissner commented Feb 4, 2025

We have a bunch of async views. If this was an async thing, I'd expect it to affect lots of them.

@elisa-a-v
Copy link
Contributor

Makes sense, but couldn't it maybe be an error visibility issue? Maybe other views are being affected but we just haven't noticed it yet.

I'd have to look deeper into it, but I think it would help to know if this definitely never ever happens without the changes introduced by that PR. Do we have other async views with pagination?

If we do not have other async views with pagination (couldn't find any with a very quick grep), and this error is only happening with the changes introduced by the PR, I guess it'd be safe to assume there's an issue with the pagination, wouldn't it?

@mlissner
Copy link
Member Author

mlissner commented Feb 4, 2025

We almost always know about bugs that affect multiple pages either via Sentry or user reports, but sometimes they get missed, I suppose.

Could certainly be pagination causing it, yeah, but I don't know why pagination would affect this middleware. Interesting.

There is documentation on async middlewares: https://docs.djangoproject.com/en/5.1/topics/http/middleware/#async-middleware

@mlissner mlissner moved this from Next 1.0 to Buffer Zone in Sprint (Web Team) Feb 7, 2025
@mlissner mlissner moved this from Buffer Zone to To Do in Sprint (Web Team) Feb 10, 2025
@ERosendo
Copy link
Contributor

Hi @elisa-a-v and @mlissner,

I've been reviewing #4956 and have successfully reproduced the problem. The issue appears to be limited to views using the @cache decorator; asynchronous views are not affected.

I think we can use the is_render property (which is True for cached responses) to prevent the middleware from processing the response.

I've implemented the following tweak to the middleware code to address this issue and resolve the exception:

class IncrementalNewTemplateMiddleware:

    def process_template_response(self, request, response):
        use_new_design = flag_is_active(request, "use_new_design")

-       if use_new_design and isinstance(response, TemplateResponse):
+       if (
+           use_new_design
+           and isinstance(response, TemplateResponse)
+           and not response.is_rendered
+       ):
            old_template = response.template_name
            if isinstance(old_template, str):
                new_template = f"v2_{old_template}"
                response.template_name = [new_template, old_template]

        return response

@mlissner
Copy link
Member Author

Is that middleware tweak in the Pray and Pay PR? Are we using that decorator elsewhere?

@mlissner mlissner assigned ERosendo and unassigned elisa-a-v Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Do
Development

No branches or pull requests

4 participants