[Django] #30907: SplitArrayField.has_changed() returns True for unchanged fields
Django
--------------------------------------------+------------------------
Reporter: paveldedik | Owner: (none)
Type: Uncategorized | Status: new
Component: contrib.postgres | Version: 2.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
This ticket addresses similar issue as in
https://code.djangoproject.com/ticket/28950 only for the `SplitArrayField`
I have the following models and forms:
{{{
class Service(models.Model):
name = pg_fields.CICharField(max_length=100)
class Environment(models.Model):
service = models.ForeignKey(Service, on_delete=models.CASCADE)
name = models.CharField(max_length=100)
urls = pg_fields.ArrayField(
models.URLField(max_length=500), null=True, default=list
)
class EnvironmentForm(forms.ModelForm):
urls = SplitArrayField(
forms.URLField(required=False),
remove_trailing_nulls=True,
required=False,
size=2,
)
class Meta:
model = models.Environment
fields = ["name", "urls"]
ServiceEnvironmentsFormSet = forms.inlineformset_factory(
models.Service,
models.Environment,
form=EnvironmentForm,
extra=2,
can_delete=True,
)
class ServiceForm(forms.ModelForm):
class Meta:
model = models.Service
fields = ["name"]
}}}
Note the `ServiceEnvironmentsFormSet` which I'm rendering in a template
including the `SplitArrayField` named `urls`. When I fill only the name of
the `Service` in the rendered `ServiceForm` without entering any data to
rendered `Environment` forms, the `ServiceEnvironmentsFormSet` validation
fails. That's because in `EnvironmentForm`, the `name` is required, but
that's not the problem - this should be ok when using `InlineFormSet` as
each form in the FormSet is optional. So why does it fail? Because Django
starts validating forms in `InlineFormSet` if the `form.has_changed()`
method returns `True`. And in this case, it always returns `True` because
`SplitArrayField.has_changed()` returns `True` in the following cases:
{{{
SplitArrayField.has_changed(None, ["", ""]) # returns True even though it
shouldn't as ["", ""] means no data in the form has been entered
SplitArrayField.has_changed([], ["", ""]) # returns True - should be
False
SplitArrayField.has_changed(["a"], ["a", ""]) # returns True - should be
False
}}}
Note that I'm using `remove_trailing_nulls=True` in the `SplitArrayField`.
So the above cases should all evaluate to `False` I believe.
I already opened PR for this here:
https://github.com/django/django/pull/11970
BTW I'm not sure how `has_changed()` should be evaluated when passing both
`remove_trailing_nulls=False` and `required=False` to the `base_field`, as
in that case all required fields in the FormSet would have to be filled by
the user since `has_changed()` would always return `True` even for empty
input values.
--
Ticket URL: <https://code.djangoproject.com/ticket/30907>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: paveldedik | Owner: (none)
Type: Bug | Status: new
Component: contrib.postgres | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by paveldedik):
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:1>
Django
remove_trailing_nulls.
----------------------------------+--------------------------------------
Reporter: paveldedik | Owner: paveldedik
Type: Bug | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => assigned
* owner: (none) => paveldedik
* version: 2.2 => master
* stage: Unreviewed => Accepted
Comment:
I agree we should fix this behavior when `remove_trailing_nulls=True`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:2>
Django
remove_trailing_nulls.
----------------------------------+--------------------------------------
Reporter: paveldedik | Owner: paveldedik
Type: Bug | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:3>
Django
remove_trailing_nulls.
-------------------------------------+-------------------------------------
Reporter: Pavel Dedik | Owner: Pavel
| Dedik
Type: Bug | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:4>
Django
remove_trailing_nulls.
-------------------------------------+-------------------------------------
Reporter: Pavel Dedik | Owner: Pavel
| Dedik
Type: Bug | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d95b1ddcbef6ef61229080bab1c166d6ee4a161b" d95b1ddc]:
{{{
#!CommitTicketReference repository=""
revision="d95b1ddcbef6ef61229080bab1c166d6ee4a161b"
Refs #30907 -- Added more tests for SplitArrayField.has_changed().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:5>
Django
remove_trailing_nulls.
-------------------------------------+-------------------------------------
Reporter: Pavel Dedik | Owner: Pavel
| Dedik
Type: Bug | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"bcfbb71c63092f4bc0fa5cc3f82ece2c986e3168" bcfbb71c]:
{{{
#!CommitTicketReference repository=""
revision="bcfbb71c63092f4bc0fa5cc3f82ece2c986e3168"
Refs #30907 -- Added SplitArrayField._remove_trailing_nulls() hook.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:6>
Django
remove_trailing_nulls.
-------------------------------------+-------------------------------------
Reporter: Pavel Dedik | Owner: Pavel
| Dedik
Component: contrib.postgres | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"711a7d4d50c1ef194a4a71e68ce7fa38fcb5103f" 711a7d4]:
{{{
#!CommitTicketReference repository=""
revision="711a7d4d50c1ef194a4a71e68ce7fa38fcb5103f"
Fixed #30907 -- Fixed SplitArrayField.has_changed() with removal of empty
trailing values.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30907#comment:7>