Hot-path keyword validators fix (rebased) #445
Conversation
p1c2u
force-pushed
the
fix/hot-path-keywords-fix-rebased
branch
from
bb487a7 to
28ca78a
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes the performance of keyword validators by refactoring hot-path operations to minimize expensive SchemaPath traversals and operations. The changes focus on reading values once and checking types directly rather than repeatedly accessing SchemaPath objects.
Changes:
- Changed
schema_ids_registryfromOptional[list[int]]toset[int]for O(1) lookups and removed unnecessary assertion - Refactored validation pattern across multiple validators to read values first with
read_value()and check types withisinstance()before accessing nested properties - Optimized iteration patterns to use
range(len())with direct indexing instead of iterating over SchemaPath objects
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| yield from super().__call__(parameter) | ||
|
|
||
| if "default" in parameter: | ||
| parameter_value = parameter.read_value() |
There was a problem hiding this comment.
The method calls parameter.read_value() on line 213 after the parent class already called it on line 197 (via super().__call__(parameter) on line 211). While this pattern existed before this PR, it goes against the performance optimization goals. Consider refactoring the parent class to return the parameter_value so the child class can reuse it, avoiding the duplicate read operation.
| parameter_value = parameter.read_value() | ||
| if not isinstance(parameter_value, dict): | ||
| continue | ||
| key = (parameter_value.get("name"), parameter_value.get("in")) |
There was a problem hiding this comment.
The change from parameter['name'] and parameter['in'] to parameter_value.get("name") and parameter_value.get("in") alters behavior. The old code would raise a KeyError if these required fields were missing, while the new code silently treats them as None. This could lead to multiple parameters with missing fields being incorrectly grouped under the same key (None, None) for duplicate detection. If missing fields should be caught by schema validation elsewhere, this is acceptable, but the behavior change should be intentional and documented.
| yield from self.parameter_validator(parameter) | ||
|
|
||
| key = (parameter["name"], parameter["in"]) | ||
| parameter_value = parameter.read_value() |
There was a problem hiding this comment.
Within the loop, parameter.read_value() is called on line 233 after parameter_validator(parameter) on line 231 has already called read_value() internally. While this pattern may have existed before, it results in duplicate read operations. Consider if the parameter_validator could return the read value to avoid this duplication, consistent with the performance optimization goals of this PR.
| if key in seen: | ||
| yield ParameterDuplicateError( | ||
| f"Duplicate parameter `{parameter['name']}`" | ||
| f"Duplicate parameter `{parameter_value.get('name')}`" |
There was a problem hiding this comment.
The error message on line 239 uses parameter_value.get('name') which could be None if the 'name' field is missing. This would result in an error message like "Duplicate parameter None" which is not user-friendly. Consider using a more descriptive message when the name is missing, or ensure that schema validation catches this case before reaching this code.
| self.schema_ids_registry.append(schema_id) | ||
| self.schema_ids_registry.add(schema_id) | ||
|
|
||
| schema_dict = schema_value |
There was a problem hiding this comment.
The assignment schema_dict = schema_value on line 115 is redundant since schema_value is already assigned on line 106 and both variables refer to the same object. Consider removing this line and using schema_value consistently throughout the method, or rename schema_value to schema_dict at line 106 to avoid confusion.
No description provided.