Class based generic DetailView tries to access non-existant _meta field when get_object has been modified to return a ModelForm

[kd4ttc]

Summary

I used DetailView and subclassed it, then modified get_object to return a ModelForm instance. When it executes an eeror message is generated due to the Meta class in ModelForm clashing with the expected Meta attributes in the usual object that is passed. This was initially posted to stackoverflow ​http://stackoverflow.com/q/6564068/820321 where the formatting is pleasant to read. This is my first bug report, so the formatting of the bug report may not be that good. Apologies in advance.

Report Details

I've been impressed how rapidly a functional website can go together with generic views in the tutorials. Also, the workflow for form processing is nice. I used the ModelForm helper class to create a form from a model I made and was delighted to see that so much functionality came together. When I used the generic list_detail.object_detail I was disappointed that all that I could display were fields individually. I knew the ModelForm class contained information for rendering, so I wanted to use the ModelForm with a generic view.

I was asking around on stackoverflow to get some direction, and appreciate the answers and comments from several posters. I've figured out how to get this to work, but there is a bug in DetailView. The solution includes a workaround.

To use a ModelView with the generic view and get all the fields to render automatically the following works:

Create a project, and in it create application inpatients.

If you have

# inpatients/models.py

class Inpatient(models.Model):
    last_name = models.CharField(max_length=30)
    first_name = models.CharField(max_length=30,blank=True)
    address = models.CharField(max_length=50,blank=True)
    city = models.CharField(max_length=60,blank=True)
    state = models.CharField(max_length=30,blank=True)
    DOB = models.DateField(blank=True,null=True)
    notes = models.TextField(blank=True)

def  __unicode__(self):
        return u'%s, %s %s' % (self.last_name, self.first_name, self.DOB)

class InpatientForm(ModelForm):
    class Meta:
        model = Inpatient

and

# inpatients/views.py

from django.http import HttpResponse, HttpResponseRedirect
from django.shortcuts import render_to_response
from django.views.generic import DetailView
from portal.inpatients.models import *

def formtest(request):
    if request.method == 'POST':
        form = InpatientForm(request.POST)
        if form.is_valid():
            form.save()
            return HttpResponseRedirect('/inpatients')
    else:
        form = InpatientForm()
    return render_to_response("formtest.html", {'form': form})

class FormDetailView(DetailView):
    model=Inpatient
    context_object_name='inpatient'   # defines the name in the template
    template_name_field='inpatient_list_page.html'

    def get_object(self):
        inpatient=super(FormDetailView,self).get_object()
        form=InpatientForm(instance=inpatient)
        return form

    def get_template_names(self):
        return ['inpatient_list_page.html',]

and

#urls.py

from django.conf.urls.defaults import patterns, include, url
from django.views.generic import ListView
from portal.inpatients.models import Inpatient, InpatientForm
from portal.inpatients.views import FormDetailView

urlpatterns = patterns('',
    (r'^formtest/$','portal.inpatients.views.formtest'),
    (r'^inpatients/$', ListView.as_view(
        model=Inpatient, template_name='inpatient_list_page.html')),
    (r'^inpatient-detail/(?P<pk>\d+)/$', FormDetailView.as_view()),
)

# with a template containing

{% block content %}
    <h2>Inpatients</h2>
    <ul>
        {% for aninpatient in object_list %}
            <li><a href='/inpatient-detail/{{ aninpatient.id }}/'>
            {{ aninpatient }}, id={{ aninpatient.id }}</a></li>
        {% endfor %}
    </ul>
    {{ inpatient.as_p }}
{% endblock %}
# Yeah, kind of hokey. The template is for both the list view and detail view. 
# Note how the form is rendered with one line - {{ inpatient.as_p }}

t works. The instructions for using class based generic views lives at ​https://docs.djangoproject.com/en/1.3/topics/class-based-views/ Instructions there are pretty clear. The key to making things work is to redefine get_object. In the documentation under the section "Performing extra work" it nicely describes how to do this, the steps being to call the original version of get_object, and then to the extra work. The bit that I realized is that the return object can be a ModelForm object. The object that get_object returns goes straight into the template in a render. By taking the retrieved inpatient object and running it through InpatientForm it can be passed to a view as a form which then renders itself.

As to the bug: The bug in DetailView is that the get_template_names function tries to make a template name from a structure that does not exist. In https://code.djangoproject.com/browser/django/trunk/django/views/generic/detail.py on lines 127 to 140 we have within SingleObjectTemplateResponseMixin.get_template_names:

127        # The least-specific option is the default <app>/<model>_detail.html;
128         # only use this if the object in question is a model.
129         if hasattr(self.object, '_meta'):
130             names.append("%s/%s%s.html" % (
131                 self.object._meta.app_label,
132                 self.object._meta.object_name.lower(),
133                 self.template_name_suffix
134             ))
135         elif hasattr(self, 'model') and hasattr(self.model, '_meta'):
136             names.append("%s/%s%s.html" % (
137                 self.model._meta.app_label,
138                 self.model._meta.object_name.lower(),
139                 self.template_name_suffix
140             ))

