Opened 6 months ago

Closed 7 weeks ago

#35449 closed Bug (fixed)

SplitArrayField doesn't validate properly with remove_trailing_nulls=True

Reported by: Matthijs Owned by: Calvin Vu
Component: contrib.postgres Version: 4.2
Severity: Normal Keywords:
Cc: Calvin Vu Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I created an ArrayField(models.CharField(), size=3) on one of my models and created a form for it using the SplitArrayField. My requirements where that the array field was optional to fill in and that you could also just fill in one of the CharFields and leave the others blank. So according to the docs I was to set the CharField in my SplitArrayField to required=True and remove_trailing_nulls=True on the SplitArrayField.

SplitArrayField(
    forms.CharField(required=True, max_length=148),
    size=3,
    widget=SplitArrayWidget(forms.TextInput(attrs={'size': '148'}), 3),
    remove_trailing_nulls=True,
    required=False
)

To my surprise the form did not validate properly and I was able to submit inputs that exceeded the max_length specified in my form.
Looking at the def clean method of the SplitArrayField I suspect a problem there.

try:
    cleaned_data.append(self.base_field.clean(item))
except ValidationError as error:
    errors.append(
        prefix_validation_error(
            error,
            self.error_messages["item_invalid"],
            code="item_invalid",
            params={"nth": index + 1},
        )
    )
    cleaned_data.append(None)
    ...

If there is any ValidationError the error is added to errors, at the same time None is appended to the cleaned_data.
This causes issues with the self._remove_trailing_nulls later on.

cleaned_data, null_index = self._remove_trailing_nulls(cleaned_data)
if null_index is not None:
    errors = errors[:null_index]
errors = list(filter(None, errors))

cleaned_data is passed to self._remove_trailing_nulls and because remove_trailing_nulls=True, null_index will be 0 thus errors is reassigned to an empty list.

I wrote a test case showcasing this issue:

from django import forms
from django.contrib.postgres.forms import SplitArrayField

class TestForm1(forms.Form):
    field = SplitArrayField(
        forms.CharField(max_length=5, required=True),
        size=3,
        remove_trailing_nulls=True,
        required=False,
    )

class TestForm2(forms.Form):
    field = forms.CharField(max_length=5, required=True)

class SplitArrayFieldTest(TestCase):
    def test_validation_error(self):
        invalid_value = 'invalid'
        form1 = TestForm1(data={'field': [invalid_value, '', '']})
        form2 = TestForm2(data={'field': invalid_value})

        self.assertFalse(form1.is_valid())  #is_valid() should return False, but returns True
        self.assertFalse(form2.is_valid())

Change History (13)

comment:1 by Sarah Boyce, 6 months ago

Summary: SplitArrayField doesn't validate properly (django.contrib.postgres.forms)SplitArrayField doesn't validate properly with remove_trailing_nulls=True
Triage Stage: UnreviewedAccepted

Thank you! Was able to replicate on main
Here is a test if someone needs.

  • tests/postgres_tests/test_array.py

    diff --git a/tests/postgres_tests/test_array.py b/tests/postgres_tests/test_array.py
    index 386a0afa3a..d45fc1d153 100644
    a b class TestSplitFormField(PostgreSQLSimpleTestCase):  
    13391339            ],
    13401340        )
    13411341
     1342    def test_invalid_char_length_with_remove_trailing_nulls(self):
     1343        field = SplitArrayField(
     1344            forms.CharField(max_length=2),
     1345            size=3,
     1346            remove_trailing_nulls=True,
     1347        )
     1348        with self.assertRaises(exceptions.ValidationError) as cm:
     1349            field.clean(["abc", "", ""])
     1350        self.assertEqual(
     1351            cm.exception.messages,
     1352            [
     1353                "Item 1 in the array did not validate: Ensure this value has at most 2 "
     1354                "characters (it has 3).",
     1355            ],
     1356        )
     1357
    13421358    def test_splitarraywidget_value_omitted_from_data(self):

comment:2 by Jae Hyuck Sa , 6 months ago

Owner: set to Jae Hyuck Sa
Status: newassigned

comment:3 by Jae Hyuck Sa , 6 months ago

Has patch: set

comment:4 by Sarah Boyce, 6 months ago

Patch needs improvement: set

comment:5 by Jae Hyuck Sa , 2 months ago

Owner: Jae Hyuck Sa removed
Status: assignednew

comment:6 by Calvin Vu, 2 months ago

Owner: set to Calvin Vu
Status: newassigned

comment:7 by Calvin Vu, 2 months ago

When I include Sarah's test in test_array.py and run the tests with

python runtests.py postgres_tests.test_array

The test passes, does this mean that the bug has been resolved?

Last edited 2 months ago by Calvin Vu (previous) (diff)

comment:8 by Calvin Vu, 2 months ago

Cc: Calvin Vu added

in reply to:  7 comment:9 by Calvin Vu, 2 months ago

Replying to Calvin Vu:

When I include Sarah's test in test_array.py and run the tests with

python runtests.py postgres_tests.test_array

The test passes, does this mean that the bug has been resolved?

Nevermind, ignore this

comment:10 by Jacob Walls, 2 months ago

Patch needs improvement: unset

comment:11 by Sarah Boyce, 8 weeks ago

Patch needs improvement: set

comment:12 by Sarah Boyce, 7 weeks ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Sarah Boyce <42296566+sarahboyce@…>, 7 weeks ago

Resolution: fixed
Status: assignedclosed

In a417c0e:

Fixed #35449 -- Fixed validation of array items in SplitArrayField when remove_trailing_nulls=True.

Note: See TracTickets for help on using tickets.
Back to Top