refactor: centralize test and test set type validation#1535
refactor: centralize test and test set type validation#1535harry-rhesis wants to merge 5 commits intomainfrom
Conversation
- Moved `TestType` enum from `rhesis.backend.tasks.enums` to `rhesis.backend.app.constants` alongside `TestSetType`. - Created `format_test_type` and `format_test_set_type` validators in a new `validators.py` file to automatically format string inputs and ensure they match the allowed enum values. - Updated schema field validators in `test.py` and `test_set.py` to use these centralized functions instead of duplicating validation logic. - Updated imports across the codebase to refer to the new `TestType` enum location.
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Centralization looks good, but the new format_test_type / format_test_set_type normalization via v.title() will reject common snake_case inputs (e.g. single_turn → Single_Turn). Given existing *.from_string() helpers (and a frontend interface typed as single_turn|multi_turn), this is likely a breaking change.
Fix suggestion: implement validators using TestType.from_string() / TestSetType.from_string() and return .value so you accept snake_case/hyphenated/case-insensitive aliases while still enforcing the allowed set.
| if v is None: | ||
| return None | ||
|
|
||
| formatted = v.title() |
There was a problem hiding this comment.
Critical: v.title() won’t normalize snake_case/underscore inputs (e.g. "single_turn" → "Single_Turn"), so those will now 422 even though TestType.from_string() / TestSetType.from_string() explicitly support snake_case.
Fix: consider implementing these validators via
TestType.from_string(v)/TestSetType.from_string(v)(and returning.value), so you acceptsingle_turn,single-turn,SINGLE_TURN, etc. while still enforcing the allowed set. (There’s already a frontend interface typed as'single_turn' | 'multi_turn'.)
There was a problem hiding this comment.
Ack — if rejecting snake_case/underscore inputs is intentional for API consistency, then the v.title() approach here is fine and my earlier comment isn’t a blocker.
One thing to consider: since we now have both TestType.from_string() (which does accept snake_case) and these validators (which intentionally don’t), it may be worth updating/removing from_string() (or at least its docstring) to avoid sending mixed signals about what inputs are supported.
|
|
||
| @classmethod | ||
| def from_string(cls, value: str): | ||
| """Get enum from string value (case-insensitive, accepts snake_case and hyphenated).""" |
There was a problem hiding this comment.
Improvement: TestType.from_string()/TestSetType.from_string() already handle snake_case + hyphenated inputs, but the new pydantic validators don’t use them. If you update validators to use from_string(), these helpers will become the single normalization source (and avoid subtle differences like underscores).
There was a problem hiding this comment.
Given your note about strict API enforcement (only Single-Turn/Multi-Turn allowed), I agree these validators shouldn’t normalize snake_case. Suggestion here is just consistency: TestType.from_string() currently claims to accept snake_case+hyphenated; since it’s unused in the backend, consider either removing it or adjusting its docstring/behavior so it doesn’t contradict the strict-validation direction.
|
@peqy Thanks for the suggestion! However, the strict enforcement of the hyphenated format is intentional. The only permissible types are |
|
Overall this refactor looks good and the centralization reduces duplicated validation. Improvement: you mentioned strict enforcement (only Found 1 improvement. |
Fixes a startup error in the SDK test backend caused by the missing SESSION_SECRET_KEY environment variable.
…/TestSetType Update `TestType.from_string()` and `TestSetType.from_string()` to strictly enforce case sensitivity and exact match for allowed values (e.g. `Single-Turn`, `Multi-Turn`), rather than silently mapping `snake_case` or case-insensitive inputs to the canonical forms. This aligns the underlying enum methods with the validation strictness applied in Pydantic models.
|
Looks good now. The earlier inconsistency (validators strict but No further issues from me—ship it. |
1 participant
Purpose
This PR centralizes the validation logic for
test_type,test_set_type, andtest_configurationto prevent pushing garbage types.What Changed
TestTypeintorhesis.backend.app.constantsalongsideTestSetType.format_test_typeandformat_test_set_typevalidators in a newvalidators.pyfile to automatically format inputs usingv.title()and check them against the allowed enum values.test.pyandtest_set.pymodels with the centralized methods.TestTypemodule location.