Opened 4 weeks ago

Last modified 3 weeks ago

#35449 assigned Bug

SplitArrayField doesn't validate properly with remove_trailing_nulls=True

Reported by: Matthijs Owned by: Jae Hyuck Sa
Component: contrib.postgres Version: 4.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (4)

comment:1 by Sarah Boyce, 4 weeks 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 , 4 weeks ago

Owner: set to Jae Hyuck Sa
Status: newassigned

comment:3 by Jae Hyuck Sa , 4 weeks ago

Has patch: set

comment:4 by Sarah Boyce, 3 weeks ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top