Code

Opened 3 years ago

Closed 10 months ago

#16502 closed Bug (fixed)

CreateView useless error message when template_name is not specified

Reported by: silverghost3@… Owned by: ianawilson
Component: Generic views Version: master
Severity: Normal Keywords: CreateView "generic view"
Cc: Silver_Ghost, anton@…, bhuztez, tinodb, preston@…, ianawilson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

According to documentation CreateView should use %app_name%/%model_name%_form.html template by default. But if template_name is not specified it returns uninformative error:

Traceback (most recent call last):

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/core/servers/basehttp.py", line 283, in run
    self.result = application(self.environ, self.start_response)

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/contrib/staticfiles/handlers.py", line 68, in __call__
    return self.application(environ, start_response)

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/core/handlers/wsgi.py", line 273, in __call__
    response = self.get_response(request)

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/core/handlers/base.py", line 169, in get_response
    response = self.handle_uncaught_exception(request, resolver, sys.exc_info())

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/core/handlers/base.py", line 203, in handle_uncaught_exception
    return debug.technical_500_response(request, *exc_info)

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/views/debug.py", line 59, in technical_500_response
    html = reporter.get_traceback_html()

  File "/home/kirill/workplace/projects/createview_test/lib/python2.7/site-packages/django/views/debug.py", line 89, in get_traceback_html
    for loader in template_source_loaders:

TypeError: 'NoneType' object is not iterable

There is not anything except traceback on white background.

It will be cool if there will be default template for CreateView. If it is not a bug then note that template_name is required would be useful.

Attachments (6)

handle_empty_list_of_templates_r16549.diff (614 bytes) - added by Silver_Ghost 3 years ago.
Adds check for non-emptyness for list of template_names in django.template.loader.select_template.
SingleObjectMixin-get_model.diff (3.4 KB) - added by bhuztez 3 years ago.
add a get_model to SingleObjectMixin?
aggregated_patch_with_tests.diff (7.3 KB) - added by Silver_Ghost 3 years ago.
merging of patches and adding regression tests
get_model_with_tests.diff (6.2 KB) - added by Silver_Ghost 3 years ago.
Separate get_model-patch.
get_model_with_tests.2.diff (6.7 KB) - added by bhuztez 2 years ago.
update patch for revision 17517
get_model_with_tests.3.diff (6.8 KB) - added by bhuztez 2 years ago.
update patch for revision 17904

Download all attachments as: .zip

Change History (35)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Actually, this is a crash of the debug view itself.

When Django encounters a TemplateDoesNotExist exception, the debug view attempts to gather information about available template loaders and templates. It relies on the fact that django.template.loaders.template_source_loaders is already populated (by django.template.loaders.find_template). But in your case, it isn't. So a new exception is raised, it overrides the initial exception, and — unfortunately — it makes it difficult to understand what really happens here.

Changed 3 years ago by Silver_Ghost

Adds check for non-emptyness for list of template_names in django.template.loader.select_template.

comment:2 Changed 3 years ago by Silver_Ghost

  • Owner changed from nobody to Silver_Ghost
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

aaugustin, through information from your comment I was able to find possible place in code where additional check could be perfomed. It is django.template.loader.select_template function.

It expects list of template names and select first loadable. If there are some template names in list and no one from them couldn't be loaded then raising TemplateDoesNotExist exception is the right decision since. But if there isn't any template names in the list then >>Template<<DoesNotExist is wrong exception, I think.

So maybe checks if list is empty and raise exception with message like "I can't load any template for you because you didn't gave me any possible variants"?

comment:3 Changed 3 years ago by Silver_Ghost

  • Has patch set

comment:4 Changed 3 years ago by Silver_Ghost

  • Version changed from 1.3 to SVN

Changed 3 years ago by bhuztez

add a get_model to SingleObjectMixin?

comment:5 Changed 3 years ago by zuko

  • Cc anton@… added

