feat: add model validation for types #708
Conversation
mvadari
force-pushed
the
model-type-validation
branch
from
cd1e0d0 to
bc3e043
Compare
mvadari
force-pushed
the
model-type-validation
branch
from
bc3e043 to
310363e
Compare
|
thanks, this is very interesting work. I feel we could have used this library to enforce type checks, had it been better maintained: https://pypi.org/project/enforce-typing/ |
![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
🧹 Outside diff range and nitpick comments (1)
xrpl/models/requests/ledger_entry.py (1)
260-262: LGTM! Consider documenting the type expansion in the field's docstring.The addition of
inttype support is consistent withxchain_claim_idand improves API uniformity. Consider updating the docstring to explicitly mention that both string and integer values are accepted.Add type information to the docstring:
xchain_create_account_claim_id: Optional[ Union[int, str, XChainCreateAccountClaimID] ] = None + """ + The XChainCreateAccountClaimID associated with a cross-chain account create. + Accepts both string and integer values. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)xrpl/models/requests/ledger_entry.py(1 hunks)
🔇 Additional comments (1)
xrpl/models/requests/ledger_entry.py (1)
259-259: LGTM! Type expansion for xchain_claim_id is well-justified.
The addition of int type support maintains backward compatibility while improving developer experience by accepting native integer values, consistent with similar fields like oracle_document_id.
Let's verify the consistency of integer type support across the codebase:
✅ Verification successful
Type expansion for xchain_claim_id is consistent with codebase patterns
The addition of int type to xchain_claim_id is verified to be consistent with the codebase:
- All XChain transaction types (
XChainCommit,XChainClaim,XChainAddClaimAttestation) already supportUnion[int, str]for theirxchain_claim_idfields - The
XChainClaimIDclass is properly referenced in the type union, maintaining compatibility with object-based usage - Test fixtures show both string ("b0", "af") and numeric (1) claim IDs being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other XChain-related claim ID fields to ensure consistent type handling
ast-grep --pattern 'xchain_claim_id: Union[$_]'
Length of output: 49
Script:
#!/bin/bash
# Let's try a broader search for xchain_claim_id fields and their type annotations
rg "xchain_claim_id.*:" -A 1
# Also search for XChainClaimID to understand its definition and usage
rg "XChainClaimID" -A 2
Length of output: 7766
CHANGELOG.md
Outdated
| ### Added | ||
| - Support for the DeliverMax field in Payment transactions | ||
| - Support for the `feature` RPC | ||
| - Improved validation for models to also check param types |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move entry to "Unreleased" section and enhance description.
The changelog entry for model validation should be moved to the "Unreleased" section since this is a new feature that hasn't been released yet. Additionally, consider expanding the description to provide more context about the types of validation being added.
Apply this diff to move and enhance the entry:
-### Added
-- Improved validation for models to also check param types
## [[Unreleased]]
### Added
+ - Added type validation for model parameters to catch type mismatches early and improve error reporting📝 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.
| - Improved validation for models to also check param types | |
| ### Added | |
| ## [[Unreleased]] | |
| ### Added | |
| - Added type validation for model parameters to catch type mismatches early and improve error reporting |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
xrpl/models/base_model.py (1)
295-354: LGTM! Consider enhancing error messages for better debugging.The type checking implementation is thorough and handles all major type scenarios correctly. However, the error messages could be more descriptive for better debugging.
Consider enhancing error messages to include the actual value in addition to its type:
- return {attr: f"{attr} is {type(value)}, expected {expected_type}"} + return {attr: f"{attr} is {value} of type {type(value)}, expected {expected_type}"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)xrpl/models/base_model.py(2 hunks)xrpl/models/transactions/oracle_set.py(2 hunks)
🔇 Additional comments (5)
xrpl/models/base_model.py (2)
363-370: LGTM! Comprehensive validation implementation.
The updated _get_errors method now provides both required field validation and thorough type checking using the new _check_type method.
407-409: LGTM! Clean and readable implementation.
The updated __repr__ method uses a clean list comprehension approach for building the string representation.
xrpl/models/transactions/oracle_set.py (1)
23-26: LGTM! Ensures consistent integer type for epoch offset.
Using int() for the EPOCH_OFFSET calculation ensures consistent typing and aligns with the improved type validation.
CHANGELOG.md (2)
Line range hint 1-24: LGTM! The changelog format is correct.
The file correctly follows the Keep a Changelog format and Semantic Versioning guidelines. The "Unreleased" section is properly structured with appropriate subsections for different types of changes.
27-27: 🛠️ Refactor suggestion
Move and enhance the model validation entry.
The changelog entry needs to be moved to the "Unreleased" section since this is a new feature that hasn't been released yet. Additionally, the description should be enhanced to better reflect the PR's purpose of adding type validation for model parameters.
Apply this diff to move and enhance the entry:
-### Added
-- Improved validation for models to also check param types
## [[Unreleased]]
### Added
+ - Added type validation for model parameters to catch type mismatches early and improve error reporting
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
xrpl/models/base_model.py (1)
312-312: Remove unclear comment or resolve the mypy issue.The comment
# unsure what the problem with mypy is hereindicates uncertainty and may confuse future maintainers. Consider resolving the mypy issue or removing the comment before merging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
xrpl/models/base_model.py(2 hunks)
🔇 Additional comments (2)
xrpl/models/base_model.py (2)
323-323: Duplicate comment regarding list type handling in _check_type.
This line has been previously discussed and addressed in past review comments. The handling of list types in the type checking function seems appropriate.
351-352: Verify that isinstance correctly handles all expected types.
The check isinstance(value, expected_type) may not properly handle complex types like generics or type aliases. Ensure that all expected types are correctly validated, and consider augmenting the type checking logic if necessary.
Run the following script to test how isinstance behaves with various expected types:
✅ Verification successful
Type checking implementation is robust and handles complex types correctly
Based on the codebase analysis, the _check_type method in base_model.py has comprehensive type checking that properly handles:
- Union types through
get_originandget_args - Lists with generic type parameters
- Dictionaries
- Enum types
- Literal types
- BaseModel subclasses
- Any type
- Basic Python types
The implementation goes beyond simple isinstance checks and includes proper handling of generic types from the typing module. The type checking is thorough and covers all expected use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test isinstance with different expected types.
python3 - << EOF
from typing import List, Union, Dict, Any
def test_isinstance():
test_cases = [
(1, int),
("string", str),
([1, 2, 3], list),
([1, 2, 3], List[int]),
({"key": "value"}, dict),
({"key": "value"}, Dict[str, str]),
(None, type(None)),
(None, Union[str, None]),
]
for value, expected_type in test_cases:
try:
result = isinstance(value, expected_type)
except TypeError as e:
result = f"TypeError: {e}"
print(f"isinstance({value!r}, {expected_type}) -> {result}")
test_isinstance()
EOF
Length of output: 109
Script:
#!/bin/bash
# Let's analyze the codebase to understand how expected_type is used
# First, let's find where expected_type is defined or used
rg "expected_type" -A 3 -B 3
# Then, let's look for type hints and validation patterns
ast-grep --pattern 'def validate_$_($$$) -> $_: $$$'
# Also search for type-related imports
rg "^from typing import"
Length of output: 15075
|
|
||
| def __repr__(self: Self) -> str: | ||
| """Returns a string representation of a BaseModel object""" | ||
| """Returns a string representation of a BaseModel object.""" |
There was a problem hiding this comment.
Add missing __repr__ method definition to prevent syntax errors.
The docstring on line 407 is not associated with any function, suggesting that the __repr__ method definition is missing. This will cause a syntax error.
Apply this diff to define the __repr__ method correctly:
+ def __repr__(self: Self) -> str:
"""Returns a string representation of a BaseModel object."""📝 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.
| """Returns a string representation of a BaseModel object.""" | |
| def __repr__(self: Self) -> str: | |
| """Returns a string representation of a BaseModel object.""" |
| if issubclass(expected_type, BaseModel) and isinstance(value, dict): | ||
| return {} | ||
|
|
There was a problem hiding this comment.
Ensure proper validation of BaseModel subclasses when value is a dict.
In the condition if issubclass(expected_type, BaseModel) and isinstance(value, dict):, the method returns {} without validating the contents of value. This may allow invalid data to pass through without proper checks. Consider attempting to instantiate expected_type with value to ensure it is valid.
Apply this diff to perform validation:
if issubclass(expected_type, BaseModel) and isinstance(value, dict):
- return {}
+ try:
+ expected_type.from_dict(value)
+ return {}
+ except XRPLModelException as e:
+ return {attr: f"{attr} is invalid: {e}"}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
xrpl/models/transactions/transaction.py (1)
Line range hint
371-385: LGTM! Consider adding type hints for better clarity.The error handling for invalid flag types is a good addition that aligns with the PR's type validation objectives.
Consider adding a type hint for
self.flagsin the error message for better clarity:- raise XRPLModelException("self.flags is not an int, dict, or list") + raise XRPLModelException(f"self.flags is {type(self.flags)}, expected int, dict, or list")xrpl/models/base_model.py (1)
301-303: Add return type annotation for better type safety.The method is missing a return type annotation which would improve type safety and documentation.
- def _check_type( - self: Self, attr: str, value: Any, expected_type: Type[Any] # noqa: ANN401 - ) -> Dict[str, str]: + def _check_type( + self: Self, attr: str, value: Any, expected_type: Type[Any] # noqa: ANN401 + ) -> Dict[str, str]:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)xrpl/models/base_model.py(2 hunks)xrpl/models/requests/ledger_entry.py(1 hunks)xrpl/models/transactions/transaction.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- xrpl/models/requests/ledger_entry.py
- CHANGELOG.md
🔇 Additional comments (4)
xrpl/models/transactions/transaction.py (2)
Line range hint 419-422: LGTM! Clear error handling for unsigned transactions.
The error handling for unsigned transactions is well-implemented and provides a clear error message.
Line range hint 509-524: LGTM! Consider verifying Payment transaction handling.
The validation for Payment transactions with both amount and deliver_max fields is well-implemented.
Let's verify the Payment transaction handling:
✅ Verification successful
Validation for Payment transactions with amount and deliver_max is properly implemented and tested
The verification shows:
- The implementation correctly validates Payment transactions with both
amountanddeliver_maxfields - Unit tests cover different scenarios:
test_payment_txn_API_no_deliver_maxtest_payment_txn_API_different_amount_and_deliver_maxtest_payment_txn_API_identical_amount_and_deliver_max
- Integration tests verify the
deliver_maxalias field functionality - The code properly handles field aliasing by setting
amount = deliver_maxand removing thedeliver_maxfield before serialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Payment transactions using both amount and deliver_max fields
# Search for test files containing Payment transactions
echo "Searching for test files with Payment transactions..."
rg -l "Payment.*deliver_max" --type py
# Search for actual usage in non-test files
echo "Searching for actual usage in non-test files..."
rg -l "Payment.*deliver_max" --type py -g '!test*'
Length of output: 315
Script:
#!/bin/bash
# Let's try a different approach to verify Payment transaction handling
# Search for Payment transaction tests with broader context
echo "Searching for Payment transaction tests..."
rg "class.*Payment.*Test" -A 10 --type py
# Search for deliver_max field usage
echo "Searching for deliver_max field usage..."
rg "deliver_max" -B 2 -A 2 --type py
# Search for amount field in Payment transactions
echo "Searching for amount field in Payment context..."
rg "amount.*Payment|Payment.*amount" -B 2 -A 2 --type py
Length of output: 10330
xrpl/models/base_model.py (2)
369-376: LGTM! The error collection logic is well-implemented.
The method correctly:
- Handles required field validation
- Performs type validation for all fields
- Accumulates errors in a single dictionary
354-356: 🛠️ Refactor suggestion
Enhance BaseModel validation to verify dict contents.
The current implementation only checks if the value is a dict without validating that it can be converted to the expected BaseModel type.
This issue was previously identified in a past review comment. Apply this diff to validate the dict contents:
- if issubclass(expected_type, BaseModel) and isinstance(value, dict):
- return {}
+ if issubclass(expected_type, BaseModel) and isinstance(value, dict):
+ try:
+ expected_type.from_dict(value)
+ return {}
+ except XRPLModelException as e:
+ return {attr: f"{attr} is invalid: {e}"}| if expected_type_origin is dict: | ||
| return ( | ||
| {} | ||
| if isinstance(value, dict) | ||
| else {attr: f"{attr} is {type(value)}, expected {expected_type}"} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance dict validation to check key and value types.
The current implementation only verifies that the value is a dict without validating its contents against the expected key and value types.
- if expected_type_origin is dict:
- return (
- {}
- if isinstance(value, dict)
- else {attr: f"{attr} is {type(value)}, expected {expected_type}"}
- )
+ if expected_type_origin is dict:
+ if not isinstance(value, dict):
+ return {attr: f"{attr} is {type(value)}, expected {expected_type}"}
+ key_type, value_type = get_args(expected_type)
+ result = {}
+ for k, v in value.items():
+ result.update(self._check_type(f"{attr}[{k}].key", k, key_type))
+ result.update(self._check_type(f"{attr}[{k}].value", v, value_type))
+ return result📝 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.
| if expected_type_origin is dict: | |
| return ( | |
| {} | |
| if isinstance(value, dict) | |
| else {attr: f"{attr} is {type(value)}, expected {expected_type}"} | |
| ) | |
| if expected_type_origin is dict: | |
| if not isinstance(value, dict): | |
| return {attr: f"{attr} is {type(value)}, expected {expected_type}"} | |
| key_type, value_type = get_args(expected_type) | |
| result = {} | |
| for k, v in value.items(): | |
| result.update(self._check_type(f"{attr}[{k}].key", k, key_type)) | |
| result.update(self._check_type(f"{attr}[{k}].value", v, value_type)) | |
| return result |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
xrpl/models/transactions/transaction.py (1)
416-419: LGTM! Added validation for unsigned transactions.The added check prevents invalid hash generation for unsigned transactions with a clear error message.
Consider making the error message more specific:
- "Cannot get the hash from an unsigned Transaction." + "Cannot get the hash from an unsigned Transaction. Either txn_signature or signers must be present."tests/unit/models/test_base_model.py (2)
87-95: Improve test clarity by isolating type validation per field.The test would be clearer if it validated one field at a time, making it easier to identify which field's type validation failed.
Apply this diff to split the test into focused test methods:
-def test_bad_type(self): - transaction_dict = { - "account": 1, - "amount": 10, - "destination": 1, - } - with self.assertRaises(XRPLModelException): - Payment(**transaction_dict) +def test_bad_type_account(self): + transaction_dict = { + "account": 1, # should be str + "amount": "10", + "destination": destination, + } + with self.assertRaises(XRPLModelException): + Payment(**transaction_dict) + +def test_bad_type_amount(self): + transaction_dict = { + "account": account, + "amount": 10, # should be str or dict + "destination": destination, + } + with self.assertRaises(XRPLModelException): + Payment(**transaction_dict) + +def test_bad_type_destination(self): + transaction_dict = { + "account": account, + "amount": "10", + "destination": 1, # should be str + } + with self.assertRaises(XRPLModelException): + Payment(**transaction_dict)
453-453: Add a comment explaining the literal constraint.Add a comment to clarify that
was_locking_chain_sendis a boolean-like field that only accepts 0 or 1 as valid values.Apply this diff to add the clarifying comment:
- "was_locking_chain_send": 2, # supposed to be 0 or 1 + "was_locking_chain_send": 2, # Invalid value: was_locking_chain_send is a boolean-like field that only accepts 0 or 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)tests/unit/models/test_base_model.py(18 hunks)xrpl/models/requests/ledger_entry.py(1 hunks)xrpl/models/transactions/transaction.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- xrpl/models/requests/ledger_entry.py
🧰 Additional context used
🧠 Learnings (1)
tests/unit/models/test_base_model.py (1)
Learnt from: mvadari
PR: XRPLF/xrpl-py#708
File: tests/unit/models/test_base_model.py:448-448
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In `tests/unit/models/test_base_model.py`, the `public_key` values are sample strings used as placeholders and do not contain actual cryptographic keys.
🔇 Additional comments (4)
xrpl/models/transactions/transaction.py (2)
371-374: LGTM! Documentation update is clear and helpful.The added documentation for the error case in
has_flagmethod is well-written and follows the standard format.
382-385: LGTM! Enhanced error handling for invalid flag types.The added error handling improves type validation by explicitly handling all possible flag types and providing a clear error message.
tests/unit/models/test_base_model.py (2)
120-120: LGTM! Improved test method naming.The removal of the "from_dict" prefix makes the test method names more concise while maintaining clarity about their purpose.
Also applies to: 124-124, 135-135, 155-155, 171-171, 187-187, 218-218, 242-242, 262-262, 271-271, 286-286, 309-309, 322-322, 339-339, 411-411
820-862: LGTM! Well-documented case format validation tests.The tests clearly validate the case format requirements for XRPL inputs with good documentation and clear test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/test_base_model.py (1)
480-502: Good test for literal type validationThe test correctly verifies that the was_locking_chain_send field rejects invalid literal values (2 instead of the valid 0 or 1).
Consider adding a comment to clearly indicate that
was_locking_chain_send: 2is the bad literal value being tested, as suggested in the past review comments.- "was_locking_chain_send": 2, # supposed to be 0 or 1 + "was_locking_chain_send": 2, # This is the bad literal value - should be 0 or 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)tests/unit/models/test_base_model.py(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (1)
tests/unit/models/test_base_model.py (1)
Learnt from: mvadari
PR: XRPLF/xrpl-py#708
File: tests/unit/models/test_base_model.py:448-448
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In `tests/unit/models/test_base_model.py`, the `public_key` values are sample strings used as placeholders and do not contain actual cryptographic keys.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (10)
tests/unit/models/test_base_model.py (10)
87-111: Good implementation of comprehensive type validation testThis test effectively validates that proper type checking is performed for critical fields in the Payment model, ensuring integers aren't accepted for string fields or specialized types.
112-136: Well-structured test with proper error validationThe test correctly verifies that providing a string where an integer is expected raises an appropriate exception with detailed error information. The test also captures multiple validation errors in a single test case.
137-157: Great enum validation testThis test effectively confirms that invalid enum values are properly rejected with clear error messages indicating the expected enum type.
162-166: Good cleanup: More concise test method namesThe method naming is more concise without the redundant "from_dict" prefix, while maintaining clarity about what's being tested.
Also applies to: 166-176, 177-196, 197-212, 213-228, 229-259, 260-283, 284-303, 304-312, 313-327, 328-350, 351-363, 364-380, 381-452, 453-465
466-479: Good validation for nonexistent fieldsThis test appropriately verifies that an exception is raised when a field that doesn't exist in the model is provided.
503-531: Comprehensive positive test for literal valuesThis test provides good coverage for the valid case of literal types, ensuring that the model accepts valid literal values.
532-540: Effective enum string value testThis test confirms that string values that correspond to enum values are properly accepted and converted.
552-563: Good coverage with use_json parameter variationsThese test cases effectively exercise different code paths in the from_xrpl method by using both object and string inputs, improving code coverage.
The use_json=True test uses json.dumps to test a different code path in Transaction.from_xrpl, which is good for coverage.
862-885: Good validation of input format requirementsThis test effectively verifies that the Request.from_xrpl method properly validates input format, rejecting snake_case keys in favor of camelCase or PascalCase.
886-904: Good validation of Transaction input formatThis test ensures that Transaction.from_xrpl properly rejects snake_case JSON keys, maintaining consistent format validation.
* add type validation * add tests, fix tests * improve test coverage of BaseModel, fix issues * split out from_xrpl tests from from_dict * update changelog * fix tests * fix snippet * respond to comments * fix tests * fix test * add comment * fix merge issue * fix mypy * fix comment * fix other syntax issues * fix issues * fix linter * fix changelog * add message verification
3 participants
High Level Overview of Change
This PR adds additional validation for models to also check parameter types and ensure they match what is expected. It also fixes issues that were uncovered via adding this additional checking.
Context of Change
It's a common annoyance and improves the user experience.
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added tests, and added a few additional tests to improve code coverage in the
base_model.pyfile. CI passes.