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

TemplateHTMLRenderer - TypeError - context must be a dict rather than ReturnList. #5236

Open
5 of 6 tasks
Eskimon opened this issue Jun 26, 2017 · 22 comments
Open
5 of 6 tasks
Labels

Comments

@Eskimon
Copy link

Eskimon commented Jun 26, 2017

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

In Django 1.8+, the template's render method takes a dictionary for the context parameter. Support for passing a Context instance is deprecated, and gives an error in Django 1.10+ (source1 / source2).

When adding a renderer to a generics.ListCreateAPIView, a TypeError: context must be a dict rather than ReturnList. pop.

To reproduce, create a simple Model+Serializer to fill data in a generics.ListCreateAPIView. Add a renderer_classes = [TemplateHTMLRenderer] to the ListView and try to access the page from a browser to render the template. Should crash (error 500 as an Exception is raised).

Expected behavior

The template should be rendered.

Actual behavior

A TypeError is thrown because context must be a dict rather than ReturnList

@tomchristie
Copy link
Member

Thanks for raising this.

@kezabelle
Copy link
Contributor

Example stacktrace:

Traceback (most recent call last):
  File "/PATH/python3.5/site-packages/django/core/handlers/exception.py", line 41, in inner
    response = get_response(request)
  File "/PATH/python3.5/site-packages/django/core/handlers/base.py", line 217, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/PATH/python3.5/site-packages/django/core/handlers/base.py", line 215, in _get_response
    response = response.render()
  File "/PATH/python3.5/site-packages/django/template/response.py", line 107, in render
    self.content = self.rendered_content
  File "/PATH/python3.5/site-packages/rest_framework/response.py", line 72, in rendered_content
    ret = renderer.render(self.data, accepted_media_type, context)
  File "/PATH/python3.5/site-packages/rest_framework/renderers.py", line 176, in render
    return template_render(template, context, request=request)
  File "/PATH/python3.5/site-packages/rest_framework/compat.py", line 340, in template_render
    return template.render(context, request=request)
  File "/PATH/python3.5/site-packages/django/template/backends/django.py", line 64, in render
    context = make_context(context, request, autoescape=self.backend.engine.autoescape)
  File "/PATH/python3.5/site-packages/django/template/context.py", line 287, in make_context
    raise TypeError('context must be a dict rather than %s.' % context.__class__.__name__)
TypeError: context must be a dict rather than ReturnList.

I think it may only be the case if PAGE_SIZE is None, too, because changing that to != None wraps the results up nested within an OrderedDict, which passes the check by the look of it.

@ambsw-technology
Copy link

I just ran into this. Is there a workaround? or a local fix I can apply for development?

@arijeetmkh
Copy link

@kezabelle @tomchristie As far as I understand this issue, irrespective of the change in Django 1.10+ mentioned originally by @Eskimon, the TemplateHTMLRenderer has never been able to work with a ReturnList, or anything other than a dict like object for that matter. When serializers are run in read mode using many=True, the data structure coming out of them are lists. The problem occurs when your pagination_class is set to None. I have had to alter the behaviour of the ListSerializer's to_representation and ensure the returned object is a dict, in order to get my template renderer to work. If you are using pagination, then the list of results are anyways wrapped up in an dict and sent to the renderer, so it should work in that case.

@kezabelle You are right in saying that simply ensuring (somehow) that dicts are returned will make the renderer work.

As far as the Django 1.10+ issue is concerned, the change suggests using native dicts instead of Context objects. From what I can see, this should not affect the working of TemplateHTMLRenderer.

TL;DR: Does not look like a bug to me.

@ambsw-technology
Copy link

FWIW I expected a TemplateHTMLRenderer to behave like the API UI.

  • On single pages, I expected the template to accept and render a single object.
  • On list pages, I expected the template to accept a list of objects and be able to render a single page that contains a representation of each of those objects.

Apparently, this is not the expected behavior. It's also non-obvious why (or even how) paging magically changes an unacceptable list into a shorter but acceptable "list".

@ambsw-technology
Copy link

I stand by my "intuitive" argument, but dug through the code to figure out why paging "fixes" the issue.
It turns out that the default paginator nests the list of objects under a results key. However, I believe this is a quirk of implementation as it would be acceptable (if not best practice) to return a simple list and place the paging metatdata in headers (like this) or exclude it entirely.

Obviously, I'm with the OP that this seems like a valid use case (albeit not a new issue). While the pagination class seems like the right place to handle this, I don't think it's practical:

  • There's a proposal to remove the Default Pagination Class so no class would even be available.
  • If the Default Pagination Class were retained, fixing this issue would spill over to all rendering engines which is presumably unwanted.

