From 50fcaf1b8255af6a6f3b216ef1d93dbe0aa94360 Mon Sep 17 00:00:00 2001 From: Vishnu KV Date: Sat, 24 May 2025 15:07:13 +0530 Subject: [PATCH 1/3] Add validation for required title and description fields in properties - Introduced a new function `check_properties_have_required_fields` to ensure that all user-facing properties in the spec have both title and description fields. - Updated the `validate_yaml` function to include this new validation check. - Added comprehensive unit tests to cover various scenarios, including missing title, description, and both fields, as well as handling of override-disabled properties. --- ftf_cli/utils.py | 54 +++++ tests/test_utils_validation.py | 371 +++++++++++++++++++++++++-------- 2 files changed, 334 insertions(+), 91 deletions(-) diff --git a/ftf_cli/utils.py b/ftf_cli/utils.py index b48842e..7d48a03 100644 --- a/ftf_cli/utils.py +++ b/ftf_cli/utils.py @@ -271,6 +271,58 @@ def update_spec_variable( return +def check_properties_have_required_fields(spec_obj, path="spec"): + """ + Recursively check that individual properties within spec.properties have proper title and description fields. + This ensures that user-facing properties are properly documented. + Raises a UsageError if a property is missing title or description. + """ + if not isinstance(spec_obj, dict): + return + + # Check if this is a properties object + if "properties" in spec_obj and isinstance(spec_obj["properties"], dict): + properties = spec_obj["properties"] + for prop_name, prop_def in properties.items(): + if isinstance(prop_def, dict): + # Check if this property has a type (indicating it's a user-facing property) + if "type" in prop_def: + # Skip properties that are marked as override-disabled (internal/system properties) + # Properties with x-ui-overrides-only are still user-facing and need validation + override_disable_flag = prop_def.get("x-ui-override-disable", False) + + if not override_disable_flag: + missing_fields = [] + + # Check if title is missing + if "title" not in prop_def or not prop_def["title"]: + missing_fields.append("title") + + # Check if description is missing + if "description" not in prop_def or not prop_def["description"]: + missing_fields.append("description") + + if missing_fields: + if len(missing_fields) == 1: + field_text = f"'{missing_fields[0]}' field" + else: + fields_str = "', '".join(missing_fields) + field_text = f"'{fields_str}' fields" + + raise click.UsageError( + f"Missing required {field_text} for property '{prop_name}' at {path}.properties.{prop_name}. " + f"All user-facing properties must have both title and description to help users understand their purpose." + ) + + # Recursively check nested objects + check_properties_have_required_fields(prop_def, path=f"{path}.properties.{prop_name}") + + # Also check nested objects that might have their own properties + for key, value in spec_obj.items(): + if isinstance(value, dict) and key != "properties": + check_properties_have_required_fields(value, path=f"{path}.{key}") + + def check_no_array_or_invalid_pattern_in_spec(spec_obj, path="spec"): """ Recursively check that no field in spec is of type 'array'. @@ -361,6 +413,8 @@ def validate_yaml(data): if spec_obj: check_no_array_or_invalid_pattern_in_spec(spec_obj) check_conflicting_ui_properties(spec_obj) + # Check that properties have proper title and description fields + check_properties_have_required_fields(spec_obj) except jsonschema.exceptions.ValidationError as e: raise click.UsageError( f"Validation error in `facets.yaml`: `facets.yaml` is not following Facets Schema: {e}" diff --git a/tests/test_utils_validation.py b/tests/test_utils_validation.py index 375ff02..2e5e006 100644 --- a/tests/test_utils_validation.py +++ b/tests/test_utils_validation.py @@ -1,6 +1,7 @@ import pytest import click from ftf_cli.utils import check_no_array_or_invalid_pattern_in_spec, check_conflicting_ui_properties +from ftf_cli.utils import check_no_array_or_invalid_pattern_in_spec, check_properties_have_required_fields, validate_yaml def test_no_array_type_pass(): @@ -54,145 +55,333 @@ def test_nested_structure_with_array_type_raises(): assert "Invalid array type found at spec.level1.level2.field" in str(excinfo.value) -# Tests for check_conflicting_ui_properties function -def test_no_conflicting_properties_pass(): - """Test that valid configurations pass without errors.""" +# Tests for property title and description validation +def test_properties_with_title_and_description_pass(): + """Test that properties with both title and description pass validation.""" spec = { - "field1": {"type": "string"}, - "field2": {"type": "string", "x-ui-yaml-editor": True}, - "field3": {"type": "object", "patternProperties": {"^.*$": {"type": "object"}}}, - "field4": {"type": "string", "x-ui-override-disable": True}, - "field5": {"type": "string", "x-ui-overrides-only": True} + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context to create google cloud resources" + }, + "service_account": { + "type": "string", + "title": "Service Account", + "description": "The service account to impersonate" + } + } } # Should pass silently - check_conflicting_ui_properties(spec) + check_properties_have_required_fields(spec) -def test_pattern_properties_with_yaml_editor_raises(): - """Test that patternProperties + x-ui-yaml-editor conflict is detected.""" +def test_property_missing_title_raises(): + """Test that property missing title raises error.""" spec = { - "field": { - "type": "object", - "patternProperties": {"^.*$": {"type": "object"}}, - "x-ui-yaml-editor": True + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context to create google cloud resources" + }, + "service_account": { + "type": "string", + "description": "The service account to impersonate" + # Missing title + } } } with pytest.raises(click.UsageError) as excinfo: - check_conflicting_ui_properties(spec) - assert "Configuration conflict at spec.field" in str(excinfo.value) - assert "patternProperties" in str(excinfo.value) - assert "x-ui-yaml-editor: true" in str(excinfo.value) - assert "mutually exclusive" in str(excinfo.value) + check_properties_have_required_fields(spec) + assert "Missing required 'title' field for property 'service_account'" in str(excinfo.value) + assert "spec.properties.service_account" in str(excinfo.value) -def test_override_disable_with_overrides_only_raises(): - """Test that x-ui-override-disable + x-ui-overrides-only conflict is detected.""" +def test_property_missing_description_raises(): + """Test that property missing description raises error.""" spec = { - "field": { - "type": "string", - "x-ui-override-disable": True, - "x-ui-overrides-only": True + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context to create google cloud resources" + }, + "service_account": { + "type": "string", + "title": "Service Account" + # Missing description + } } } with pytest.raises(click.UsageError) as excinfo: - check_conflicting_ui_properties(spec) - assert "Configuration conflict at spec.field" in str(excinfo.value) - assert "x-ui-override-disable: true" in str(excinfo.value) - assert "x-ui-overrides-only: true" in str(excinfo.value) - assert "cannot be overridden and will only have a default value" in str(excinfo.value) - assert "must be specified at environment level via overrides" in str(excinfo.value) + check_properties_have_required_fields(spec) + assert "Missing required 'description' field for property 'service_account'" in str(excinfo.value) + assert "spec.properties.service_account" in str(excinfo.value) -def test_nested_conflicting_properties_raises(): - """Test that conflicts in nested structures are detected with correct path.""" +def test_property_missing_both_title_and_description_raises(): + """Test that property missing both title and description raises error.""" spec = { - "level1": { - "level2": { - "field": { - "type": "string", - "x-ui-override-disable": True, - "x-ui-overrides-only": True - } + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context to create google cloud resources" + }, + "service_account": { + "type": "string" + # Missing both title and description } } } with pytest.raises(click.UsageError) as excinfo: - check_conflicting_ui_properties(spec) - assert "Configuration conflict at spec.level1.level2.field" in str(excinfo.value) + check_properties_have_required_fields(spec) + error_message = str(excinfo.value) + assert "Missing required 'title', 'description' fields for property 'service_account'" in error_message + assert "spec.properties.service_account" in error_message -def test_pattern_properties_with_yaml_editor_false_pass(): - """Test that patternProperties with x-ui-yaml-editor: false is allowed.""" +def test_property_empty_fields_raises(): + """Test that property with empty title and description raises error.""" spec = { - "field": { - "type": "object", - "patternProperties": {"^.*$": {"type": "object"}}, - "x-ui-yaml-editor": False + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "", # Empty title + "description": "" # Empty description + } } } - # Should pass silently - check_conflicting_ui_properties(spec) + with pytest.raises(click.UsageError) as excinfo: + check_properties_have_required_fields(spec) + error_message = str(excinfo.value) + assert "Missing required 'title', 'description' fields for property 'project'" in error_message -def test_override_disable_false_with_overrides_only_true_pass(): - """Test that x-ui-override-disable: false with x-ui-overrides-only: true is allowed.""" +def test_override_disabled_properties_skip_validation(): + """Test that properties with x-ui-override-disable skip title and description validation.""" spec = { - "field": { - "type": "string", - "x-ui-override-disable": False, - "x-ui-overrides-only": True + "type": "object", + "properties": { + "internal_field": { + "type": "string", + "x-ui-override-disable": True + # No title or description required for override-disabled fields + }, + "user_field": { + "type": "string", + "title": "User Field", + "description": "A user-facing field" + } } } # Should pass silently - check_conflicting_ui_properties(spec) + check_properties_have_required_fields(spec) + + +def test_overrides_only_properties_require_validation(): + """Test that properties with x-ui-overrides-only still require title and description validation.""" + spec = { + "type": "object", + "properties": { + "override_field": { + "type": "string", + "x-ui-overrides-only": True + # Missing title and description - should fail validation + }, + "user_field": { + "type": "string", + "title": "User Field", + "description": "A user-facing field" + } + } + } + # Should fail because override_field is missing title and description + with pytest.raises(click.UsageError) as excinfo: + check_properties_have_required_fields(spec) + error_message = str(excinfo.value) + assert "Missing required 'title', 'description' fields for property 'override_field'" in error_message + assert "spec.properties.override_field" in error_message -def test_override_disable_true_with_overrides_only_false_pass(): - """Test that x-ui-override-disable: true with x-ui-overrides-only: false is allowed.""" +def test_overrides_only_properties_with_title_and_description_pass(): + """Test that properties with x-ui-overrides-only pass when they have title and description.""" spec = { - "field": { - "type": "string", - "x-ui-override-disable": True, - "x-ui-overrides-only": False + "type": "object", + "properties": { + "override_field": { + "type": "string", + "title": "Override Field", + "description": "A field available for overrides", + "x-ui-overrides-only": True + }, + "user_field": { + "type": "string", + "title": "User Field", + "description": "A user-facing field" + } } } # Should pass silently - check_conflicting_ui_properties(spec) + check_properties_have_required_fields(spec) -def test_multiple_conflicts_in_different_fields(): - """Test that the function detects the first conflict encountered.""" +def test_nested_object_properties_validation(): + """Test that nested object properties are also validated for title and description.""" spec = { - "field1": { + "type": "object", + "properties": { + "database": { + "type": "object", + "title": "Database Configuration", + "description": "Database configuration", + "properties": { + "host": { + "type": "string", + "title": "Database Host", + "description": "Database host" + }, + "port": { + "type": "number", + "title": "Database Port" + # Missing description - should fail + } + } + } + } + } + with pytest.raises(click.UsageError) as excinfo: + check_properties_have_required_fields(spec) + assert "Missing required 'description' field for property 'port'" in str(excinfo.value) + assert "spec.properties.database.properties.port" in str(excinfo.value) + + +def test_validate_yaml_with_missing_property_title(): + """Test that validate_yaml function catches missing property title.""" + yaml_data = { + "intent": "test-intent", + "flavor": "test-flavor", + "version": "1.0", + "description": "Test module description", + "clouds": ["aws"], + "spec": { "type": "object", - "patternProperties": {"^.*$": {"type": "object"}}, - "x-ui-yaml-editor": True - }, - "field2": { - "type": "string", - "x-ui-override-disable": True, - "x-ui-overrides-only": True + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context" + }, + "region": { + "type": "string", + "description": "The region to deploy resources" + # Missing title + } + } } } with pytest.raises(click.UsageError) as excinfo: - check_conflicting_ui_properties(spec) - # Should detect one of the conflicts (order may vary based on dict iteration) - assert "Configuration conflict at spec." in str(excinfo.value) + validate_yaml(yaml_data) + assert "Missing required 'title' field for property 'region'" in str(excinfo.value) -def test_empty_spec_pass(): - """Test that empty spec passes validation.""" - spec = {} - # Should pass silently - check_conflicting_ui_properties(spec) +def test_validate_yaml_with_missing_property_description(): + """Test that validate_yaml function catches missing property description.""" + yaml_data = { + "intent": "test-intent", + "flavor": "test-flavor", + "version": "1.0", + "description": "Test module description", + "clouds": ["aws"], + "spec": { + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context" + }, + "region": { + "type": "string", + "title": "Region" + # Missing description + } + } + } + } + with pytest.raises(click.UsageError) as excinfo: + validate_yaml(yaml_data) + assert "Missing required 'description' field for property 'region'" in str(excinfo.value) -def test_non_dict_values_ignored(): - """Test that non-dict values are ignored gracefully.""" - spec = { - "field1": "string_value", - "field2": 123, - "field3": {"type": "string"} +def test_validate_yaml_with_overrides_only_missing_fields(): + """Test that validate_yaml fails when x-ui-overrides-only properties are missing title/description.""" + yaml_data = { + "intent": "test-intent", + "flavor": "test-flavor", + "version": "1.0", + "description": "Test module description", + "clouds": ["aws"], + "spec": { + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context" + }, + "override_field": { + "type": "string", + "description": "An override field", + "x-ui-overrides-only": True + # Missing title - should fail + } + } + } } - # Should pass silently - check_conflicting_ui_properties(spec) + with pytest.raises(click.UsageError) as excinfo: + validate_yaml(yaml_data) + assert "Missing required 'title' field for property 'override_field'" in str(excinfo.value) + + +def test_validate_yaml_with_proper_property_fields_passes(): + """Test that validate_yaml passes when all properties have title and description.""" + yaml_data = { + "intent": "test-intent", + "flavor": "test-flavor", + "version": "1.0", + "description": "Test module description", + "clouds": ["aws"], + "spec": { + "title": "Test Spec", # Optional + "description": "Test spec description", # Optional + "type": "object", + "properties": { + "project": { + "type": "string", + "title": "Project ID", + "description": "The project in context to create google cloud resources" + }, + "service_account": { + "type": "string", + "title": "Service Account", + "description": "The service account to impersonate" + }, + "override_field": { + "type": "string", + "title": "Override Field", + "description": "A field available for overrides", + "x-ui-overrides-only": True + } + } + } + } + # Should pass without raising an exception + result = validate_yaml(yaml_data) + assert result is True From 7f648fecdacf0cf64269d19cb60b3abf2fdeebfd Mon Sep 17 00:00:00 2001 From: Vishnu KV Date: Mon, 26 May 2025 19:26:57 +0530 Subject: [PATCH 2/3] Enhance property validation to support skipping for override-disabled objects - Updated `check_properties_have_required_fields` to include a `skip_validation` parameter, allowing validation to be skipped for properties marked with `x-ui-override-disable`. - Added a new test case to ensure that nested properties under override-disabled objects are correctly skipped during validation. --- ftf_cli/utils.py | 19 +++++++++++---- tests/test_utils_validation.py | 44 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/ftf_cli/utils.py b/ftf_cli/utils.py index 7d48a03..2a35f4e 100644 --- a/ftf_cli/utils.py +++ b/ftf_cli/utils.py @@ -271,11 +271,16 @@ def update_spec_variable( return -def check_properties_have_required_fields(spec_obj, path="spec"): +def check_properties_have_required_fields(spec_obj, path="spec", skip_validation=False): """ Recursively check that individual properties within spec.properties have proper title and description fields. This ensures that user-facing properties are properly documented. Raises a UsageError if a property is missing title or description. + + Args: + spec_obj: The spec object to validate + path: The current path in the spec for error reporting + skip_validation: If True, skip validation for this object and all nested objects """ if not isinstance(spec_obj, dict): return @@ -291,7 +296,10 @@ def check_properties_have_required_fields(spec_obj, path="spec"): # Properties with x-ui-overrides-only are still user-facing and need validation override_disable_flag = prop_def.get("x-ui-override-disable", False) - if not override_disable_flag: + # Skip validation if parent is override-disabled or this property is override-disabled + should_skip = skip_validation or override_disable_flag + + if not should_skip: missing_fields = [] # Check if title is missing @@ -314,13 +322,14 @@ def check_properties_have_required_fields(spec_obj, path="spec"): f"All user-facing properties must have both title and description to help users understand their purpose." ) - # Recursively check nested objects - check_properties_have_required_fields(prop_def, path=f"{path}.properties.{prop_name}") + # Recursively check nested objects, propagating skip flag if this property is override-disabled + nested_skip = skip_validation or override_disable_flag + check_properties_have_required_fields(prop_def, path=f"{path}.properties.{prop_name}", skip_validation=nested_skip) # Also check nested objects that might have their own properties for key, value in spec_obj.items(): if isinstance(value, dict) and key != "properties": - check_properties_have_required_fields(value, path=f"{path}.{key}") + check_properties_have_required_fields(value, path=f"{path}.{key}", skip_validation=skip_validation) def check_no_array_or_invalid_pattern_in_spec(spec_obj, path="spec"): diff --git a/tests/test_utils_validation.py b/tests/test_utils_validation.py index 2e5e006..ad5c9e2 100644 --- a/tests/test_utils_validation.py +++ b/tests/test_utils_validation.py @@ -185,6 +185,50 @@ def test_override_disabled_properties_skip_validation(): check_properties_have_required_fields(spec) +def test_nested_override_disabled_objects_skip_validation(): + """Test that nested objects with x-ui-override-disable skip validation for all descendants.""" + spec = { + "type": "object", + "properties": { + "user_config": { + "type": "object", + "title": "User Configuration", + "description": "User-facing configuration", + "properties": { + "name": { + "type": "string", + "title": "Name", + "description": "User name" + } + } + }, + "internal_config": { + "type": "object", + "x-ui-override-disable": True, + # No title or description required for override-disabled objects + "properties": { + "system_id": { + "type": "string" + # No title or description required - should be skipped due to parent override-disable + }, + "nested_internal": { + "type": "object", + # No title or description required - should be skipped due to parent override-disable + "properties": { + "deep_field": { + "type": "string" + # No title or description required - should be skipped due to ancestor override-disable + } + } + } + } + } + } + } + # Should pass silently - nested properties under override-disabled objects should not require validation + check_properties_have_required_fields(spec) + + def test_overrides_only_properties_require_validation(): """Test that properties with x-ui-overrides-only still require title and description validation.""" spec = { From 703b2c5b24de60509c473ce624896eb5dc9a5e6d Mon Sep 17 00:00:00 2001 From: Vishnu KV Date: Tue, 27 May 2025 00:50:50 +0530 Subject: [PATCH 3/3] fix --- ftf_cli/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ftf_cli/utils.py b/ftf_cli/utils.py index 2a35f4e..07d35f3 100644 --- a/ftf_cli/utils.py +++ b/ftf_cli/utils.py @@ -440,7 +440,7 @@ def validate_yaml(data): validate(instance=spec_obj, schema=additional_properties_schema) except jsonschema.exceptions.ValidationError as e: raise click.UsageError( - f"Validation error in `facets.yaml`: Field additionalProperties is not allowed under any object." + "Validation error in `facets.yaml`: Field additionalProperties is not allowed under any object." ) click.echo("✅ Facets YAML validation successful!")