Opened 5 weeks ago

Last modified 36 hours ago

#35676 assigned Bug

ExclusionConstraint that includes a ForeignKey fails validation in ModelForm

Reported by: Take Weiland Owned by: Lufafa Joshua
Component: Forms Version: 5.1
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm not sure if the summary above is good, but let me demonstrate:

class BlockedDaysConfig(models.Model):
    name = models.CharField(max_length=255)


class BlockedDaysRange(models.Model):
    config = models.ForeignKey(BlockedDaysConfig, on_delete=models.CASCADE, related_name='days')
    date_range = DateRangeField()

    class Meta:
        constraints = (
            ExclusionConstraint(
                name='%(app_label)s_%(class)s_no_overlapping_ranges_per_config',
                expressions=(
                    ('config_id', RangeOperators.EQUAL, ),
                    ('date_range', RangeOperators.OVERLAPS, ),
                )
            ),
        )

BlockedDaysRange represents a range of blocked days. BlockedDaysConfig represents a range of these blocked days. Example:
A BlockedDaysConfig with name "School Holidays in Germany" contains several BlockedDaysConfig entries, one for "summer holidays", one for "winter holidays", etc.
The exclusion constraint validates that the date ranges within one config do not overlap.

Now I have configured a ModelAdmin for this:

class BlockedDaysRangeInline(admin.TabularInline):
    model = BlockedDaysRange


@admin.register(BlockedDaysConfig)
class BlockedDaysConfigAdmin(admin.ModelAdmin):
    inlines = (BlockedDaysRangeInline, )

The problem now happens when saving, because BaseModelForm._post_clean will exclude the InlineForeignKey for config when calling full_clean (see https://github.com/django/django/blob/b99c608ea10cabc97a6b251cdb6e81ef2a83bdcf/django/forms/models.py#L477-L488).
Because config is in exclude, when ExclusionConstraint eventually calls _get_field_value_map, the config is not included and thus a nonsensical SQL query happens (WHERE config_id=config_id and date_range && '[2024-01-01,2024-01-03)'::daterange). This will now validate against any date ranges, not just ones with the same config_id.
As you can see from the comment in the code snippet linked above, this problem is already recognized for unique constraints, which are handled specially by ModelForm.

Change History (7)

comment:1 by Simon Charette, 5 weeks ago

Component: UncategorizedForms
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Thanks for the detailed report. The comment you pointed at makes it clear that this is a variant of #12960 but for constraints instead of uniques.

I think something along the following lines should address the issue but I'm curious to hear your thoughts on it.

  • django/forms/models.py

    diff --git a/django/forms/models.py b/django/forms/models.py
    index 8084e16c8d..bbb4d94e33 100644
    a b def __init__(  
    370370        # if initial was provided, it should override the values from instance
    371371        if initial is not None:
    372372            object_data.update(initial)
    373         # self._validate_unique will be set to True by BaseModelForm.clean().
    374         # It is False by default so overriding self.clean() and failing to call
    375         # super will stop validate_unique from being called.
     373        # self._validate_(unique|constraints) will be set to True by
     374        # BaseModelForm.clean(). It is False by default so overriding
     375        # self.clean() and failing to call super will stop
     376        # validate_(unique|constraints) from being called.
    376377        self._validate_unique = False
     378        self._validate_constraints = False
    377379        super().__init__(
    378380            data,
    379381            files,
    def _get_validation_exclusions(self):  
    436438
    437439    def clean(self):
    438440        self._validate_unique = True
     441        self._validate_constraints = True
    439442        return self.cleaned_data
    440443
    441444    def _update_errors(self, errors):
    def _post_clean(self):  
    495498            self._update_errors(e)
    496499
    497500        try:
    498             self.instance.full_clean(exclude=exclude, validate_unique=False)
     501            self.instance.full_clean(
     502                exclude=exclude, validate_unique=False, validate_constraints=False
     503            )
    499504        except ValidationError as e:
    500505            self._update_errors(e)
    501506
    502         # Validate uniqueness if needed.
     507        # Validate uniqueness and constraints if needed.
    503508        if self._validate_unique:
    504509            self.validate_unique()
     510        if self._validate_constraints:
     511            self.validate_constraints()
    505512
    506513    def validate_unique(self):
    507514        """
    def validate_unique(self):  
    514521        except ValidationError as e:
    515522            self._update_errors(e)
    516523
     524    def validate_constraints(self):
     525        """
     526        Call the instance's validate_constraints() method and update the form's
     527        validation errors if any were raised.
     528        """
     529        exclude = self._get_validation_exclusions()
     530        try:
     531            self.instance.validate_constraints(exclude=exclude)
     532        except ValidationError as e:
     533            self._update_errors(e)
     534
    517535    def _save_m2m(self):
    518536        """
    519537        Save the many-to-many fields and generic relations for this form.

comment:2 by Take Weiland, 5 weeks ago

Yes, that definitely does seem sensible.
The only potential issue might be that when creating new objects then the foreign key field will still be None, because the related object isn't saved to the DB yet.
In my crude hack to work around this (override Model.validate_constraints and remove the field from exclude) this works fine, however I don't know if there are situations where it doesn't. In my case (shown above) it will result in a qs.filter(config=None) to happen. As I understand it, this doesn't do anything, but lookups other than exact might not like a None there.

comment:3 by Simon Charette, 5 weeks ago

Cc: Simon Charette added

The only potential issue might be that when creating new objects then the foreign key field will still be None, because the related object isn't saved to the DB yet.

That's another can of worm and an issue that is unfortunately pretty hard to solve. The way this is emulated in today for unique constraints validation in admin inlines can be found in ModelFormSet.validate_unique source.

The gist of it is that for each fields marked to be unique within the formset all associated values are combined and the name of the field is used to make duplicate values collide. This is poorly implemented IMO as it doesn't account for database level uniqueness (e.g. if a field has a specific db_collation that is case insensitive then "foo" == "Foo" and only a transit through the database can determine that)

If you wanted to manually implement a similar emulation for exclusion constraints you could define a BaseInlineFormSet subclass and override its clean method to iterate through all forms and ensure that each form contains non-overlapping date_range in Python. This subclass could then be assigned to BlockedDaysRangeInline.formset.

What I'm curious to hear about is whether the patch above prevents the problematic SQL query from being executed in the first place.

comment:4 by Take Weiland, 5 weeks ago

I've manually applied your patch to a custom ModelForm subclass and it does solve the problematic SQL query.
When altering existing objects, the query happens correctly (with the correct config ID substituted). When creating a new object, it will do a WHERE config_id = NULL AND date_range && (...). That doesn't cause any problems though, because the = NULL comparison will just always yield NULL and thus no rows will be returned.

comment:5 by Calvin Vu, 2 weeks ago

Owner: set to Calvin Vu
Status: newassigned

comment:6 by Calvin Vu, 2 weeks ago

Owner: Calvin Vu removed
Status: assignednew

comment:7 by Lufafa Joshua, 36 hours ago

Owner: set to Lufafa Joshua
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top