feat: replace check_expected_keys with typed pydantic models (#1034) - #1071
Open
reachsridhard wants to merge 7 commits into
Open
feat: replace check_expected_keys with typed pydantic models (#1034)#1071reachsridhard wants to merge 7 commits into
reachsridhard wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThis change adds shared Pydantic models for REST, MQTT, and gRPC specifications, replaces plugin-specific key checks, adds type validation coverage, and moves Pydantic to the runtime dependency set. ChangesPydantic validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tavern/_core/pydantic_models.py`:
- Around line 19-52: Sanitize type-validation errors in
_BaseKeyValidator.validate_keys so UnexpectedKeysError never exposes raw input
values. Enable Pydantic’s input-repr hiding option in model_config and replace
the direct str(e) fallback with a safe validation-error summary that retains
field and type information without including offending values; preserve the
existing unexpected-key handling.
In `@tests/unit/test_pydantic_models.py`:
- Around line 300-309: Reformat the data dictionary literals in
test_grpc_request_body_can_be_dict and test_grpc_request_body_can_be_string
using ruff-format’s multi-line layout so the tests pass formatting checks; do
not change their behavior or assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf83fe31-983a-4a53-9831-50bfe3530dd6
📒 Files selected for processing (9)
pyproject.tomltavern/_core/pydantic_models.pytavern/_plugins/grpc/client.pytavern/_plugins/grpc/request.pytavern/_plugins/grpc/response.pytavern/_plugins/mqtt/client.pytavern/_plugins/mqtt/request.pytavern/_plugins/rest/request.pytests/unit/test_pydantic_models.py
Comment on lines
+19
to
+52
| class _BaseKeyValidator(BaseModel): | ||
| """Base model that forbids extra keys and raises UnexpectedKeysError on validation failure.""" | ||
|
|
||
| model_config = ConfigDict( | ||
| extra="forbid", arbitrary_types_allowed=True, populate_by_name=True | ||
| ) | ||
|
|
||
| @classmethod | ||
| def validate_keys(cls, data: Mapping) -> dict: | ||
| """Validate that ``data`` contains only expected keys and types. | ||
|
|
||
| Args: | ||
| data: Dictionary to validate against this model's fields. | ||
|
|
||
| Returns: | ||
| The validated data as a dict. | ||
|
|
||
| Raises: | ||
| exceptions.UnexpectedKeysError: If unexpected keys are present or | ||
| a value has an invalid type. | ||
| """ | ||
| try: | ||
| return cls(**dict(data)).model_dump(exclude_unset=True, by_alias=True) | ||
| except ValidationError as e: | ||
| # Extract unexpected field names from the error | ||
| unexpected = set() | ||
| for err in e.errors(): | ||
| if err["type"] == "extra_forbidden": | ||
| unexpected.add(err["loc"][-1]) | ||
| if unexpected: | ||
| msg = f"Unexpected keys {unexpected}" | ||
| else: | ||
| msg = str(e) | ||
| raise exceptions.UnexpectedKeysError(msg) from e |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does pydantic v2 ConfigDict "hide_input_in_errors" also suppress input values from ValidationError.errors() dict output, or only from __str__()?
💡 Result:
No, the Pydantic v2 hide_input_in_errors configuration option does not suppress input values from the dictionary output returned by ValidationError.errors() or the JSON output from ValidationError.json() [1][2]. The hide_input_in_errors setting specifically only affects the string representation (str) and the repr of the ValidationError object [1]. When you print the exception or convert it to a string, Pydantic will omit the input_value and input_type fields from that text output [3][4][5]. However, the .errors() and .json() methods include the input field by default, and they do not currently respect the hide_input_in_errors configuration flag [1][2]. To remove input values from the output of these methods, you must explicitly pass include_input=False when calling them, for example: e.errors(include_input=False) or e.json(include_input=False) [1][2].
Citations:
- 1: hide_input_in_errors does not work in all cases pydantic/pydantic#7461
- 2: Pydantic "hide_input_in_errors" does not hide "input" field when request validation fails on model fastapi/fastapi#10025
- 3: https://pydantic.dev/docs/validation/latest/api/pydantic/config/
- 4: https://pydantic.dev/docs/validation/2.0/usage/model_config/
- 5: https://github.com/pydantic/pydantic/blob/08b64f7a/tests/test_config.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate pydantic_models.py =="
fd -a 'pydantic_models.py' . || true
echo "== Inspect target file excerpt =="
if [ -f tavern/_core/pydantic_models.py ]; then
wc -l tavern/_core/pydantic_models.py
sed -n '1,120p' tavern/_core/pydantic_models.py | cat -n
fi
echo "== Search pydantic version references =="
rg -n "pydantic|hide_input_in_errors|validate_keys|UnexpectedKeysError" -S pyproject.toml uv.lock poetry.lock requirements*.txt setup.py setup.cfg tavern || true
echo "== Inspect test files that may exercise pydantic errors =="
rg -n "UnexpectedKeysError|validate_keys|hide_input_in_errors|model_config|MQTTAuthArgs|RestRequestSpec|MQTTTLSArgs" -S tests tavern || true
echo "== Python pydantic availability and ValidationError string behaviour probe =="
python3 - <<'PY'
import sys
try:
import pydantic
from pydantic import BaseModel, ValidationError, ConfigDict
except Exception as e:
print(f"pydantic unavailable: {type(e).__name__}: {e}")
sys.exit(0)
print(f"pydantic={pydantic.__version__}")
print("python=", sys.version.split()[0])
class WithHide(BaseModel):
model_config = ConfigDict(extra="forbid", arbitrary_types_allowed=True, populate_by_name=True, hide_input_in_errors=True)
password: str
class WithoutHide(BaseModel):
model_config = ConfigDict(extra="forbid", arbitrary_types_allowed=True, populate_by_name=True)
password: str
for cls, name in [(WithoutHide, "without hide"), (WithHide, "with hide")]:
try:
cls(password=12345)
except ValidationError as e:
print(f"\n--- {name}: str ---\n{str(e)}")
print(f"--- {name}: errors include input? ---")
for err in e.errors():
print({k: err[k] for k in sorted(err)})
PYRepository: taverntesting/tavern
Length of output: 50376
Sanitise validation error messages before raising UnexpectedKeysError.
On type-validation failures, this path uses msg = str(e) directly, so Pydantic can include the raw offending value in the exception text. If that value is sensitive, such as an unquoted numeric MQTT password or TLS password, it may be displayed in test/CI output. Enable Pydantic input hiding in ConfigDict and/or build the custom message without input values.
🔒 Suggested fix
model_config = ConfigDict(
- extra="forbid", arbitrary_types_allowed=True, populate_by_name=True
+ extra="forbid",
+ arbitrary_types_allowed=True,
+ populate_by_name=True,
+ hide_input_in_errors=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class _BaseKeyValidator(BaseModel): | |
| """Base model that forbids extra keys and raises UnexpectedKeysError on validation failure.""" | |
| model_config = ConfigDict( | |
| extra="forbid", arbitrary_types_allowed=True, populate_by_name=True | |
| ) | |
| @classmethod | |
| def validate_keys(cls, data: Mapping) -> dict: | |
| """Validate that ``data`` contains only expected keys and types. | |
| Args: | |
| data: Dictionary to validate against this model's fields. | |
| Returns: | |
| The validated data as a dict. | |
| Raises: | |
| exceptions.UnexpectedKeysError: If unexpected keys are present or | |
| a value has an invalid type. | |
| """ | |
| try: | |
| return cls(**dict(data)).model_dump(exclude_unset=True, by_alias=True) | |
| except ValidationError as e: | |
| # Extract unexpected field names from the error | |
| unexpected = set() | |
| for err in e.errors(): | |
| if err["type"] == "extra_forbidden": | |
| unexpected.add(err["loc"][-1]) | |
| if unexpected: | |
| msg = f"Unexpected keys {unexpected}" | |
| else: | |
| msg = str(e) | |
| raise exceptions.UnexpectedKeysError(msg) from e | |
| class _BaseKeyValidator(BaseModel): | |
| """Base model that forbids extra keys and raises UnexpectedKeysError on validation failure.""" | |
| model_config = ConfigDict( | |
| extra="forbid", arbitrary_types_allowed=True, populate_by_name=True, hide_input_in_errors=True | |
| ) | |
| `@classmethod` | |
| def validate_keys(cls, data: Mapping) -> dict: | |
| """Validate that ``data`` contains only expected keys and types. | |
| Args: | |
| data: Dictionary to validate against this model's fields. | |
| Returns: | |
| The validated data as a dict. | |
| Raises: | |
| exceptions.UnexpectedKeysError: If unexpected keys are present or | |
| a value has an invalid type. | |
| """ | |
| try: | |
| return cls(**dict(data)).model_dump(exclude_unset=True, by_alias=True) | |
| except ValidationError as e: | |
| # Extract unexpected field names from the error | |
| unexpected = set() | |
| for err in e.errors(): | |
| if err["type"] == "extra_forbidden": | |
| unexpected.add(err["loc"][-1]) | |
| if unexpected: | |
| msg = f"Unexpected keys {unexpected}" | |
| else: | |
| msg = str(e) | |
| raise exceptions.UnexpectedKeysError(msg) from e |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tavern/_core/pydantic_models.py` around lines 19 - 52, Sanitize
type-validation errors in _BaseKeyValidator.validate_keys so UnexpectedKeysError
never exposes raw input values. Enable Pydantic’s input-repr hiding option in
model_config and replace the direct str(e) fallback with a safe validation-error
summary that retains field and type information without including offending
values; preserve the existing unexpected-key handling.
Comment on lines
+300
to
+309
| def test_grpc_request_body_can_be_dict(self): | ||
| data = {"host": "localhost:50051", "service": "MyService/Method", "body": {"key": "value"}} | ||
| result = GRPCRequestSpec.validate_keys(data) | ||
| assert result["body"] == {"key": "value"} | ||
|
|
||
| def test_grpc_request_body_can_be_string(self): | ||
| data = {"host": "localhost:50051", "service": "MyService/Method", "body": "raw string"} | ||
| result = GRPCRequestSpec.validate_keys(data) | ||
| assert result["body"] == "raw string" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
CI-failing formatting: ruff-format reformatting required.
Pipeline logs report ruff-format reformats these two dict literals (line length). Apply the formatter's expected multi-line layout.
🎨 Proposed formatting fix
def test_grpc_request_body_can_be_dict(self):
- data = {"host": "localhost:50051", "service": "MyService/Method", "body": {"key": "value"}}
+ data = {
+ "host": "localhost:50051",
+ "service": "MyService/Method",
+ "body": {"key": "value"},
+ }
result = GRPCRequestSpec.validate_keys(data)
assert result["body"] == {"key": "value"}
def test_grpc_request_body_can_be_string(self):
- data = {"host": "localhost:50051", "service": "MyService/Method", "body": "raw string"}
+ data = {
+ "host": "localhost:50051",
+ "service": "MyService/Method",
+ "body": "raw string",
+ }
result = GRPCRequestSpec.validate_keys(data)
assert result["body"] == "raw string"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_grpc_request_body_can_be_dict(self): | |
| data = {"host": "localhost:50051", "service": "MyService/Method", "body": {"key": "value"}} | |
| result = GRPCRequestSpec.validate_keys(data) | |
| assert result["body"] == {"key": "value"} | |
| def test_grpc_request_body_can_be_string(self): | |
| data = {"host": "localhost:50051", "service": "MyService/Method", "body": "raw string"} | |
| result = GRPCRequestSpec.validate_keys(data) | |
| assert result["body"] == "raw string" | |
| def test_grpc_request_body_can_be_dict(self): | |
| data = { | |
| "host": "localhost:50051", | |
| "service": "MyService/Method", | |
| "body": {"key": "value"}, | |
| } | |
| result = GRPCRequestSpec.validate_keys(data) | |
| assert result["body"] == {"key": "value"} | |
| def test_grpc_request_body_can_be_string(self): | |
| data = { | |
| "host": "localhost:50051", | |
| "service": "MyService/Method", | |
| "body": "raw string", | |
| } | |
| result = GRPCRequestSpec.validate_keys(data) | |
| assert result["body"] == "raw string" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_pydantic_models.py` around lines 300 - 309, Reformat the data
dictionary literals in test_grpc_request_body_can_be_dict and
test_grpc_request_body_can_be_string using ruff-format’s multi-line layout so
the tests pass formatting checks; do not change their behavior or assertions.
Source: Pipeline failures
TypeConvertToken objects (from !force_format_include, !int, etc.) and dict values (from $ext function calls) exist at validation time before resolution. Add these to Union types so pydantic accepts them while still rejecting clearly wrong types (e.g. int for headers, list for method).
Integration tests use cookies as a list (e.g. cookie name lists, cookie override dicts in list form, empty list to send no cookies).
Contributor
Author
|
@michaelboulton, as per your suggestion in PR 1065, I have updated the code to include type checking. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the 9-year-old
check_expected_keyspattern with pydantic models that useextra='forbid'for key validation and proper type annotations for value validation.This replaces #1065 by addressing the review feedback: all fields now have specific types instead of
Optional[Any], so pydantic actually validates both keys and values — otherwise using dataclasses/dacite would be just as effective.Changes
tavern/_core/pydantic_models.py— typedBaseModelsubclasses for REST, MQTT, and gRPC request/response/client specsvalidate_keys()instead ofcheck_expected_keys()tests/unit/test_pydantic_models.py— tests for key validation + type enforcementpyproject.toml— pydantic as runtime dependencyType annotations
RestRequestSpecmethod: str,headers: dict,stream: bool,json: JSONType,timeout: Union[float, list]MQTTRequestSpectopic: str,qos: int,retain: bool,payload: Union[str, bytes, int, float]MQTTConnectArgshost: str,port: int,keepalive: intGRPCResponseSpecstatus: Union[str, int, list[str], list[int]],body: dictGRPCClientTopLevelattempt_reflection: bool,connect: dict,metadata: dictCloses
Closes #1034
Summary by CodeRabbit
New Features
Tests