django-rest-framework icon indicating copy to clipboard operation
django-rest-framework copied to clipboard

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

Open Eskimon opened this issue 7 years ago • 22 comments

Checklist

  • [x] I have verified that that issue exists against the master branch of Django REST framework.
  • [x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • [x] This is not a usage question. (Those should be directed to the discussion group instead.)
  • [x] 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.)
  • [x] 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

Eskimon avatar Jun 26 '17 20:06 Eskimon

Thanks for raising this.

tomchristie avatar Jun 26 '17 20:06 tomchristie

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.

kezabelle avatar Jul 24 '17 14:07 kezabelle

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

ambsw-technology avatar Jul 26 '17 21:07 ambsw-technology

@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.

arijeetmkh avatar Aug 01 '17 12:08 arijeetmkh

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 avatar Aug 01 '17 15:08 ambsw-technology

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 avatar Aug 01 '17 18:08 ambsw-technology

... 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}

ambsw-technology avatar Aug 01 '17 18:08 ambsw-technology

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.

tofu-rocketry avatar Jul 24 '18 20:07 tofu-rocketry

PR to improve the documentation is welcomed :)

xordoquy avatar Jul 25 '18 09:07 xordoquy

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

tofu-rocketry avatar Jul 25 '18 09:07 tofu-rocketry

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 avatar Jul 03 '20 07:07 NDevox

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.

NDevox avatar Jul 03 '20 09:07 NDevox

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

tofu-rocketry avatar Jul 06 '20 12:07 tofu-rocketry

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?

alvaroscelza avatar Jun 06 '21 01:06 alvaroscelza

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.

w- avatar Nov 06 '21 06:11 w-

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: https://github.com/encode/django-rest-framework/pull/6095

tofu-rocketry avatar Dec 02 '21 13:12 tofu-rocketry

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[bot] avatar Apr 18 '22 10:04 stale[bot]

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

tofu-rocketry avatar Apr 19 '22 09:04 tofu-rocketry

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[bot] avatar Jun 19 '22 00:06 stale[bot]

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

tofu-rocketry avatar Oct 03 '22 12:10 tofu-rocketry

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.

tomchristie avatar Oct 04 '22 10:10 tomchristie

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[bot] avatar Dec 24 '22 04:12 stale[bot]