Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20643 closed Bug (fixed)

Examples in Chapter Using mixins with class-based views

Reported by: michal@… Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I tried to use following examples:
https://docs.djangoproject.com/en/dev/topics/class-based-views/mixins/

import json
from django.http import HttpResponse
class JSONResponseMixin(object):
    response_class = HttpResponse
    def render_to_response(self, context, **response_kwargs):
        response_kwargs['content_type'] = 'application/json'
        return self.response_class(
            self.convert_context_to_json(context),
            **response_kwargs
        )
    def convert_context_to_json(self, context):
        return json.dumps(context)


from django.views.generic.detail import SingleObjectTemplateResponseMixin
class HybridDetailView(JSONResponseMixin, SingleObjectTemplateResponseMixin, BaseDetailView):
    def render_to_response(self, context):
        # Look for a 'format=json' GET argument
        if self.request.GET.get('format','html') == 'json':
            return JSONResponseMixin.render_to_response(self, context)
        else:
            return SingleObjectTemplateResponseMixin.render_to_response(self, context)

It seems to me, that attribute response_class = HttpResponse in JSONReponseMixin overlaps attribute response_class = TemplateResponse in TemplateResponseMixin (and SingleObjectTemplateResponseMixin) and breaks non-json requests.

Following code works but doesn't look like proper solution to me:

import json
from django.http import HttpResponse
class JSONResponseMixin(object):
    def render_to_response(self, context, **response_kwargs):
        response_kwargs['content_type'] = 'application/json'
        return HttpResponse(
            self.convert_context_to_json(context),
            **response_kwargs
        )
    def convert_context_to_json(self, context):
        return json.dumps(context)

Michal

Attachments (1)

ticket-20643.patch (4.1 KB) - added by bmispelon 2 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Hi,

Thanks for catching this. I actually fixed up a bunch of similar mistakes in the CBV topic docs yesterday but I missed this one.

I think the way that JSONResponseMixin is written makes it so that it's hard to implement HybridDetailview correctly.

The solution you propose doesn't seem so bad to me.
The idea is to keep JSONResponseMixin.render_to_response self-contained so that it can be more easily mixed-in without interfering too much.

I would actually go one step further and remove render_to_response from JSONResponseMixin in favor of a dedicated render_to_json_response method (see attached path), making it the view's responsability to call the prefered rendering method.

It makes things a bit more verbose and the documentation needs to be changed quite a bit more, but I think it makes things a bit cleaner.
What do you think?

Changed 2 years ago by bmispelon

comment:2 follow-up: Changed 2 years ago by loic84

Btw, is there any reason to write:

class HybridDetailView(JSONResponseMixin, SingleObjectTemplateResponseMixin, BaseDetailView): 

Instead of :

class HybridDetailView(JSONResponseMixin, DetailView): 

Other than that, I'm happy with the fix.

comment:3 follow-up: Changed 2 years ago by anonymous

I have two suggestions.

Would it be possible to mention somewhere in the docs (chapter Using Django/Class-based views or API Reference/Class-based views) following site? It's extremely useful for everyone working with class-based views:
http://ccbv.co.uk/

It would be also nice to mention some tips how to handle the conversion of context to json. For example model_to_dict method can be useful when processing single objects:

from django.forms.models import model_to_dict
...
def convert_context_to_json(self, context):
    return json.dumps(model_to_dict(context['object']))

Or we can show some custom JSON encoders for handling datetime.date objects etc.


comment:4 in reply to: ↑ 3 Changed 2 years ago by msladek

I forgot to login before sending my previous comment...

Here is an article showing another way of model to dictionary conversion:
http://www.pythondiary.com/tutorials/django-and-ajax-jquery.html

Replying to anonymous:

I have two suggestions.

Would it be possible to mention somewhere in the docs (chapter Using Django/Class-based views or API Reference/Class-based views) following site? It's extremely useful for everyone working with class-based views:
http://ccbv.co.uk/

It would be also nice to mention some tips how to handle the conversion of context to json. For example model_to_dict method can be useful when processing single objects:

from django.forms.models import model_to_dict
...
def convert_context_to_json(self, context):
    return json.dumps(model_to_dict(context['object']))

Or we can show some custom JSON encoders for handling datetime.date objects etc.


comment:5 Changed 2 years ago by Baptiste Mispelon <bmispelon@…>

  • Resolution set to fixed
  • Status changed from new to closed

In cd000dacc7cef8e55793d19242d0df0f3e736949:

Fixed #20643 -- Fixed implementation of JSONResponseMixin in CBV docs

Thanks to Michal Sládek for the report and initial patch,
and to loic84 for the review.

comment:6 in reply to: ↑ 2 Changed 2 years ago by bmispelon

(to loic84)
You're right, since DetailView inherits from SingleObjectTemplateResponseMixin and BaseDetailView, we could switch the two.

I suspect this was done to show that the view was made of two *ResponseMixin, without having to spend time explaining the inheritance chain of DetailView.
However, I think it works fine this way, so I decided not to change it.

(to msladek)
I decided against adding a link to ccbv.co.uk.
While I agree that this tool is very useful, I'd rather see Django's documentation be improved than having to rely on external tools to compensate.

I did however add a note linking to the documentation that explains how to serialize django models properly [1].

Thanks to both of you.

[1] https://docs.djangoproject.com/en/1.5/topics/serialization/

Note: See TracTickets for help on using tickets.
Back to Top