The error is that the code on line 131 is executed and dies with error message <'ModelFormOptions' object has no attribute 'app_label'>. I conclude that the _meta object is defined. I suppose that the problem is that in a ModelForm the class Meta is defined. That Meta probably doesn't have the fields set that are expected. The workaround is just to rewrite get_template_names and return the correct template.

I suppose a try statement could just go around where the assignments are done. The other issue is to decide what the best way is to define the template when a ModelForm is used.

Some Feedback and Reply on StackOverflow

I don't think this is a bug, and I do think get_object should always return model instance not ModelForm instance. Try using editing CBV. – rebus

I think it is a bug for several reasons. The documentation does not say it is invalid. The test for valid data before the assignment tests for the existence of _meta rather than the actual fields. The routine that is looking for the template didn't find the template. Additionally, on the principal of Don't Repeat Yourself, the ModelForm should be able to be delivered to a template for rendering. – kd4ttc

Another User Had Additional Insight

You are right I believe. This is a bug which stems from the fact that both ModelForm and Models have a _meta attribute. This same bug would exhibit itself anytime an object is returned from get_object() that contains a _meta attribute.

get_object does not have to return a Model instance. You can confirm this by looking at the source for DetailView and reading it's docstring:

class DetailView(SingleObjectTemplateResponseMixin, BaseDetailView):
    """
    Render a "detail" view of an object.

    By default this is a model instance looked up from `self.queryset`, but the
    view will support display of *any* object by overriding `self.get_object()`.
    """

Notice that the doc string explicitly says that any object is supported by overriding self.get_object().

Another piece of corroborating evidence is from the location where this bug itself occurs which is the get_template_names method of SingleObjectTemplateResponseMixin.

    # The least-specific option is the default <app>/<model>_detail.html;
    # only use this if the object in question is a model.
    if hasattr(self.object, '_meta'):
        names.append("%s/%s%s.html" % (
            self.object._meta.app_label,
            self.object._meta.object_name.lower(),
            self.template_name_suffix
        ))
    elif hasattr(self, 'model') and hasattr(self.model, '_meta'):
        names.append("%s/%s%s.html" % (
            self.model._meta.app_label,
            self.model._meta.object_name.lower(),
            self.template_name_suffix
        ))

Again looking at this code, the comment itself say "If the object in question is a model". From this comment we can infer that the object doesn't always have to be a model.

-- Donald Stufft

[Matt McClanahan]

Ignoring whether or not this is a bug for the moment, if the primary object of your view is a form, wouldn't it work better to use CreateView, UpdateView, or your own view that pulls in ModelFormMixin?

[kd4ttc]

What I did was generate a list of entries with a generic view and then want to drill down to review the entries in all the fields. If it needs editing there are controls that will then go to a modification page. I will be trying out the generic edit views next. A similar bug might be there, too.

[kd4ttc]

BTW I thought it was pretty well documented. What other documentation is needed? Should I propose a patch? What tests are needed?(this is my first bug report)

[Aymeric Augustin]

Based on the evidence provided in the original report, I'd say this attempt to check if self.object is a model is incorrect:

if hasattr(self.object, '_meta'):

It should be replaced by something along these lines:

from django.db import models
if isinstance(self.object, models.Model)

[Donald Stufft]

​https://github.com/django/django/pull/52

[Łukasz Rekucki]

I have to disagree. Doing this kind of explicit checks for types is not very pythonic, unless you plan to change models.Model into an ABC. Duck typing is usefull. Sure, there will be cases where attribute names overlap and lead to some confusion, but those are fairly rare.

Lets not forget that the OP's problem, comes from misuse of the API (DetailView is designed to display instances of Model or objects behaving similar). @kd4ttc, if you need some extra objects (like a form) for rendering you can put the into context by overriding get_context_data(), no need to hack through the whole API.

[Donald Stufft]

Doing the explicit checks for types here makes sense because all Django models must be derived from models.Model and the generic views are using the binary "is this a model y/n" question to kick in additional functionality. Essentially it's already doing a check for type, it's just doing so poorly because it assumes that only a Model would have a _meta attribute.

And DetailView is _not_ designed only for model instances or objects behaving similarly, it is designed for an instance of an object regardless of how it behaves. This is evident through both the doc strings and the conditionals. While a model instance might be the most common use case, it is not the only valid use case as designed.

Because this view not designed only for model instances, and because it wants to enable additional functionality when the object is a model the only sane choices, in my opinion, are to explicitly check for subclasses, or make the _meta api a public api so that people can make their objects conform to it. Without a public api you cannot expect people to even know that _meta exists. A quick grep of the docs show that _meta existing is only incidentally documented through it's use in example code of other features.

[Anssi Kääriäinen]

I agree that isinstance check is the correct one here. I don't see the value of duck typing in this case, it is not at all well defined what behavior the duck should have. It just having the _meta/model attribute is _not_ what we are interested in. So, isinstance is valid here.

[Anssi Kääriäinen <akaariai@…>]

In [484fcd34a4819ff11ef72f3c9e1eb467a282b2f6]:

Fixed #16418 -- Made generic views work with ModelForms

Generic views assumed any object's _meta will be model Options. This
is not true for ModelForms for example. Took isinstance(obj, Model)
in use instead.