OneToOnes + primary_key = True ... fails in Admin

[magneto]

This goes back to the primary keys and missing in the Admin forms as hidden fields, but it also effect 2 things . I've brought this up before #7938 for instance, where a slightly different bug was mended.

1) Addition of an element
2) editing a OneToOne directly

#create a simple model
from django.db import models

class BaseMoo(models.Model):
	name = models.CharField(max_length = 50)
	is_moo = models.BooleanField(default = True)


class MooOne(models.Model):
	basemoo = models.OneToOneField(BaseMoo, primary_key = True)
	is_moo_one = models.BooleanField(default = True)

#create the ADmin
from testmod.models import BaseMoo, MooOne

from django.contrib import admin

class MooOne_Inline(admin.StackedInline):
	model = MooOne
	extra = 1
	max_num = 1
	raw_id_fields = ('basemoo',)
	
class BaseMooOptions(admin.ModelAdmin):
	inlines = [MooOne_Inline]
	list_display = ('name', 'is_moo',)

class MooOneOptions(admin.ModelAdmin):
	raw_id_fields = ('basemoo',)
	list_display = ('basemoo', 'is_moo_one',)

admin.site.register(BaseMoo, BaseMooOptions)
admin.site.register(MooOne, MooOneOptions)

1) In Admin, try to 'add' a BaseMoo Object. It will give you the option for the Inlined MooOne, but on save the MooOne object is _not_ saved

2) ok a work around (but assumes that MooOne is registered as not an inline like above), so try to add the MooOne directly buy adding one attached to BaseMoo ..

3) Go back to the BaseMoo object, try to edit and save .. boom

Traceback:
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/contrib/admin/sites.py" in root
  173.                 return self.model_page(request, *url.split('/', 2))
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/views/decorators/cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/contrib/admin/sites.py" in model_page
  192.         return admin_obj(request, rest_of_url)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/contrib/admin/options.py" in __call__
  191.             return self.change_view(request, unquote(url))
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/db/transaction.py" in _commit_on_success
  238.                 res = func(*args, **kw)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/contrib/admin/options.py" in change_view
  573.                     self.save_formset(request, form, formset, change=True)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/contrib/admin/options.py" in save_formset
  373.         formset.save()
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/forms/models.py" in save
  280.         return self.save_existing_objects(commit) + self.save_new_objects(commit)
File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/django/forms/models.py" in save_existing_objects
  294.             obj = existing_objects[form.cleaned_data[self.model._meta.pk.attname]]

Exception Type: KeyError at /testmod/basemoo/34/
Exception Value: 'basemoo_id'

4) Ok so now go back to the MooOne raw object and try to edit it. Well you can, except now the raw_id_fields of 'BaseMoo" is NOT filled in automatically, (If i turn off raw_id_fields, the select box does _not_ choose the current BaseMoo Object). Meaning the 'save' will fail until you pick the old object again.

I Imagine all of these little issues are related to the primary_key vs autofield display bits in admin.

[magneto]

I Guess the "raw_id_fields" in the subject line here is misleading .. i can repeat the above issue without any raw_id_fields defined.

[magneto]

ok, so it has again to do with the Primary key designation that was the issue in #7938.

If i change one of the Models to

class MooOne(models.Model):
	basemoo = models.ForeignKey(BaseMoo, primary_key = True, unique = True)
	is_moo_one = models.BooleanField(default = True)

Then Editing a MooOne directly pre-populates the Dropdown menu/Raw ID text box ...

The 'editing inline' for these of a BaseMoo also still fails

[magneto]

In regards to the MooOne object not begin saved on an Addition from the Inlined add of a BaseMoo.

It seems that this is an edge case. Where if i 'change' the BooleanFeild from its default "True" to "False" in the form, the MooOne object does get added.

I.e. the If the Inlined object is NOT changed from the defaults, it will not be saved.

I tried this with a second model

class MooOne(models.Model):
	basemoo = models.ForeignKey(BaseMoo, primary_key = True, unique = True)
	is_moo_one = models.BooleanField(default = True)
	txt = models.CharField(default = "",  blank = True, max_length = 50)

and

class MooOne(models.Model):
	basemoo = models.OnetoOneField(BaseMoo, primary_key = True)
	is_moo_one = models.BooleanField(default = True)
	txt = models.CharField(default = "",  blank = True, max_length = 50)
}

with the same results if both txt and is_moo_one are left in their default state at Save.

[magneto]

The small change in patch from r8528 causes the Forms not to be Auto Populated (the initial data needs both "attname" and "name" depending on the form apparently and the Field name is "name" and thus the initial data dict is looking for "name" not "attname"). The next patch, fixes this part of the ticket, but it may have other 'effects' unknown (it passes the tests at least)

[magneto]

set both the attname and name for onetoonefields

[magneto]

An even better solution that seems to solve the most of the ticket.

The method is to think of OneToOne+Primary AND ForiegnKey+primary+unique as equivalent as Hidden AutoFields in the Inline cases.

This still does not solve the Add not saving the OneToOne (or other Inline primary key type) that did not have its defaults changed (but the fields are not required). Beginning to think that a OneToOne with no required fields, perhaps should not be saved. It does leave a hole in the

my_obj.my_oneto_one

raising a "DoesNotExist" error in the reverse relations, when it probably should return None if one allows the above behavior in the Admin.

i.e. a simple if my_obj.my_oneto_one: ... instead of try/except makes more sense.

.. but i'll leave that to the rest of y'all.

[magneto]

treat OneToOne+primary & ForeignKey+unique+primary as a type of Form Autofield

[Karen Tracey]

