Add resource property validation framework to SAM translator. #3805
Conversation
|
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: Removing deprecated and unused |
|
Current usage analysis: What IS being used: However, we cannot delete this file yet because this public facing interface might be used by package consuming this package. |
vicheey
force-pushed
the
validation-framework-extraction
branch
from
188db2a to
b52c322
Compare
samtranslator/model/__init__.py
Outdated
|
|
||
| error_messages = [] | ||
|
|
||
| for rule_type, properties in rules: |
There was a problem hiding this comment.
Can we do something like [ rule.run() for rules in self.__validation_rules__] so we can save all the if /elif below?
samtranslator/model/__init__.py
Outdated
| 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] |
There was a problem hiding this comment.
is there a better name for present? It's hard to understnad
samtranslator/model/__init__.py
Outdated
| 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Updated commit allows mutually inclusive, mutually exclusive, conditionally inclusive and conditionally exclusive.
samtranslator/model/__init__.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
should collect_all_errors and schema_class be a class attr? So it's like this.collect_all_errors and this. schema_class
There was a problem hiding this comment.
Adding a new variable requires adding the class to _keyword. I prefer function parameter for that matter.
samtranslator/model/__init__.py
Outdated
| 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 "") |
There was a problem hiding this comment.
why? why not ", ".join(words)
There was a problem hiding this comment.
This is to allow: A, B, and C, A and B, and A
samtranslator/model/__init__.py
Outdated
| 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: |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
The validate_properties_and_return_model is used by other classes. It is risky to modify the method.
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.
validation
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.