From 1ed261d4b246d1754530cc107fedac7dd935d386 Mon Sep 17 00:00:00 2001 From: Tobias Kunze Date: Fri, 7 Nov 2025 11:28:49 +0100 Subject: [PATCH 1/3] Coerce string-numbers to numbers in admin form --- src/servala/core/forms.py | 39 ++++++++++++ src/tests/test_form_config.py | 116 ++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/src/servala/core/forms.py b/src/servala/core/forms.py index 2b283f2..116e4d9 100644 --- a/src/servala/core/forms.py +++ b/src/servala/core/forms.py @@ -161,6 +161,9 @@ class ServiceDefinitionAdminForm(forms.ModelForm): form_config = cleaned_data.get("form_config") if form_config: + form_config = self._normalize_form_config_types(form_config) + cleaned_data["form_config"] = form_config + try: jsonschema.validate( instance=form_config, schema=self.form_config_schema @@ -182,6 +185,42 @@ class ServiceDefinitionAdminForm(forms.ModelForm): return cleaned_data + def _normalize_form_config_types(self, form_config): + """ + Normalize form_config by converting string representations of numbers + to actual integers/floats. The JSON form widget sends all values + as strings, but the schema expects proper types. + """ + if not isinstance(form_config, dict): + return form_config + + integer_fields = ["max_length", "rows", "min_values", "max_values"] + number_fields = ["min_value", "max_value"] + + for fieldset in form_config.get("fieldsets", []): + for field in fieldset.get("fields", []): + for field_name in integer_fields: + if field_name in field and field[field_name] is not None: + value = field[field_name] + if isinstance(value, str): + try: + field[field_name] = int(value) if value else None + except (ValueError, TypeError): + pass + + for field_name in number_fields: + if field_name in field and field[field_name] is not None: + value = field[field_name] + if isinstance(value, str): + try: + field[field_name] = ( + int(value) if "." not in value else float(value) + ) + except (ValueError, TypeError): + pass + + return form_config + def _validate_field_mappings(self, form_config, cleaned_data): if not self.instance.pk: return diff --git a/src/tests/test_form_config.py b/src/tests/test_form_config.py index 0228107..feede60 100644 --- a/src/tests/test_form_config.py +++ b/src/tests/test_form_config.py @@ -633,3 +633,119 @@ def test_default_value_not_override_existing_instance(): assert form.initial["name"] == "existing-name" assert form.initial["port"] == 3000 + + +def test_form_config_coerces_string_numbers_to_integers(): + form = ServiceDefinitionAdminForm() + schema = form.form_config_schema + + config_with_string_numbers = { + "fieldsets": [ + { + "fields": [ + { + "type": "text", + "label": "Service Name", + "controlplane_field_mapping": "spec.serviceName", + "max_length": "64", # String instead of integer + "required": True, + }, + { + "type": "textarea", + "label": "Description", + "controlplane_field_mapping": "spec.description", + "rows": "5", # String instead of integer + "max_length": "500", # String instead of integer + }, + { + "type": "number", + "label": "Port", + "controlplane_field_mapping": "spec.port", + "min_value": "1", # String instead of integer + "max_value": "65535", # String instead of integer + }, + { + "type": "array", + "label": "Tags", + "controlplane_field_mapping": "spec.tags", + "min_values": "0", # String instead of integer + "max_values": "10", # String instead of integer + }, + ] + } + ] + } + + normalized_config = form._normalize_form_config_types(config_with_string_numbers) + fields = normalized_config["fieldsets"][0]["fields"] + + assert fields[0]["max_length"] == 64 + assert isinstance(fields[0]["max_length"], int) + + assert fields[1]["rows"] == 5 + assert isinstance(fields[1]["rows"], int) + assert fields[1]["max_length"] == 500 + assert isinstance(fields[1]["max_length"], int) + + assert fields[2]["min_value"] == 1 + assert isinstance(fields[2]["min_value"], int) + assert fields[2]["max_value"] == 65535 + assert isinstance(fields[2]["max_value"], int) + + assert fields[3]["min_values"] == 0 + assert isinstance(fields[3]["min_values"], int) + assert fields[3]["max_values"] == 10 + assert isinstance(fields[3]["max_values"], int) + + jsonschema.validate(instance=normalized_config, schema=schema) + + +def test_form_config_handles_float_numbers(): + form = ServiceDefinitionAdminForm() + + config_with_floats = { + "fieldsets": [ + { + "fields": [ + { + "type": "number", + "label": "Price", + "controlplane_field_mapping": "spec.price", + "min_value": "0.01", # String float + "max_value": "999.99", # String float + }, + ] + } + ] + } + + normalized_config = form._normalize_form_config_types(config_with_floats) + field = normalized_config["fieldsets"][0]["fields"][0] + + assert field["min_value"] == 0.01 + assert isinstance(field["min_value"], float) + assert field["max_value"] == 999.99 + assert isinstance(field["max_value"], float) + + +def test_form_config_handles_empty_string_as_none(): + form = ServiceDefinitionAdminForm() + + config_with_empty_strings = { + "fieldsets": [ + { + "fields": [ + { + "type": "text", + "label": "Name", + "controlplane_field_mapping": "name", + "max_length": "", # Empty string + }, + ] + } + ] + } + + normalized_config = form._normalize_form_config_types(config_with_empty_strings) + field = normalized_config["fieldsets"][0]["fields"][0] + assert field["max_length"] is None From 6182b36daf16bcd4f7b4e50696219b3ec1e83494 Mon Sep 17 00:00:00 2001 From: Tobias Kunze Date: Fri, 7 Nov 2025 11:36:28 +0100 Subject: [PATCH 2/3] Implicitly fix admin choices configuration --- src/servala/core/forms.py | 28 +++++- src/tests/test_form_config.py | 155 ++++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 2 deletions(-) diff --git a/src/servala/core/forms.py b/src/servala/core/forms.py index 116e4d9..076a9d9 100644 --- a/src/servala/core/forms.py +++ b/src/servala/core/forms.py @@ -278,6 +278,32 @@ class ServiceDefinitionAdminForm(forms.ModelForm): if not mapping: return + field_name = field.get("label", mapping) + custom_choices = field.get("choices", []) + + # Single-element choices [value] are transformed to [value, value] + for i, choice in enumerate(custom_choices): + if not isinstance(choice, (list, tuple)): + errors.append( + _( + "Field '{}': Choice at index {} must be a list or tuple, " + "but got: {}" + ).format(field_name, i, repr(choice)) + ) + return + + choice_len = len(choice) + if choice_len == 1: + custom_choices[i] = [choice[0], choice[0]] + elif choice_len == 0 or choice_len > 2: + errors.append( + _( + "Field '{}': Choice at index {} must have 1 or 2 elements " + "(got {}): {}" + ).format(field_name, i, choice_len, repr(choice)) + ) + return + field_schema = self._get_field_schema(spec_schema, mapping, prefix) if not field_schema: return @@ -286,7 +312,6 @@ class ServiceDefinitionAdminForm(forms.ModelForm): if not control_plane_choices: return - custom_choices = field.get("choices", []) custom_choice_values = [choice[0] for choice in custom_choices] invalid_choices = [ @@ -296,7 +321,6 @@ class ServiceDefinitionAdminForm(forms.ModelForm): ] if invalid_choices: - field_name = field.get("label", mapping) errors.append( _( "Field '{}' has invalid choice values: {}. " diff --git a/src/tests/test_form_config.py b/src/tests/test_form_config.py index feede60..ba44d72 100644 --- a/src/tests/test_form_config.py +++ b/src/tests/test_form_config.py @@ -749,3 +749,158 @@ def test_form_config_handles_empty_string_as_none(): normalized_config = form._normalize_form_config_types(config_with_empty_strings) field = normalized_config["fieldsets"][0]["fields"][0] assert field["max_length"] is None + + +def test_single_element_choices_are_normalized(): + form = ServiceDefinitionAdminForm() + mock_crd = Mock() + mock_crd.resource_schema = { + "properties": { + "spec": { + "properties": { + "version": { + "type": "string", + "enum": ["6.2", "7.0", "7.2"], + } + } + } + } + } + + config_with_single_choices = { + "fieldsets": [ + { + "fields": [ + { + "type": "text", + "label": "Name", + "controlplane_field_mapping": "name", + }, + { + "type": "choice", + "label": "Version", + "controlplane_field_mapping": "spec.version", + "choices": [["6.2"]], # Single element - should be transformed + }, + ] + } + ] + } + + spec_schema = mock_crd.resource_schema["properties"]["spec"] + errors = [] + + for field in config_with_single_choices["fieldsets"][0]["fields"]: + if field.get("type") == "choice": + form._validate_choice_field( + field, field["controlplane_field_mapping"], spec_schema, "spec", errors + ) + + assert len(errors) == 0, f"Expected no errors but got: {errors}" + version_field = config_with_single_choices["fieldsets"][0]["fields"][1] + assert version_field["choices"] == [["6.2", "6.2"]] + + +def test_two_element_choices_work_correctly(): + form = ServiceDefinitionAdminForm() + mock_crd = Mock() + mock_crd.resource_schema = { + "properties": { + "spec": { + "properties": { + "version": { + "type": "string", + "enum": ["6.2", "7.0"], + } + } + } + } + } + + config_with_proper_choices = { + "fieldsets": [ + { + "fields": [ + { + "type": "choice", + "label": "Version", + "controlplane_field_mapping": "spec.version", + "choices": [["6.2", "Version 6.2"], ["7.0", "Version 7.0"]], + }, + ] + } + ] + } + + spec_schema = mock_crd.resource_schema["properties"]["spec"] + errors = [] + + for field in config_with_proper_choices["fieldsets"][0]["fields"]: + if field.get("type") == "choice": + form._validate_choice_field( + field, field["controlplane_field_mapping"], spec_schema, "spec", errors + ) + + assert len(errors) == 0, f"Expected no errors but got: {errors}" + version_field = config_with_proper_choices["fieldsets"][0]["fields"][0] + assert version_field["choices"] == [["6.2", "Version 6.2"], ["7.0", "Version 7.0"]] + + +def test_empty_choices_fail_validation(): + form = ServiceDefinitionAdminForm() + config_with_empty_choice = { + "fieldsets": [ + { + "fields": [ + { + "type": "choice", + "label": "Version", + "controlplane_field_mapping": "spec.version", + "choices": [[]], # Empty choice - invalid + }, + ] + } + ] + } + + errors = [] + + for field in config_with_empty_choice["fieldsets"][0]["fields"]: + if field.get("type") == "choice": + form._validate_choice_field( + field, field["controlplane_field_mapping"], {}, "spec", errors + ) + + assert len(errors) > 0 + assert "must have 1 or 2 elements" in str(errors[0]) + + +def test_three_plus_element_choices_fail_validation(): + form = ServiceDefinitionAdminForm() + config_with_long_choice = { + "fieldsets": [ + { + "fields": [ + { + "type": "choice", + "label": "Version", + "controlplane_field_mapping": "spec.version", + "choices": [ + ["6.2", "Version 6.2", "Extra"] + ], # 3 elements - invalid + }, + ] + } + ] + } + + errors = [] + + for field in config_with_long_choice["fieldsets"][0]["fields"]: + if field.get("type") == "choice": + form._validate_choice_field( + field, field["controlplane_field_mapping"], {}, "spec", errors + ) + + assert len(errors) > 0 + assert "must have 1 or 2 elements" in str(errors[0]) From 985e4b47c00f2ed9bbe1b456ec7853933acc1608 Mon Sep 17 00:00:00 2001 From: Tobias Kunze Date: Fri, 7 Nov 2025 11:41:08 +0100 Subject: [PATCH 3/3] Make expert mode toggle less prominent --- .../templates/includes/tabbed_fieldset_form.html | 11 +++++------ src/servala/static/js/expert-mode.js | 7 ++++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/servala/frontend/templates/includes/tabbed_fieldset_form.html b/src/servala/frontend/templates/includes/tabbed_fieldset_form.html index d07b4e3..ace05af 100644 --- a/src/servala/frontend/templates/includes/tabbed_fieldset_form.html +++ b/src/servala/frontend/templates/includes/tabbed_fieldset_form.html @@ -7,12 +7,11 @@ {% csrf_token %} {% include "frontend/forms/errors.html" %} {% if form %} -
- +
+ {% translate "Show Expert Mode" %}
{% endif %}