This militates for a fix that is limited to the TemplateHTMLRenderer. The two options I see are:

  1. If response.data is a list, wrap it in a key. I don't like this option because it hard codes a specific key for a particular edge case. If an actual paginator were used, the keys wouldn't necessarily match.
  2. Instead of expanding response.data into the context, always next it under a key (e.g. data). This key would be consistent regardless of the paginator or call type (instance vs. list). However, this would be backwards incompatible for current users of this Renderer.

Note that the documentation for TemplateHTMLRenderer states that that:

Unlike other renderers, the data passed to the Response does not need to be serialized

I imagine there are many unserialized objects that are not dict-like. While backwards incompatible changes are undesirable, (2) would better align the implementation with the documentation.

@ambsw-technology
Copy link

ambsw-technology commented Aug 1, 2017

... and for anyone who needs a workaround, add a line to your view like:

response.data = {'results': response.data}

or, mimicking my recommended fix (at data instead of results):

from rest_framework.renderers import TemplateHTMLRenderer
    
class MyTemplateHTMLRenderer(TemplateHTMLRenderer):
    def get_template_context(self, data, renderer_context):
        response = renderer_context['response']
        if response.exception:
            data['status_code'] = response.status_code
        return {'data': data}

@tofu-rocketry
Copy link
Contributor

I seem to have come across this issue as well. From my point of view it's a bit baffling - with a ModelViewSet, most of the renderers you'd expect to "just work" apart from TemplateHTMLRenderer. This should at least be clarified in the documentation.

Thank you to @ambsw-technology for the workarounds.

@xordoquy
Copy link
Collaborator

PR to improve the documentation is welcomed :)

tofu-rocketry added a commit to tofu-rocketry/django-rest-framework that referenced this issue Jul 25, 2018
This partially addresses encode#5236 to clarify that the response from a view may need to be modified to provide TemplateHTMLRenderer with a dict for it to use.

I do not completely understand the issue in encode#5236 so the wording could possibly be improved.
@tofu-rocketry
Copy link
Contributor

Okay. Would be useful to have someone who understands this issue better give some input on #6095.

@NDevox
Copy link

NDevox commented Jul 3, 2020

I have been bitten by this this week. And agree with the above - it doesn't feel like expected behaviour - nor is it clear what is happening immediately.

I'm quite happy to submit a PR which would wrap the list in a dict with a key of items or whatever is most appropriate (maybe results to match the pagination class).

But I do want to know if this is even desirable for the maintainers - given this issue has been left open for 2 years.

@NDevox
Copy link

NDevox commented Jul 3, 2020

FYI I've worked around this for now like so:

from rest_framework.renderers import TemplateHTMLRenderer


class MyHTMLRenderer(TemplateHTMLRenderer):
    def get_template_context(self, *args, **kwargs):
        context = super().get_template_context(*args, **kwargs)
        if isinstance(context, list):
            context = {"items": context}
        return context

Which gives me the list of objects as an "items" object in my context.

@tofu-rocketry
Copy link
Contributor

@xordoquy, so I opened a PR two years ago to improve the documentation as you suggested. A review and merge would be welcomed. 😜

@alvaroscelza
Copy link

Hello there, just jumped into this problem and found an unanswered question about it in stackoverflow: https://stackoverflow.com/q/60816455/5750078

I did not find an answer so after some debugging the DRF found out a workaround similar to those proposed here. Also wrote it as my own answer in that SO page.

This is still opened right? wouldn't a slight change in TemplateHTMLRenderer's get_template_context method suffice to fix it?

@w-
Copy link

w- commented Nov 6, 2021

Just got bitten by this. Interesting that the DRF docs which are usually very comprehensive do not cover this at all. If TemplateHTMLRenderer is not meant to work in certain scenarios by design, (at least not without tweaking) would be good to have some docs at a minimum around this.

@tofu-rocketry
Copy link
Contributor

Just got bitten by this. Interesting that the DRF docs which are usually very comprehensive do not cover this at all. If TemplateHTMLRenderer is not meant to work in certain scenarios by design, (at least not without tweaking) would be good to have some docs at a minimum around this.

From my limited understanding, I did try to clarify the docs myself: #6095

@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2022
@tofu-rocketry
Copy link
Contributor

People were still complaining about this last year. It would be good to have input from the maintainers.

@stale stale bot removed the stale label Apr 19, 2022
@stale
Copy link

stale bot commented Jun 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tofu-rocketry
Copy link
Contributor

There's a PR that seems to address this that's open.

@stale stale bot removed the stale label Oct 3, 2022
@tomchristie
Copy link
Member

Okay, so #8569 changes the behaviour more than I'd like.
I'd suggest we aim to keep things absolutely as minimal as possible here, so...

  • Add a test case that demonstrates the issue.
  • Change TemplateHTMLRenderer so that when data is a list, not a dict/dict-like then it is returned under a {"results": data} key. This has the advantage of being compatible with paginated results.

@stale
Copy link

stale bot commented Dec 24, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 24, 2022
@auvipy auvipy removed the stale label Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.