comment:6 Changed 3 years ago by bhuztez

  • Cc bhuztez added

function-based generic view, django.views.generic.create_update.create_object, use default template if no template_name is given. When you switch to class-based view, CreateView will not work as you has expected, since django.views.generic.edit.CreateView has no default template. This must be a bug, please give CreateView a default template in 1.3.

comment:7 Changed 3 years ago by tinodb

  • Cc tinodb added
  • Needs tests set

Changed 3 years ago by Silver_Ghost

merging of patches and adding regression tests

comment:8 Changed 3 years ago by Silver_Ghost

  • Needs tests unset

I've merged my and bhustez 's patches and added regression tests for both.

comment:9 Changed 3 years ago by ptone

  • Cc preston@… added

comment:10 Changed 3 years ago by ptone

  • Patch needs improvement set

There are too many issues being addressed here in one patch.

The ticket is valid and seeks the addition of a default template to CreateView

Patches addressing how the template loader works, or a missing get_model method on SingleObjectMixin should have their own tickets opened and patches submitted.

Changed 3 years ago by Silver_Ghost

Separate get_model-patch.

comment:11 Changed 3 years ago by Silver_Ghost

  • Easy pickings set
  • Patch needs improvement unset

According to this discussion patch for select_template was separated to ticket:16866. Patch with get_model was left here since there are multiple ways to define which model should be used (the self.model, self.queryset or self.form_class attributes) and it justifies having a utility function for it.

Tests provided.

comment:12 Changed 2 years ago by charettes

Just hit this issue, any chances this get attention for 1.4?

Changed 2 years ago by bhuztez

update patch for revision 17517

comment:13 Changed 2 years ago by bhuztez

update Silver_Ghost's patch for revision 17517

Changed 2 years ago by bhuztez

update patch for revision 17904

comment:14 Changed 2 years ago by aaugustin

  • Owner changed from Silver_Ghost to aaugustin
  • Status changed from assigned to new

comment:16 follow-up: Changed 2 years ago by andrewgodwin

  • Patch needs improvement set

I don't like the way this patch/pull request works with ModelForms - it magically extracts a model from a ModelForm, which already needs discussion as it's new behaviour, but even worse it then passes that model out and then makes a brand new ModelForm out of it - that shouldn't happen.

comment:17 in reply to: ↑ 16 Changed 23 months ago by bhuztez

I don't like the way this patch/pull request works with ModelForms - it magically extracts a model from a ModelForm, which already needs discussion as it's new behaviour

Yes, but it is not a new behaviour, the function-based generic view counterpart did the same magic. I did not know whether or not the design decision had been changed to not providing a default template_name. To provide a default template_name, I don't think there is a much less magical way , given current ModelForm API.

https://github.com/django/django/blob/stable/1.4.x/django/views/generic/create_update.py#L29

but even worse it then passes that model out and then makes a brand new ModelForm out of it - that shouldn't happen.

No, it does not. If form_class already exists, it will NOT make a new ModelForm from model extracted from that, it will just return form_class you defined. The function-based generic view counterpart did the same.

comment:18 Changed 22 months ago by aaugustin

  • Owner changed from aaugustin to nobody

comment:19 Changed 14 months ago by krak3n

  • Owner changed from nobody to krak3n
  • Status changed from new to assigned

comment:20 follow-up: Changed 14 months ago by krak3n

  • Patch needs improvement unset
  • Resolution set to worksforme
  • Status changed from assigned to closed

I was unable to duplicate this in 1.5.1.

I created a basic model as below:

#models.py

from django.db import models

class Author(models.Model):
    name = models.CharField(max_length=100)

A basic view:

#views.py

from django.views.generic import CreateView
from .models import Author

class CreateAuthor(CreateView):
    model = Author

The traceback I got back was:

