Add resource property validation framework to SAM translator. #3805

vicheey · 2025-08-22T21:16:21Z

Issue #, if available

None

Description of changes

This PR introduces a comprehensive validation framework for SAM resource properties, and enhancing the translator's ability to catch configuration errors early in the deployment process.

Enhanced validation logic in samtranslator/model/init.py
Updated SAM resources in samtranslator/model/sam_resources.py to integrate
validation
Comprehensive test coverage in tests/test_model.py

Description of how you validated changes

Added extensive test suite covering validation scenarios
All existing tests continue to pass

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

vicheey · 2025-08-22T23:23:18Z

The Tests failed because of lower coverage percentage. (94.82% instead of 95%). Added validation logic push the overall total line increase and lower the coverage percentage.

Low coverage files:
• samtranslator/validator/validator.py - 27% coverage
• samtranslator/model/init.py - 69% coverage
• samtranslator/model/naming.py - 71% coverage
• samtranslator/region_configuration.py - 79% coverage

Removing deprecated and unused samtranslator/validator/validator.py

vicheey · 2025-08-22T23:28:39Z

Current usage analysis:
• No active imports of SamTemplateValidator in any source
files
• No test files directly testing SamTemplateValidator

What IS being used:
• samtranslator.validator.value_validator.sam_expect - Used extensively across 12+ files
• Direct jsonschema.validate() calls in tests (instead of the deprecated SAM wrapper)

However, we cannot delete this file yet because this public facing interface might be used by package consuming this package.
I added the samtranslator.validator.validator.py to test coverage ignore list.

--------------------------------
TOTAL   9640    314   3402    220    95%

Required test coverage of 95% reached. Total coverage: 95.28%

=============== 4118 passed in 86.15s (0:01:26) ================

roger-zhangg · 2025-08-25T23:45:43Z

samtranslator/model/__init__.py

+            elif (
+                rule_type == ValidationRule.CONDITIONAL_REQUIREMENT
+                and self._get_property_value(properties[0], validated_model) is not None
+                and self._get_property_value(properties[1], validated_model) is None


you can only set 2 prop in one rule?

roger-zhangg · 2025-08-25T23:49:55Z

samtranslator/model/__init__.py

+
+        error_messages = []
+
+        for rule_type, properties in rules:


Can we do something like [ rule.run() for rules in self.__validation_rules__] so we can save all the if /elif below?

roger-zhangg · 2025-08-25T23:50:50Z

samtranslator/model/__init__.py

+        error_messages = []
+
+        for rule_type, properties in rules:
+            present = [prop for prop in properties if self._get_property_value(prop, validated_model) is not None]


is there a better name for present? It's hard to understnad

roger-zhangg · 2025-08-25T23:52:41Z

samtranslator/model/__init__.py

+                    prop_names = [f"'{p}'" for p in present]
+                    error_messages.append(f"Cannot specify {self._combine_string(prop_names)} together.")
+
+            elif rule_type == ValidationRule.MUTUALLY_INCLUSIVE:


Is this like an antipattern? User need to constantly set 2 or more params to achieve a single action. Is there an example for this MUTUALLY_INCLUSIVE? Can we optimize it to be CONDITIONAL_REQUIREMENT?

roger-zhangg · 2025-08-25T23:55:56Z

samtranslator/model/__init__.py

@@ -582,6 +687,71 @@ def _check_tag(self, reserved_tag_name, tags):  # type: ignore[no-untyped-def]
                "input.",
            )

+    def validate_before_transform(self, schema_class: Optional[Type[RT]], collect_all_errors: bool = False) -> None:


should collect_all_errors and schema_class be a class attr? So it's like this.collect_all_errors and this. schema_class

roger-zhangg · 2025-08-26T00:04:03Z

samtranslator/model/__init__.py

+            raise InvalidResourceException(self.logical_id, "\n".join(error_messages))
+
+    def _combine_string(self, words: List[str]) -> str:
+        return ", ".join(words[:-1]) + (" and " + words[-1] if len(words) > 1 else words[0] if words else "")


why? why not ", ".join(words)

roger-zhangg · 2025-08-26T00:05:19Z

samtranslator/model/__init__.py

