feat: replace check_expected_keys with pydantic models (#1034)#1065
Open
reachsridhard wants to merge 6 commits into
Open
feat: replace check_expected_keys with pydantic models (#1034)#1065reachsridhard wants to merge 6 commits into
reachsridhard wants to merge 6 commits into
Conversation
…#1034) Replace the 9-year-old check_expected_keys pattern with pydantic models that use extra='forbid' for key validation. This provides the same behavior (rejecting unexpected keys) with better error messages and type safety. Changes: - Add tavern/_core/pydantic_models.py with pydantic models for each request/client spec (REST, MQTT, gRPC) - Replace all check_expected_keys calls with .validate_keys() calls in rest/request.py, mqtt/request.py, mqtt/client.py, grpc/request.py, grpc/response.py, grpc/client.py - Remove now-unused expected_blocks dicts from MQTT and gRPC clients - Add tests/unit/test_pydantic_models.py with 27 tests covering all new models - Keep check_expected_keys in dict_util.py for backward compatibility Closes taverntesting#1034
📝 WalkthroughB[Pydantic model construction] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/test_pydantic_models.py (2)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the offending key in the failure cases.
A core goal of this change is better unexpected-key diagnostics, but these tests only check the exception type. If the implementation regressed to a generic
UnexpectedKeysError, this suite would still pass.Suggested assertion pattern
def test_unexpected_key(self): data = {"method": "GET", "url": "http://example.com", "bad_key": "value"} - with pytest.raises(exceptions.UnexpectedKeysError): + with pytest.raises(exceptions.UnexpectedKeysError) as exc_info: RestRequestSpec.validate_keys(data) + assert "bad_key" in str(exc_info.value)Also applies to: 47-50, 59-62, 69-72, 79-82, 89-92, 99-102, 109-112, 121-124, 131-134, 141-144, 151-154, 161-164
🤖 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 31 - 34, The unexpected-key tests only assert the exception type, so they won’t catch regressions in the actual offending-key diagnostics. Update the affected cases in test_pydantic_models.py to assert the specific bad key is present in the UnexpectedKeysError message or payload when calling RestRequestSpec.validate_keys and the other similar validation paths. Use the existing test methods (for example, test_unexpected_key and the other listed cases) to verify the exception includes the exact unexpected field name, not just that an error was raised.
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the happy-path assertions.
Most of these tests only prove that one field survives validation. They would still pass if
validate_keys()dropped sibling keys or changed the returned shape, even though callers consume the full validated dict. Comparing against the full expected payload would lock down the round-trip contract much better.Suggested test shape
def test_valid_keys(self): data = {"method": "GET", "url": "http://example.com", "json": {"a": 1}} result = RestRequestSpec.validate_keys(data) - assert "method" in result - assert "url" in result - assert "json" in result + assert result == dataAlso applies to: 42-45, 54-57, 64-67, 74-77, 84-87, 94-97, 104-107, 116-119, 126-129, 136-139, 146-149, 156-159
🤖 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 24 - 29, The happy-path tests for RestRequestSpec.validate_keys are too loose because they only assert individual keys exist, so tighten them to compare the returned dict against the full expected payload. Update test_valid_keys and the other similar cases in test_pydantic_models to verify the complete validated structure preserves all fields and values, not just one surviving key.
🤖 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 107-114: The MQTT SSL-context args model currently blocks a key
that MQTTClient expects to read for certificate verification. Update
MQTTSSLContextArgs so cert_reqs is accepted alongside the existing SSL fields,
and make sure the validation inherited from _BaseKeyValidator allows that key to
pass through to MQTTClient’s SSL context handling.
---
Nitpick comments:
In `@tests/unit/test_pydantic_models.py`:
- Around line 31-34: The unexpected-key tests only assert the exception type, so
they won’t catch regressions in the actual offending-key diagnostics. Update the
affected cases in test_pydantic_models.py to assert the specific bad key is
present in the UnexpectedKeysError message or payload when calling
RestRequestSpec.validate_keys and the other similar validation paths. Use the
existing test methods (for example, test_unexpected_key and the other listed
cases) to verify the exception includes the exact unexpected field name, not
just that an error was raised.
- Around line 24-29: The happy-path tests for RestRequestSpec.validate_keys are
too loose because they only assert individual keys exist, so tighten them to
compare the returned dict against the full expected payload. Update
test_valid_keys and the other similar cases in test_pydantic_models to verify
the complete validated structure preserves all fields and values, not just one
surviving key.
🪄 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
Run ID: 4b6ffa7c-de81-4e50-8e2c-1019e7c776b6
📒 Files selected for processing (8)
tavern/_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
+107
to
+114
| class MQTTSSLContextArgs(_BaseKeyValidator): | ||
| ca_certs: Optional[Any] = None | ||
| certfile: Optional[Any] = None | ||
| keyfile: Optional[Any] = None | ||
| password: Optional[Any] = None | ||
| tls_version: Optional[Any] = None | ||
| ciphers: Optional[Any] = None | ||
| alpn_protocols: Optional[Any] = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow cert_reqs in SSL-context config.
MQTTClient reads self._ssl_context_args.get("cert_reqs") to set verify_mode and tls_insecure_set, but this model forbids that key, so users cannot reach that path.
🐛 Proposed fix
class MQTTSSLContextArgs(_BaseKeyValidator):
ca_certs: Optional[Any] = None
+ cert_reqs: Optional[Any] = None
certfile: Optional[Any] = None📝 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 MQTTSSLContextArgs(_BaseKeyValidator): | |
| ca_certs: Optional[Any] = None | |
| certfile: Optional[Any] = None | |
| keyfile: Optional[Any] = None | |
| password: Optional[Any] = None | |
| tls_version: Optional[Any] = None | |
| ciphers: Optional[Any] = None | |
| alpn_protocols: Optional[Any] = None | |
| class MQTTSSLContextArgs(_BaseKeyValidator): | |
| ca_certs: Optional[Any] = None | |
| cert_reqs: Optional[Any] = None | |
| certfile: Optional[Any] = None | |
| keyfile: Optional[Any] = None | |
| password: Optional[Any] = None | |
| tls_version: Optional[Any] = None | |
| ciphers: Optional[Any] = None | |
| alpn_protocols: Optional[Any] = None |
🤖 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 107 - 114, The MQTT SSL-context
args model currently blocks a key that MQTTClient expects to read for
certificate verification. Update MQTTSSLContextArgs so cert_reqs is accepted
alongside the existing SSL fields, and make sure the validation inherited from
_BaseKeyValidator allows that key to pass through to MQTTClient’s SSL context
handling.
grpc/response.py passes _GRPCExpected | Mapping to validate_keys, which mypy rejects when the parameter is typed as dict. Changed to Mapping and added dict() conversion before ** unpacking.
pydantic_models.py is imported by core plugin modules (rest, mqtt, grpc), so pydantic must be a runtime dependency, not just a dev dependency. Moved from optional-dependencies to dependencies.
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.
Replace the 9-year-old check_expected_keys pattern with pydantic models that use extra='forbid' for key validation. This provides the same behavior (rejecting unexpected keys) with better error messages and type safety.
Changes:
Closes #1034
Summary by CodeRabbit
New Features
Bug Fixes
Tests