From 64bc1d4bfab439a10a45dd068bf30bf7f8f59bb9 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 15 Mar 2024 17:01:03 +0100 Subject: [PATCH 1/3] :bug: [#4016] Fix invalid default validate defaults Formio uses empty strings for length-indication defaults in the validation configuration (minLength, maxLength, min, max, minWords, maxWords) which leaks into textfield, number and derived components like email, phone number... This is wrong for a number of reasons: 1. Semantically you'd expect undefined or null, not an empty string 2. Comparing string with a number to perform validation crashes the backend 3. Their own typescript types are defined as 'maxLength?: number', stating that it's either undefined or a number. Using 'null' will cause us typescript issues, so the patch targets these invalid defaults and deletes the keys alltogether, which results in: 1. it being 'undefined' in the frontend code, which should pass the falsy checks 2. the missing key in the backend will default to 'None', which is explicitly checked. This only applies to new components, existing data needs a data migration to correct this. --- src/openforms/js/components/form/cosign.js | 3 ++ src/openforms/js/components/form/currency.js | 7 +++++ src/openforms/js/components/form/email.js | 7 +++++ src/openforms/js/components/form/iban.js | 8 +++++ .../js/components/form/licenseplate.js | 7 +++++ src/openforms/js/components/form/number.js | 19 ++++++++++++ .../js/components/form/phoneNumber.js | 7 +++++ src/openforms/js/components/form/textarea.js | 7 +++++ src/openforms/js/components/form/textfield.js | 30 +++++++++++++++++++ 9 files changed, 95 insertions(+) diff --git a/src/openforms/js/components/form/cosign.js b/src/openforms/js/components/form/cosign.js index 07b88c50b5..52291deb96 100644 --- a/src/openforms/js/components/form/cosign.js +++ b/src/openforms/js/components/form/cosign.js @@ -4,6 +4,7 @@ import {AUTH_PLUGINS_ENDPOINT} from 'components/admin/form_design/constants.js'; import {get} from 'utils/fetch'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './textfield'; export const getAvailableAuthPlugins = async () => { const response = await get(AUTH_PLUGINS_ENDPOINT); @@ -16,6 +17,8 @@ class CoSignField extends FormioEmail { constructor(...args) { super(...args); + patchValidateDefaults(this); + // somewhere the default emptyValue/defaultValue does not seem to be used and it forces // component.defaultValue to be null, which crashes the builder. if (this.component.defaultValue === null) { diff --git a/src/openforms/js/components/form/currency.js b/src/openforms/js/components/form/currency.js index 9754577502..ae5386a9c0 100644 --- a/src/openforms/js/components/form/currency.js +++ b/src/openforms/js/components/form/currency.js @@ -3,6 +3,7 @@ import CurrencyEditData from 'formiojs/components/currency/editForm/Currency.edi import _ from 'lodash'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './number'; const FormioCurrency = Formio.Components.components.currency; @@ -20,6 +21,12 @@ class CurrencyField extends FormioCurrency { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultValue() { let defaultValue = super.defaultValue; diff --git a/src/openforms/js/components/form/email.js b/src/openforms/js/components/form/email.js index 91d0da6c27..265ebdb487 100644 --- a/src/openforms/js/components/form/email.js +++ b/src/openforms/js/components/form/email.js @@ -1,6 +1,7 @@ import {Formio} from 'formiojs'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './textfield'; const FormioEmail = Formio.Components.components.email; @@ -25,6 +26,12 @@ class EmailField extends FormioEmail { schema: EmailField.schema(), }; } + + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } } export default EmailField; diff --git a/src/openforms/js/components/form/iban.js b/src/openforms/js/components/form/iban.js index fd75d8dc66..499c668ff7 100644 --- a/src/openforms/js/components/form/iban.js +++ b/src/openforms/js/components/form/iban.js @@ -1,5 +1,7 @@ import {Formio} from 'formiojs'; +import {patchValidateDefaults} from './textfield'; + const TextField = Formio.Components.components.textfield; class IbanField extends TextField { @@ -25,6 +27,12 @@ class IbanField extends TextField { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultSchema() { return IbanField.schema(); } diff --git a/src/openforms/js/components/form/licenseplate.js b/src/openforms/js/components/form/licenseplate.js index 0e2812ac6f..3f77c89e2c 100644 --- a/src/openforms/js/components/form/licenseplate.js +++ b/src/openforms/js/components/form/licenseplate.js @@ -1,6 +1,7 @@ import {Formio} from 'formiojs'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './textfield'; const TextField = Formio.Components.components.textfield; @@ -34,6 +35,12 @@ class LicensePlate extends TextField { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultSchema() { return LicensePlate.schema(); } diff --git a/src/openforms/js/components/form/number.js b/src/openforms/js/components/form/number.js index e3bbcfe742..13a70b9611 100644 --- a/src/openforms/js/components/form/number.js +++ b/src/openforms/js/components/form/number.js @@ -4,6 +4,19 @@ import {localiseSchema} from './i18n'; const FormioNumber = Formio.Components.components.number; +export const patchValidateDefaults = instance => { + // Similar fix to the one in the textfield component. For some reason, Formio defaults + // to empty strings for numeric values instead of using null or undefined :/ + const validate = instance.component?.validate; + + if (validate?.min === '') { + delete validate.min; + } + if (validate?.max === '') { + delete validate.max; + } +}; + class NumberField extends FormioNumber { static schema(...extend) { return localiseSchema(FormioNumber.schema(...extend)); @@ -20,6 +33,12 @@ class NumberField extends FormioNumber { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultValue() { let defaultValue = super.defaultValue; diff --git a/src/openforms/js/components/form/phoneNumber.js b/src/openforms/js/components/form/phoneNumber.js index e414adad4f..64de132058 100644 --- a/src/openforms/js/components/form/phoneNumber.js +++ b/src/openforms/js/components/form/phoneNumber.js @@ -1,6 +1,7 @@ import {Formio} from 'formiojs'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './textfield'; const PhoneNumber = Formio.Components.components.phoneNumber; @@ -25,6 +26,12 @@ class PhoneNumberField extends PhoneNumber { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultSchema() { return PhoneNumberField.schema(); } diff --git a/src/openforms/js/components/form/textarea.js b/src/openforms/js/components/form/textarea.js index 07b04c9cd9..e1ed20eaf2 100644 --- a/src/openforms/js/components/form/textarea.js +++ b/src/openforms/js/components/form/textarea.js @@ -3,6 +3,7 @@ import NativePromise from 'native-promise-only'; import {Formio} from 'react-formio'; import {localiseSchema} from './i18n'; +import {patchValidateDefaults} from './textfield'; const FormioTextarea = Formio.Components.components.textarea; @@ -18,6 +19,12 @@ class TextArea extends FormioTextarea { }; } + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } + get defaultSchema() { return TextArea.schema(); } diff --git a/src/openforms/js/components/form/textfield.js b/src/openforms/js/components/form/textfield.js index f15210f5ef..61035031ee 100644 --- a/src/openforms/js/components/form/textfield.js +++ b/src/openforms/js/components/form/textfield.js @@ -4,6 +4,30 @@ import {localiseSchema} from './i18n'; const FormioTextField = Formio.Components.components.textfield; +export const patchValidateDefaults = instance => { + // Formio.js itself doesn't respect their own typescript declarations... + // https://github.com/formio/formio.js/blob/master/src/components/textfield/TextField.js#L21 + // So we patch up these badly typed default values, letting the default behaviour of + // our own formio-builder kick in. + // Fixing this in static schema doesn't seem to apply it to component instances (?), + // so we need to patch the weird typing information here. + const validate = instance.component?.validate; + + if (validate?.minLength === '') { + delete validate.minLength; + } + if (validate?.maxLength === '') { + delete validate.maxLength; + } + + if (validate?.minWords === '') { + delete validate.minWords; + } + if (validate?.maxWords === '') { + delete validate.maxWords; + } +}; + class TextField extends FormioTextField { static schema(...extend) { return localiseSchema(FormioTextField.schema(...extend)); @@ -15,6 +39,12 @@ class TextField extends FormioTextField { schema: TextField.schema(), }; } + + constructor(...args) { + super(...args); + + patchValidateDefaults(this); + } } export default TextField; From 7c5fcece5f8ae9dfbf167d3cacb2948ca7142533 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 15 Mar 2024 17:13:28 +0100 Subject: [PATCH 2/3] :white_check_mark: [#4016] Add test for migration fix --- ...0096_fix_invalid_validate_configuration.py | 12 ++ src/openforms/forms/tests/test_migrations.py | 180 ++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py diff --git a/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py b/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py new file mode 100644 index 0000000000..20e9ad9c57 --- /dev/null +++ b/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py @@ -0,0 +1,12 @@ +# Generated by Django 4.2.10 on 2024-03-15 16:02 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("forms", "0095_merge_20240313_1742"), + ] + + operations = [] diff --git a/src/openforms/forms/tests/test_migrations.py b/src/openforms/forms/tests/test_migrations.py index 72d42f149b..c0982bba8f 100644 --- a/src/openforms/forms/tests/test_migrations.py +++ b/src/openforms/forms/tests/test_migrations.py @@ -107,3 +107,183 @@ def test_builder_enabled(self): self.assertIsNotNone(variables[0].service_fetch_configuration) self.assertIsNone(variables[1].service_fetch_configuration) + + +class FixValidateConfigurationMigrationTests(TestMigrations): + app = "forms" + migrate_from = "0095_merge_20240313_1742" + migrate_to = "0096_fix_invalid_validate_configuration" + + def setUpBeforeMigration(self, apps): + FormDefinition = apps.get_model("forms", "FormDefinition") + + bad_config = [ + { + "type": "textfield", + "key": "textfield", + "key": "textfield", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "email", + "key": "email", + "key": "email", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "phoneNumber", + "key": "phoneNumber", + "key": "phoneNumber", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "postcode", + "key": "postcode", + "key": "postcode", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "textarea", + "key": "textarea", + "key": "textarea", + "validate": { + "minLength": "", + "maxLength": "", + "minWords": "", + "maxWords": "", + }, + }, + { + "type": "number", + "key": "number", + "key": "number", + "validate": { + "min": "", + "max": "", + }, + }, + { + "type": "currency", + "key": "currency", + "key": "currency", + "validate": { + "min": "", + "max": "", + }, + }, + { + "type": "iban", + "key": "iban", + "key": "iban", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "licenseplate", + "key": "licenseplate", + "key": "licenseplate", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "bsn", + "key": "bsn", + "key": "bsn", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + { + "type": "cosign", + "key": "cosign", + "key": "cosign", + "validate": { + "minLength": "", + "maxLength": "", + }, + }, + ] + FormDefinition.objects.create( + name="broken", configuration={"components": bad_config} + ) + + def test_migration_fixes_issues(self): + FormDefinition = self.apps.get_model("forms", "FormDefinition") + fixed_config = FormDefinition.objects.get().configuration["components"] + + ( + textfield, + email, + phone_number, + postcode, + textarea, + number, + currency, + iban, + licenseplate, + bsn, + cosign, + ) = fixed_config + + with self.subTest("textfield"): + self.assertNotIn("minLength", textfield["validate"]) + self.assertNotIn("maxLength", textfield["validate"]) + + with self.subTest("email"): + self.assertNotIn("minLength", email["validate"]) + self.assertNotIn("maxLength", email["validate"]) + + with self.subTest("phone_number"): + self.assertNotIn("minLength", phone_number["validate"]) + self.assertNotIn("maxLength", phone_number["validate"]) + + with self.subTest("postcode"): + self.assertNotIn("minLength", postcode["validate"]) + self.assertNotIn("maxLength", postcode["validate"]) + + with self.subTest("textarea"): + self.assertNotIn("minLength", textarea["validate"]) + self.assertNotIn("maxLength", textarea["validate"]) + self.assertNotIn("minWords", textarea["validate"]) + self.assertNotIn("maxWords", textarea["validate"]) + + with self.subTest("number"): + self.assertNotIn("min", number["validate"]) + self.assertNotIn("max", number["validate"]) + + with self.subTest("currency"): + self.assertNotIn("min", currency["validate"]) + self.assertNotIn("max", currency["validate"]) + + with self.subTest("iban"): + self.assertNotIn("minLength", iban["validate"]) + self.assertNotIn("maxLength", iban["validate"]) + + with self.subTest("licenseplate"): + self.assertNotIn("minLength", licenseplate["validate"]) + self.assertNotIn("maxLength", licenseplate["validate"]) + + with self.subTest("bsn"): + self.assertNotIn("minLength", bsn["validate"]) + self.assertNotIn("maxLength", bsn["validate"]) + + with self.subTest("cosign"): + self.assertNotIn("minLength", cosign["validate"]) + self.assertNotIn("maxLength", cosign["validate"]) From 757b9fb3c0ad6ad2e750733dfc569c1bc49e8567 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 15 Mar 2024 17:24:57 +0100 Subject: [PATCH 3/3] :card_file_box: [#4016] Set up data migration to fix invalid formio configuration The invalid validate keys are dropped from the existing configuration. This patch also ensures that old form imports are automatically patched up during import. --- src/openforms/formio/migration_converters.py | 44 +++++++++++++++++-- ...0096_fix_invalid_validate_configuration.py | 16 ++++++- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/openforms/formio/migration_converters.py b/src/openforms/formio/migration_converters.py index 504fb0ab1f..9a2c191871 100644 --- a/src/openforms/formio/migration_converters.py +++ b/src/openforms/formio/migration_converters.py @@ -149,10 +149,27 @@ def prevent_datetime_components_from_emptying_invalid_values( return True +def fix_empty_validate_lengths(component: Component) -> bool: + if not (validate := component.get("validate")): + return False + + changed = False + for key in ("minLength", "maxLength", "min", "max", "minWords", "maxWords"): + if key in validate and validate[key] == "": # type: ignore + changed = True + del validate[key] # type: ignore + + return changed + + CONVERTERS: dict[str, dict[str, ComponentConverter]] = { # Input components "textfield": { "alter_prefill_default_values": alter_prefill_default_values, + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, + "email": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, }, "date": { "alter_prefill_default_values": alter_prefill_default_values, @@ -164,22 +181,43 @@ def prevent_datetime_components_from_emptying_invalid_values( "time": { "move_time_validators": move_time_validators, }, - "select": {"set_openforms_datasrc": set_openforms_datasrc}, - "selectboxes": {"set_openforms_datasrc": set_openforms_datasrc}, - "radio": {"set_openforms_datasrc": set_openforms_datasrc}, + "phoneNumber": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, "postcode": { "alter_prefill_default_values": alter_prefill_default_values, "ensure_validate_pattern": ensure_postcode_validate_pattern, + "fix_empty_validate_lengths": fix_empty_validate_lengths, }, "file": { "fix_default_value": fix_file_default_value, }, + "textarea": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, + "number": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, + "select": {"set_openforms_datasrc": set_openforms_datasrc}, + "selectboxes": {"set_openforms_datasrc": set_openforms_datasrc}, + "currency": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, + "radio": {"set_openforms_datasrc": set_openforms_datasrc}, # Special components + "iban": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, "licenseplate": { "ensure_validate_pattern": ensure_licensplate_validate_pattern, + "fix_empty_validate_lengths": fix_empty_validate_lengths, }, "bsn": { "alter_prefill_default_values": alter_prefill_default_values, + "fix_empty_validate_lengths": fix_empty_validate_lengths, + }, + "cosign": { + "fix_empty_validate_lengths": fix_empty_validate_lengths, }, # Layout components "columns": { diff --git a/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py b/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py index 20e9ad9c57..9bb17edd48 100644 --- a/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py +++ b/src/openforms/forms/migrations/0096_fix_invalid_validate_configuration.py @@ -2,6 +2,8 @@ from django.db import migrations +from openforms.forms.migration_operations import ConvertComponentsOperation + class Migration(migrations.Migration): @@ -9,4 +11,16 @@ class Migration(migrations.Migration): ("forms", "0095_merge_20240313_1742"), ] - operations = [] + operations = [ + ConvertComponentsOperation("textfield", "fix_empty_validate_lengths"), + ConvertComponentsOperation("email", "fix_empty_validate_lengths"), + ConvertComponentsOperation("phoneNumber", "fix_empty_validate_lengths"), + ConvertComponentsOperation("postcode", "fix_empty_validate_lengths"), + ConvertComponentsOperation("textarea", "fix_empty_validate_lengths"), + ConvertComponentsOperation("number", "fix_empty_validate_lengths"), + ConvertComponentsOperation("currency", "fix_empty_validate_lengths"), + ConvertComponentsOperation("iban", "fix_empty_validate_lengths"), + ConvertComponentsOperation("licenseplate", "fix_empty_validate_lengths"), + ConvertComponentsOperation("bsn", "fix_empty_validate_lengths"), + ConvertComponentsOperation("cosign", "fix_empty_validate_lengths"), + ]