Opened 4 months ago
Last modified 2 months ago
#35676 new Bug
ExclusionConstraint that includes a ForeignKey fails validation in ModelForm
Reported by: | Take Weiland | Owned by: | |
---|---|---|---|
Component: | Forms | Version: | 5.1 |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette, Lufafa Joshua | 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 (11)
comment:1 by , 4 months ago
Component: | Uncategorized → Forms |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 4 months 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 , 4 months ago
Cc: | 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 , 4 months 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 , 4 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 4 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 2 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:9 by , 2 months ago
Cc: | added |
---|
comment:10 by , 2 months ago
Cc: | added |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:11 by , 2 months ago
Cc: | removed |
---|---|
Owner: | removed |
Status: | assigned → new |
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
if needed.