TemplateDoesNotExist at /
test_16502/author_form.html
Request Method:	GET
Request URL:	http://10.10.10.10:9000/
Django Version:	1.5.1
Exception Type:	TemplateDoesNotExist
Exception Value:	
test_16502/author_form.html
Exception Location:	/home/vagrant/django/django/django/template/loader.py in select_template, line 194
Python Executable:	/home/vagrant/.virtualenvs/django/bin/python
Python Version:	2.7.3

I think this is the correct exception that should be raised and the exception is present in the regular debug view.

Perhaps this was an issue with earlier versions of Django and it's been resolved in another ticket, though I can't hunt this down. Perhaps related to ticket:16866?

Perhaps if this is still a bug provide more information on how to reproduce it.

comment:21 in reply to: ↑ 20 Changed 14 months ago by bhuztez

  • Resolution worksforme deleted
  • Status changed from closed to new

comment:22 follow-up: Changed 12 months ago by timo

  • Patch needs improvement set

comment:23 in reply to: ↑ 22 Changed 10 months ago by magicharshit

Replying to timo:

comment:24 follow-up: Changed 10 months ago by jambonrose

This is all a little convoluted. There is no longer one problem at hand, and a little clarification might be useful.

Bug 1: get_template_names() (as defined in SingleObjectTemplateResponseMixin) is returning None, which is causing Django to throw a TemplateDoesNotExist. This should instead throw a ImproperlyConfigured error, as it does not have the information to determine the template file to load. This is more eloquently described in #18853 (marked as duplicate to this topic)

Bug 2: The TemplateDoesNotExist exception is causing the server error message, as detailed (and solved) in #21058.

Feature Request 1: Creating a CBGV by only overriding the form_class variable. The patch provided creates the ability to do so, but does not actually solve the bugs detailed.

I spoke to Russel about the possibility of the new feature. Unfortunately, determining the model based off a form specified in form_class is not desirable, because this assumes the form is a ModelForm, which may not be the case. As such, this feature (and patch) will not be approved for Django.

This leaves only Bug 1 to be solved.

Last edited 10 months ago by jambonrose (previous) (diff)

comment:25 Changed 10 months ago by ianawilson

  • Cc ianawilson added
  • Owner changed from krak3n to ianawilson
  • Patch needs improvement unset
  • Status changed from new to assigned

Here is a pull request to fix Bug 1, as described by jambonrose.

https://github.com/django/django/pull/1580

Last edited 10 months ago by ianawilson (previous) (diff)

comment:26 Changed 10 months ago by Russell Keith-Magee <russell@…>

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

In 9b2dc12b8332389d1bfb9e83123a088a084a6a47:

Merge pull request #1580 from ianawilson/ticket_16502

Fixed #16502 -- Fixed a TemplateDoesNotExist error that should be an ImproperlyConfigured.

Assistance on the patch from #jambronrose.

comment:27 Changed 10 months ago by Russell Keith-Magee <russell@…>

In 99952bab3008622b05613ed6ec54c3e1c63c0a1d:

[1.6.x] Merge pull request #1580 from ianawilson/ticket_16502

Fixed #16502 -- Fixed a TemplateDoesNotExist error that should be an ImproperlyConfigured.

Assistance on the patch from #jambronrose.

Backport of 9b2dc12b8332389d1bfb9e83123a088a084a6a47 from master.

comment:28 in reply to: ↑ 24 Changed 10 months ago by bhuztez

  • Resolution fixed deleted
  • Status changed from closed to new

determining the model based off a form specified in form_class is not desirable, because this assumes the form is a ModelForm, which may not be the case.

CreateView inherits ModelFormMixin. This already assumes form is a ModelForm.

comment:29 Changed 10 months ago by timo

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

@bhuztez I'm going to re-close this and ask that you please open a new ticket since we try to have one issue per ticket. I think the feature request and patch look reasonable, but it's a bit difficult to follow the conversation. Could you please a new ticket that summarizes the details and include the most recent patch? I think the existing patch also needs documentation. Thanks!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.