Opened 3 years ago

Closed 3 years ago

#18181 closed Uncategorized (invalid)

Not filling out a blank=True ForeignKey admin raw_id field raises ValueError because it tries to set the foreign key to None

Reported by: jaylett Owned by: jaylett
Component: contrib.admin Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

For a model with:

    created_by = models.ForeignKey(User, related_name='created_%(class)s_set', null=False, blank=True)

(via an abstract base model, hence the pattern in related_name). Submitting an admin with created_by in raw_id_fields, but not filling out the field (ie leaving it blank) results in the following backtrace:

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 111, in get_response
  response = callback(request, *callback_args, **callback_kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/contrib/admin/options.py", line 307, in wrapper
  return self.admin_site.admin_view(view)(*args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py", line 93, in _wrapped_view
  response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/cache.py", line 79, in _wrapped_view_func
  response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/contrib/admin/sites.py", line 197, in inner
  return view(request, *args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py", line 28, in _wrapper
  return bound_func(*args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py", line 93, in _wrapped_view
  response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py", line 24, in bound_func
  return func(self, *args2, **kwargs2)
File "/usr/local/lib/python2.7/dist-packages/django/db/transaction.py", line 217, in inner
  res = func(*args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/contrib/admin/options.py", line 864, in add_view
  if form.is_valid():
File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py", line 121, in is_valid
  return self.is_bound and not bool(self.errors)
File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py", line 112, in _get_errors
  self.full_clean()
File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py", line 269, in full_clean
  self._post_clean()
File "/usr/local/lib/python2.7/dist-packages/django/forms/models.py", line 308, in _post_clean
  self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
File "/usr/local/lib/python2.7/dist-packages/django/forms/models.py", line 50, in construct_instance
  f.save_form_data(instance, cleaned_data[f.name])
File "/usr/local/lib/python2.7/dist-packages/django/db/models/fields/__init__.py", line 416, in save_form_data
  setattr(instance, self.name, data)
File "/usr/local/lib/python2.7/dist-packages/django/db/models/fields/related.py", line 327, in __set__
  (instance._meta.object_name, self.field.name))
ValueError: Cannot assign None: “Collection.created_by” does not allow null values.

It should instead display a validation failure for that field in the admin page.

Forcibly setting required=True on the field as created in django.contrib.admin.options:BaseModelAdmin.formfield_for_foreignkey() (or in the ModelAdmin subclass in the app's admin.py) results in the desired behaviour, but probably isn't actually the correct approach.

This is similar to #9484, which was deduped into #8746, the final patch for which didn't deal with this, although it did deal with ValueError in another situation. See also a late comment on 8746 which is the same as the issue here, and should have become a new ticket (and may have done, but I can't find it).

It is possible that this has been fixed in 1.4, which I can't migrate to yet; if so, then at least this ticket will capture that for anyone else who has the problem while 1.3 is still common.

Change History (5)

comment:1 Changed 3 years ago by koenb

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Is there really a problem or bug here?

You specified null=False and blank=True, which means that the database will not accept a null value, but a modelform will allow leaving this empty. This combination is perfectly acceptable if you add the value to your model instance programmatically in case the user leaves it empty. You obviously have not done that, hence the ValueError on saving.

My guess is you really want to be using blank=False here, though of course I may have misunderstood your intentions. Please reopen if that is the case.

comment:2 Changed 3 years ago by jaylett

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from Not filling out a required ForeignKey admin raw_id field raises ValueError rather than showing a validation failure to Not filling out a blank=True ForeignKey admin raw_id field raises ValueError because it tries to set the foreign key to None

Sorry, I confused myself and so both missed out some important details and then came to utterly the wrong confusion. Reopened and the missing information is:

The ForeignKey is managed automatically in a presave hook if not already set at save time. However the admin form is trying to se the ForeignKey to None, which fails. Ignore everything after the stack track in the original description. I believe the admin should not try to set ForeignKey fields if they are a raw_id_field and the field in the admin interface was left blank.

comment:3 Changed 3 years ago by jaylett

Currently working this into a new ticket that makes it clear what's going on, why I think it's wrong, and possibly figuring out a test for it at the same time. I'll close this once I've got the new ticket there.

comment:4 Changed 3 years ago by jaylett

  • Owner changed from nobody to jaylett
  • Status changed from reopened to new

comment:5 Changed 3 years ago by jaylett

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

1.4 turns out to be more of a design or documentation issue. Closing as invalid.

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