Fixed #32210 -- Default value of UUIDField in Inline Formset

[sainipray]

ticket-32210

[carltongibson]

Maybe you can add a unit test that reproduces the bug stated in the ticket.

@sainipray I think we need a fresh test more clearly showing the issue, rather than just an edit on the existing test that failed, don't you? 🤔

[sainipray]

Maybe you can add a unit test that reproduces the bug stated in the ticket.

@sainipray I think we need a fresh test more clearly showing the issue, rather than just an edit on the existing test that failed, don't you? 🤔

Yes, I'll try to add a test case. but I checked the ticket https://code.djangoproject.com/ticket/24958. and commit for this https://github.com/django/django/commit/a50b66da30320887c23c73927f6b2ab41e0301bf so that commit added some code there but caused this issue (https://code.djangoproject.com/ticket/32210), I think that case I edit should not be there according to changes.

[carltongibson]

@sainipray If you think it should be removed, or adjusted as you have, please do so. (We can decide at the end)

Ideally, an extra case that would have failed preventing the regression would be awesome.

(You're looking at this, so you're the current expert 🙂)

[carltongibson]

Thanks for the quick update @sainipray — I will give it another look tomorrow. 👍

[sainipray]

Hi @felixxm, Updated all above points

[carltongibson]

Hey @timgraham -- I don't know if you've got bandwidth/interest right now, but this one has a good reproduce, is non-obvious (at least to me 🙂) and you were involved in earlier iterations, so I thought I'd mention it to you at least.

(No worries if you're otherwise engaged. Thanks!)

[sainipray]

Hi @sainipray.

Sorry for the slow follow-up here: I'm still deeply suspicious about the change here — I think we need further test cases to show that it's correct.

This was the case before a50b66d for ticket-24958:

if self.instance._state.adding and form._meta.model._meta.pk.has_default():
    self.instance.pk = None

With the change here, we would have:

if self.instance._state.adding:
    to_field = self.instance._meta.pk
    if to_field.has_default():
        setattr(self.instance, to_field.attname, None) 

Bar the reformatting it's exactly the same logic. As such, the effect is to revert a50b66d — but just inverting the assertion in the test case doesn't show we didn't introduce a regression. 🤔 (If we're saying a50b66d was wrong then some account of that would help see the conclusion.)

Combined with the semantics of ignoring the to_field I'm still not convinced this is correct.

Taking the example project from the ticket, can we examine the value of form.instance.id (the to_field in that case) coming out of BaseInlineFormSet._construct_form() — is that correct.

I'm still looking, but I thought I'd comment in the meantime in case you can make progress with additional tests/more diagnosis.

Thanks!

Hi @carltongibson, I'm ignoring to_field because it can store the parent model field name that have relation to the child model. Suppose we have two models like the bottom:

class ParentWithUUIDAlternateKey(models.Model):
    uuid = models.UUIDField(unique=True, default=uuid.uuid4, editable=False)
    name = models.CharField(max_length=50)


class ChildRelatedViaAK(models.Model):
    name = models.CharField(max_length=255)
    parent = models.ForeignKey(
        ParentWithUUIDAlternateKey, models.CASCADE, to_field="uuid"
    )

Here uuid field have a relation with the child model field parent field as ForeignKey field and this uuid field is not primary_key which means we have id field as primary_key in the Parent model.

so when we save ParentWithUUIDAlternateKey data from Django admin with inlineformset_factory of model ChildRelatedViaAK.

kwargs['to_field'] = self.fk.remote_field.field_name Here to_field value is uuid field

if kwargs.get('to_field') is not None:
    to_field = self.instance._meta.get_field(kwargs['to_field'])
else:
    to_field = self.instance._meta.pk

So In if in condition, to_field value become is uuid field

if to_field.has_default():
      setattr(self.instance, to_field.attname, None)

and now this code reset uuid field value to None so when this uuid field actually saving in the database, Database returns IntegrityError.

We only regenerate the value of the primary_key field here, so we should only reset value of primay_key. that's why I consider pk field and skip to use 'to_field' from kwargs.

You mentioned this code

if self.instance._state.adding and form._meta.model._meta.pk.has_default():
    self.instance.pk = None

this is the same as I did. We can use this as well.

Let me know if you have still any doubts.

[sainipray]

Hi @carltongibson Any update about this PR?

[carltongibson]

@sainipray I still haven't seen why it is that we're simply reverting https://github.com/django/django/commit/a50b66da30320887c23c73927f6b2ab41e0301bf — I flagged Needs tests on the ticket because I think we need to show that all cases are correct.

Returning to ticket-24958, can we show that there's not a regression there with this change? (It's not enough to just invert the assertion, and then say look it passes — we need to see that's correct, and I'm perhaps slow, but it's not obvious to me that it is.)

I haven't had a chance to read through your last comment to see if that makes it clear.

[timgraham]

I tested in the admin using this setup (models from the test suite):

from django.contrib import admin

from .models import ParentWithUUIDAlternateKey, ChildRelatedViaAK

class Inline(admin.StackedInline):
    model = ChildRelatedViaAK

admin.site.register(ParentWithUUIDAlternateKey, inlines=[Inline])

Without this patch, I can reproduce the integrity error when trying to add a new instance with inlines. When trying the same with this patch, I get "Please correct the errors below." with no other details in the UI. Printing the formset errors in ModelAdmin._changeform_view() shows: ["[{'parent': ['The inline value did not match the parent instance.']}, {'parent': ['The inline value did not match the parent instance.']}, {}]"]. It seems further investigation is needed.