@@ -553,6 +624,40 @@ def _construct_tag_list(
        # customer's knowledge.
        return get_tag_list(sam_tag) + get_tag_list(additional_tags) + get_tag_list(tags)

+    @staticmethod
+    def propagate_tags_combine(


This is not mentioned in PR title or description

roger-zhangg · 2025-08-26T00:22:35Z

samtranslator/model/__init__.py

+    def _combine_string(self, words: List[str]) -> str:
+        return ", ".join(words[:-1]) + (" and " + words[-1] if len(words) > 1 else words[0] if words else "")
+
+    def _get_property_value(self, prop: str, validated_model: Any = None) -> Any:


I would suggest to update validate_properties_and_return_model return a custom dict that allow you to directly use validated_model["a.b.c"]. Like

class DotDict(dict): def __getitem__(self, path): value = self for key in path.split('.'): value = dict.__getitem__(value, key) return value

validated_model["a.b.c"] would have a much better readability than _get_property_value("a.b.c", validated_model)

roger-zhangg · 2025-08-26T00:30:52Z

tests/test_model.py

+            class Resource(self.TestResource):
+                __validation_rules__ = [
+                    (self.ValidationRule.CONDITIONAL_REQUIREMENT, ["ConditionalVar1", "ConditionalVar2"]),
+                    (self.ValidationRule.CONDITIONAL_REQUIREMENT, ["ConditionalVar2", "ConditionalVar3"]),


from actually seeing this, does Rule('ConditionalVar2').require('ConditionalVar1') sounds better?
This can even extend to like Rule('ConditionalVar2').require('ConditionalVar1==123')

roger-zhangg · 2025-08-26T00:35:12Z

tests/test_model.py

+                        self.ValidationRule.CONDITIONAL_REQUIREMENT,
+                        ["NestedSetting1.NestedVar1", "NestedSetting2.NestedVar2"],
+                    ),
+                    (self.ValidationRule.MUTUALLY_EXCLUSIVE, ["ExclusiveVar1", "ExclusiveVar2"]),


Rule('ConditionalVar2').reject('ConditionalVar1')

roger-zhangg · 2025-08-26T00:37:21Z

tests/test_model.py

+                        ["NestedSetting1.NestedVar1", "NestedSetting2.NestedVar1"],
+                    ),
+                    (self.ValidationRule.MUTUALLY_EXCLUSIVE, ["NestedSetting1.NestedVar2", "ExclusiveVar3"]),
+                    (self.ValidationRule.MUTUALLY_INCLUSIVE, ["InclusiveVar1", "ExclusiveVar1"]),


Rule('ConditionalVar1', 'ConditionalVar2')
Just some thoughts

roger-zhangg · 2025-08-26T00:41:11Z

samtranslator/model/__init__.py

+        if not hasattr(self, "__validation_rules__"):
+            return
+
+        rules = self.__validation_rules__


One concern, currently there's no type hint on self. __validation_rules__, so we need to look at document/ previous implementation everytime when we need to use this function

add resource property validation rule and logic

89f8a01

vicheey requested a review from a team as a code owner August 22, 2025 21:16

Merge branch 'develop' into validation-framework-extraction

f762c86

vicheey requested review from bnusunny and roger-zhangg August 22, 2025 21:17

comment out validation rules for functions

d5410b0

chore: ignore validator.oy from test coverage

b52c322

vicheey force-pushed the validation-framework-extraction branch from 188db2a to b52c322 Compare August 22, 2025 23:50

roger-zhangg reviewed Aug 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add resource property validation framework to SAM translator. #3805

Add resource property validation framework to SAM translator. #3805

vicheey commented Aug 22, 2025

Uh oh!

vicheey commented Aug 22, 2025

Uh oh!

vicheey commented Aug 22, 2025 •

edited

Loading

Uh oh!

roger-zhangg Aug 25, 2025

Uh oh!

roger-zhangg Aug 25, 2025

Uh oh!

roger-zhangg Aug 25, 2025

Uh oh!

roger-zhangg Aug 25, 2025

Uh oh!

roger-zhangg Aug 25, 2025

Uh oh!

roger-zhangg Aug 26, 2025

Uh oh!

roger-zhangg Aug 26, 2025

Uh oh!

roger-zhangg Aug 26, 2025 •

edited

Loading

Uh oh!

roger-zhangg Aug 26, 2025 •

edited

Loading

Uh oh!

roger-zhangg Aug 26, 2025 •

edited

Loading

Uh oh!

roger-zhangg Aug 26, 2025 •

edited

Loading

Uh oh!

roger-zhangg Aug 26, 2025 •

edited

Loading

Uh oh!

Uh oh!

Add resource property validation framework to SAM translator. #3805

Are you sure you want to change the base?

Add resource property validation framework to SAM translator. #3805

Conversation

vicheey commented Aug 22, 2025

Issue #, if available

Description of changes

Description of how you validated changes

Checklist

Uh oh!

vicheey commented Aug 22, 2025

Uh oh!

vicheey commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roger-zhangg Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vicheey commented Aug 22, 2025 •

edited

Loading

roger-zhangg Aug 26, 2025 •

edited

Loading

roger-zhangg Aug 26, 2025 •

edited

Loading

roger-zhangg Aug 26, 2025 •

edited

Loading

roger-zhangg Aug 26, 2025 •

edited

Loading

roger-zhangg Aug 26, 2025 •

edited

Loading