Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#9865 closed (fixed)

inline-edited objects with custom PKs cannot be saved

Reported by: Wojciech Bartosiak Owned by: nobody
Component: contrib.formtools Version: dev
Severity: Keywords: inlineformset_factory
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

"patients_guardian.pesel may not be NULL" when trying to save related object after "is_valid()" on "save()" method

my models are:

class Patient(models.Model):
    pesel = models.DecimalField(verbose_name=_('PESEL'), max_digits=20, decimal_places=0, primary_key=True, unique=True)
    name  = models.CharField(verbose_name=_('Imie'), max_length=20)
    surname = models.CharField(verbose_name=_('Nazwisko'), max_length=20)
    age  = models.IntegerField(verbose_name=_('Wiek'))

    def __unicode__(self):
        return '[ %s ] - %s %s' % (self.pesel, self.surname, self.name);

    class Meta:
        verbose_name = _('Pacjent')
        verbose_name_plural = _('Pacjenci')
        ordering = ['pesel']

class Guardian(models.Model):
    FAMILY_RELATIONSHIP = (    ('R', _('Rodzic')),
                               ('s', _('Prawny')),
                               ('o', _('Inne')),
                          )
    patient = models.ForeignKey(Patient)
    pesel = models.DecimalField(verbose_name=_('PESEL'), max_digits=20, decimal_places=0 ,primary_key=True, unique=True)
    name  = models.CharField(verbose_name=_('Imie'), max_length=20)
    surname = models.CharField(verbose_name=_('Nazwisko'), max_length=20)
    relationship  = models.CharField(verbose_name=_('Pokrewienstwo'), max_length=1, choices=FAMILY_RELATIONSHIP)

    def __unicode__(self):
        return '[ %s ] - %s %s' % (self.pesel, self.surname, self.name);

    class Meta:
        verbose_name = _('Opiekun')
        verbose_name_plural = _('Opiekunowie')
        ordering = ['patient', 'pesel']

and my view is:

@login_required
@transaction.commit_on_success
def new_patient_and_guardian(request):
    patient_form = modelform_factory(Patient)
    guardian_form = inlineformset_factory(Patient, Guardian, extra=1, max_num=1)
    patient_f=patient_form()
    guardian_f=guardian_form(clone(request.POST))
    error_message = ""
    if request.method=="POST":
        patient_f=patient_form(clone(request.POST))
        if patient_f.is_valid():
            try:
                patient = patient_f.save()
            
                guardian_f=guardian_form(clone(request.POST), instance=patient)
                if guardian_f.is_valid():
                    guardian_f.save()
                    transaction.commit()
                    return HttpResponseRedirect(reverse('viomed.patients.views.patient_address_edit', args=[patient.pesel]))
            except Exception, (msg):
                error_message = "%s" % (msg)
                transaction.rollback()

Attachments (1)

inline_custom_pk.diff (3.7 KB ) - added by Karen Tracey 15 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Malcolm Tredinnick, 15 years ago

Resolution: invalid
Status: newclosed

Please ask support questions on the django-users mailing list, not in Trac. There's no bug here; the error message is telling you that a field requires a value, which is correct from the definitions you have provided.

comment:2 by Wojciech Bartosiak, 15 years ago

Resolution: invalid
Status: closedreopened

No!
Correct data has been send.
When I'm using Debugger before save method all data are parsed ok. When im trying to save error occurs.

comment:3 by Wojciech Bartosiak, 15 years ago

Owner: changed from nobody to Malcolm Tredinnick
Status: reopenednew

comment:4 by Karen Tracey, 15 years ago

Owner: changed from Malcolm Tredinnick to nobody
Summary: "patients_guardian.pesel may not be NULL" when trying to save related object after "is_valid()" on "save()" methodinline-edited objects with custom PKs cannot be saved
Triage Stage: UnreviewedAccepted

Why did you assign this to Malcolm? The general custom around here is for people assign a ticket to their own self when they want to indicate they are working on it so as to avoid having other people duplicate the work. We don't in general assign things to other people, as that's rather like saying "Here, you must work on this problem for me", which is a bit rude given everyone here is a volunteer.

This ticket could have benefited from a clearer description at the outset. You didn't mention, for example, anything about the input you provided on the form. Saying you had provided a pesel value of <whatever> on the form for the guardian you were trying to create would have helped make it clear that apparently the provided value for pesel was getting lost somewhere along the way to or in save(). I realize you said the form passed validation, but it still would have been clearer to explicitly state you had provided a value for the field that was coming up "null" in the error message.

