Simplify Lab Schema Validation #146
Conversation
…ove redundant checks
alexchristy
added
minor
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies schema validation logic by migrating from Pydantic's field_validator to model_validator with mode "after", enabling access to all validated class attributes and eliminating redundant null checks. The change centralizes validation logic through reusable mixin classes that can be inherited across different schema types.
Key changes:
- Replaced individual field validators with model validators that access validated class attributes directly
- Created validation mixin classes (
VPCCreateValidationMixin,SubnetCreateValidationMixin,RangeCreateValidationMixin) to eliminate code duplication - Removed null/existence checks for fields that are guaranteed to be validated by Pydantic
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| vpc_schemas.py | Introduces VPCCreateValidationMixin with model validators and removes duplicate validation logic from both blueprint and deployed VPC creation schemas |
| subnet_schemas.py | Creates SubnetCreateValidationMixin with model validators and refactors blueprint and deployed subnet schemas to use the mixin |
| range_schemas.py | Adds RangeCreateValidationMixin with model validators for VPC validation and removes duplicate validation code from range creation schemas |
| host_schemas.py | Converts field validator to model validator for size validation, removing the need for null checks on the OS field |
| raise ValueError(msg) | ||
|
|
||
| msg = f"All subnet in VPC: {vpc_name} must have unique names." | ||
| msg = f"All subnet in VPC: {self.name} must have unique names." |
There was a problem hiding this comment.
Grammar error: 'All subnet' should be 'All subnets' (missing 's' for plural).
| msg = f"All subnet in VPC: {self.name} must have unique names." | |
| msg = f"All subnets in VPC: {self.name} must have unique names." |
|
|
||
| vpc_cidrs = [vpc.cidr for vpc in self.vpcs] | ||
| if not mutually_exclusive_networks_v4(vpc_cidrs): | ||
| msg = "All VPCs in the range must be mutually exclusive (not overlap)." |
There was a problem hiding this comment.
[nitpick] The error message differs from the original 'All VPCs in range should be mutually exclusive (not overlap).' Consider maintaining consistency with the original wording unless the change is intentional.
| msg = "All VPCs in the range must be mutually exclusive (not overlap)." | |
| msg = "All VPCs in range should be mutually exclusive (not overlap)." |
Checklist
Description
This PR simplifies validation logic for the
Range,VPC,Subnet, andHostschemas by leveraging Pydantic'smodel_validatorthat provides access to class attributes after each field has been individually validated. This allows for the removal of all theif not fieldchecks.Additionally, this PR deduplicates the validation logic by centralizing the validation functions in a
ValidationSchemaclass that can be inheritedFixes: #107