Fist bug I see here is the KeyError on an attempt to save a model with a pre-existing inline-edited OneToOne related object.

Second is the fact that the widgets (raw or default) for the related field are not populated on the change page for the objecting containing the OneToOne field.

Both are fixed by the 8562_primary_uniques.diff, but I am not at all familiar with the code involved here so have no idea of the correctness of the fix beyond it does fix the observed symptoms.

What's not a bug is the Admin failing to create the inline-edited object in the case where you made no changes to any of its initial values. Since the admin doesn't have an explicit 'add' checkbox for adding new inlines, it relies on trying to determine if anything changed between the form as initially created and what is submitted. If they're identical, no new related object is created.

[magneto]

Replying to kmtracey:

What's not a bug is the Admin failing to create the inline-edited object in the case where you made no changes to any of its initial values. Since the admin doesn't have an explicit 'add' checkbox for adding new inlines, it relies on trying to determine if anything changed between the form as initially created and what is submitted. If they're identical, no new related object is created.

ok, just did not know if that was the desired behavior or not since the Admin interface for the onetoones was a little messed up.

[Karen Tracey]

One test fails when running with 8562_primary_uniques.diff:

======================================================================
FAIL: Doctest: modeltests.model_forms.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_forms.models.__test__.API_TESTS
  File "/home/kmt/tmp/django/trunk/tests/modeltests/model_forms/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/kmt/tmp/django/trunk/tests/modeltests/model_forms/models.py", line ?, in modeltests.model_forms.models.__test__.API_TESTS
Failed example:
    sorted(model_to_dict(bw).keys())
Expected:
    ['id', 'name', 'writer_ptr_id']
Got:
    ['id', 'name', 'writer_ptr', 'writer_ptr_id']


----------------------------------------------------------------------
Ran 515 tests in 1376.062s

FAILED (failures=1)

Someone needs to figure out if the test just needs to be updated to expect the new output or if it's a sign of a real problem.

[Karen Tracey]

#8674 looks like another report of the related field widget not displaying the correct related object.

[magneto]

It seems that the test needs to be updated (as that is what the original patch did was add the extra key) .. i'll add that to the patch

[magneto]

add the test update too

[Brian Rosner]

Ok, I am a bit confused here. The conditional check in add_fields is wrong. The only time the field needs to be marked as hidden is if it were created by Django period. Any other cases need to be given UI. I also don't understand why unique is playing a role here. It seems the real issue is the data is not being mapped correctly by model_to_dict and that fix in the patch is technically wrong too. There should be no need to map the data twice to work around problem. Sure looks like symptom patching there. In #8241 semenov brings up a point about the use of attname and he is correct. We shouldn't be stomping on that namespace like that. With a fix to that it sounds like this ticket would be correctly fixed. Please explain a bit more why the stuff in add_fields is needed. Thanks.

[Karen Tracey]

I find this ticket confusing also, partly because I do not really know the code involved but partly because I think there are two completely unrelated problems described/attempting to be fixed by the patch.

First problem: the widget on the change page for an object containing a OneToOne field does not display the current value. It displays as empty. This problem was introduced by the fix to #7888, if I back up to [8527] the widget displays the correct value. This is what the patch to model_to_dict appears to be attempting to fix. It's putting the data value in the dict under both f.attname and f.name since the former is apparently what is needed to fix #7888 but that latter is what is needed for the change page widget to get the value for display. I agree it doesn't seem right to be mapping the data twice but have no idea what the right answer is since I don't understand this code.

Second problem, that existed before [8528] and still exists: If you have a OneToOne-related object edited inline that shares a primary key with the parent object, attempting to save the parent object generates a KeyError exception. The problem here is the inline form for the child object does not contain that object's primary key anywhere, and save_existing_objects assumes a form always contains the pk value for the object. The primary key field is the same field as the link to the parent object, so it is not present in the inline forms? At least that is my guess. I think that is what the changes to add_fields are trying to fix. Again since I don't know this code I don't know what the correct fix should be.

The 2nd problem is also what was reported in #8241, but I don't see that it has been fixed, so I'm not sure that should have been closed. I believe it still exists. Might be more straightforward to use this one to fix the first problem (widget on change page getting correct value) here, and reopen #8241 to fix the key error on save of an object with an inline that shares the same primary key as the parent. I think it is very confusing to have them both lumped under this one ticket.

[Brian Rosner]

Thanks Karen. As I pointed out in #8241 the patch that semenov provided should be the fix to the first problem. It is being attached to the wrong ticket. I still need to investigate it some more before committing. Ideally that gets its own ticket. The second problem based on what was reported in #8241 (the real problem there) I was not able to reproduce. I may have done it wrong. If there is a more specific use case that I missed that too should be a new ticket I think that way we don't try to confuse ourselves even more. So it appears we understand what this ticket does and that it needs to be broken out to keep sanity. Perhaps the new tickets should be opened and this one closed in favor of those?

[Karen Tracey]

Replying to brosner:

Perhaps the new tickets should be opened and this one closed in favor of those?

If you like, I can do that with simple models and steps to recreate each problem.

[Brian Rosner]

Ok. #8241 is still a problem. I reopened it. It appears that between that ticket and #8694 this ticket is covered right?

[Karen Tracey]

Replying to brosner:

Ok. #8241 is still a problem. I reopened it. It appears that between that ticket and #8694 this ticket is covered right?

Well I already opened the other two trying to clarify one simple problem for each. I think we now have two tickets per problem. Up to you which ones you want to keep!

[Brian Rosner]

Yikes. This one goes. Closing in favor of the other two.

[Jacob]

Milestone 1.0 deleted