Also, complete examples are always better. You didn't provide the complete view function, nor the template you are using, so anyone trying to recreate has to fill in the blanks. It may be pretty obvious what should be filled in but it is harder to do than simply cut and paste and there's always the danger that the person attempting to recreate fills in the blanks differently than what you actually have in a way that affects the outcome.

All that said, there does seem to be a problem here. Specifically if an inline-edited model has a custom non-autofield primary key that is not the foreign key back to the parent object, then the assigned-in-the-form primary key value is not included when saving, leading to the database raising an error because the primary key field is neither an Auto field nor allowed to be null.

I'll attach a patch with a testcase demonstrating the problem and a potential fix, but this is not code I am very comfortable with so I'd prefer someone else look at it before committing.

The errors from the new test without the code fix are:

======================================================================
FAIL: Doctest: modeltests.model_formsets.models.__test__.API_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kmt/tmp/django/trunk/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for modeltests.model_formsets.models.__test__.API_TESTS
  File "/home/kmt/tmp/django/trunk/tests/modeltests/model_formsets/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/kmt/tmp/django/trunk/tests/modeltests/model_formsets/models.py", line ?, in modeltests.model_formsets.models.__test__.API_TESTS
Failed example:
    formset.save()
Exception raised:
    Traceback (most recent call last):
      File "/home/kmt/tmp/django/trunk/django/test/_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest modeltests.model_formsets.models.__test__.API_TESTS[99]>", line 1, in <module>
        formset.save()
      File "/home/kmt/tmp/django/trunk/django/forms/models.py", line 389, in save
        return self.save_existing_objects(commit) + self.save_new_objects(commit)
      File "/home/kmt/tmp/django/trunk/django/forms/models.py", line 424, in save_new_objects
        self.new_objects.append(self.save_new(form, commit=commit))
      File "/home/kmt/tmp/django/trunk/django/forms/models.py", line 487, in save_new
        return save_instance(form, new_obj, exclude=[self._pk_field.name], commit=commit)
      File "/home/kmt/tmp/django/trunk/django/forms/models.py", line 74, in save_instance
        instance.save()
      File "/home/kmt/tmp/django/trunk/django/db/models/base.py", line 328, in save
        self.save_base(force_insert=force_insert, force_update=force_update)
      File "/home/kmt/tmp/django/trunk/django/db/models/base.py", line 400, in save_base
        result = manager._insert(values, return_id=update_pk)
      File "/home/kmt/tmp/django/trunk/django/db/models/manager.py", line 138, in _insert
        return insert_query(self.model, values, **kwargs)
      File "/home/kmt/tmp/django/trunk/django/db/models/query.py", line 894, in insert_query
        return query.execute_sql(return_id)
      File "/home/kmt/tmp/django/trunk/django/db/models/sql/subqueries.py", line 309, in execute_sql
        cursor = super(InsertQuery, self).execute_sql(None)
      File "/home/kmt/tmp/django/trunk/django/db/models/sql/query.py", line 1756, in execute_sql
        cursor.execute(sql, params)
      File "/home/kmt/tmp/django/trunk/django/db/backends/sqlite3/base.py", line 170, in execute
        return Database.Cursor.execute(self, query, params)
    IntegrityError: model_formsets_bookwithcustompk.my_pk may not be NULL
----------------------------------------------------------------------
File "/home/kmt/tmp/django/trunk/tests/modeltests/model_formsets/models.py", line ?, in modeltests.model_formsets.models.__test__.API_TESTS
Failed example:
    for book in author.bookwithcustompk_set.all():
        print book.title
Expected:
    Les Fleurs du Mal
Got nothing


----------------------------------------------------------------------
Ran 559 tests in 387.581s

FAILED (failures=1)
Destroying test database...

With the code fix, all existing plus the new test pass (tested on sqlite).

by Karen Tracey, 15 years ago

Attachment: inline_custom_pk.diff added

comment:5 by Brian Rosner, 15 years ago

Good job tracking this down Karen. This certainly does look to be fixing the correct problem. The only suggestion I would like to make is that I would much prefer to see a declarative if / else conditional than the boolean logic done here because the former is much easier to read/understand going forward.

comment:6 by Karen Tracey, 15 years ago

Resolution: fixed
Status: newclosed

(In [9664]) Fixed #9865 -- Allow saving of new inline-edited objects with custom non-auto primary key fields that are not the foreign key to the parent object.

comment:7 by Karen Tracey, 15 years ago

(In [9665]) [1.0.X] Fixed #9865 -- Allow saving of new inline-edited objects with custom non-auto primary key fields that are not the foreign key to the parent object.

r9664 from trunk.

comment:8 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

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