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 full review |
Actions performedFull review triggered. |
|
Warning Rate limit exceeded@mvadari has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe recent changes to the project include updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai full review |
Actions performedFull review triggered. |
![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
Outside diff range and nitpick comments (2)
xrpl/models/base_model.py (2)
294-351: Consider adding a docstring to the_check_typemethod for clarity.Adding a docstring to
_check_typewould improve code readability and maintainability by documenting its purpose, parameters, and return value.
307-307: Clarify or remove the comment regarding the mypy issue.The comment
# unsure what the problem with mypy is heremay not be helpful. If there's a mypy warning, consider either resolving it or providing a more detailed explanation to assist future maintainers.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- snippets/paths.py (1 hunks)
- tests/unit/models/test_base_model.py (18 hunks)
- tests/unit/models/transactions/test_check_cash.py (1 hunks)
- tests/unit/models/transactions/test_oracle_set.py (1 hunks)
- tests/unit/models/transactions/test_xchain_claim.py (1 hunks)
- xrpl/models/base_model.py (2 hunks)
- xrpl/models/requests/ledger_entry.py (1 hunks)
- xrpl/models/transactions/oracle_set.py (2 hunks)
- xrpl/models/transactions/pseudo_transactions/enable_amendment.py (2 hunks)
- xrpl/models/transactions/transaction.py (2 hunks)
- xrpl/utils/xrp_conversions.py (3 hunks)
Additional context used
Gitleaks
tests/unit/models/test_base_model.py
448-448: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
471-471: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (22)
tests/unit/models/transactions/test_check_cash.py (1)
9-9: Verify and update other occurrences of_CHECK_IDin the codebase.The change to the
_CHECK_IDconstant, moving from a numeric format to a hexadecimal string format, appears to be valid and aligns with the provided summary.However, please ensure that any other occurrences of
_CHECK_IDin the test files or the main codebase are updated to match this new format for consistency.Run the following script to verify the usage of
_CHECK_ID:Verification successful
Verification complete:
_CHECK_IDformat change is consistentThe change to the
_CHECK_IDconstant, moving from a numeric format to a hexadecimal string format, has been consistently applied across the codebase. The new format is defined once and used in multiple locations without any instances of the old format remaining.
- Definition:
_CHECK_ID = "838766BA2B995C00744175F69A1B11E32C3DBC40E64801A4056FCBD657F57334"- Usage: Found in 4 other locations, likely in function calls or assignments
No further action is required regarding this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `_CHECK_ID` match the new hexadecimal string format. # Test: Search for the `_CHECK_ID` usage. Expect: Only occurrences of the new format. rg --type python -A 5 $'_CHECK_ID'Length of output: 64
Script:
#!/bin/bash # Description: Verify all occurrences of `_CHECK_ID` in Python files # Find Python files and search for _CHECK_ID fd '\.py$' --exec rg -n '_CHECK_ID' {} \; # Search for the old numeric format (assuming it was a number like 19048) echo "Searching for potential old numeric format:" fd '\.py$' --exec rg -n '_CHECK_ID\s*=\s*\d+' {} \; # Search for the new hexadecimal string format echo "Searching for the new hexadecimal string format:" fd '\.py$' --exec rg -n '_CHECK_ID\s*=\s*"[0-9A-F]{64}"' {} \;Length of output: 683
snippets/paths.py (1)
2-2: The addition of a blank line at the beginning of the file is a trivial formatting change that does not affect the code's functionality or logic. No review comment is necessary.tests/unit/models/transactions/test_xchain_claim.py (1)
71-71: LGTM!The change from string to integer for the
destination_tagparameter is correct and aligns with the expected input type. This improves the type safety and robustness of thetest_successful_claim_destination_tagfunction.xrpl/utils/xrp_conversions.py (2)
39-39: LGTM!The change from
type()toisinstance()for type checking is a good refactor. It improves code robustness and maintainability without altering the core functionality.
87-87: LGTM!Similar to the previous function, the change from
type()toisinstance()for type checking is a good refactor. It improves code robustness and maintainability without altering the core functionality.xrpl/models/transactions/pseudo_transactions/enable_amendment.py (2)
5-5: LGTM!The import statement is correct and necessary to support the expanded type definition of the
flagsattribute.
104-104: Verify the impact of the expandedflagstype definition.The change to the
flagsattribute type is logically correct and enhances the flexibility of the attribute. The default value and documentation comment remain valid.However, this change may affect how the
flagsattribute is utilized throughout the codebase. Please ensure that:
- Existing code that interacts with the
flagsattribute has been updated to handle the new dictionary type.- The expanded type definition is properly validated and handled in the relevant methods of the
EnableAmendmentclass.You can use the following script to search for potential areas that may require updates:
Verification successful
Verification complete:
flagsattribute change appears isolatedThe expanded type definition for the
flagsattribute in theEnableAmendmentclass seems to have minimal impact on the codebase. Our analysis shows:
- No direct read or write access to the
flagsattribute was found outside the class definition.- No method definitions were found that use
flagsas a parameter.- The
flagsattribute is defined asUnion[Dict[str, bool], int, List[int]] = 0, consistent with the original review comment.The change appears to be isolated to the
EnableAmendmentclass definition. However, to ensure complete verification:
- Review any code that instantiates or processes
EnableAmendmentobjects to confirm compatibility with the new type definition.- Verify that the expanded type aligns with the intended use of the
flagsattribute as described in the documentation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flags` attribute in the codebase. # Test 1: Search for read access of the `flags` attribute. # Expect: Relevant code should handle the expanded type definition. rg --type python -A 5 $'\.flags' # Test 2: Search for write access to the `flags` attribute. # Expect: Relevant code should conform to the expanded type definition. rg --type python -A 5 $'flags\s*='Length of output: 131
Script:
#!/bin/bash # Description: Verify the usage of the `flags` attribute in the codebase using ast-grep # Test 1: Search for read access of the `flags` attribute echo "Read access of flags attribute:" ast-grep --lang python --pattern '$_.$_flags' # Test 2: Search for write access to the `flags` attribute echo "Write access to flags attribute:" ast-grep --lang python --pattern '$_.flags = $_' # Test 3: Search for method definitions that use the flags attribute as a parameter echo "Method definitions with flags as parameter:" ast-grep --lang python --pattern 'def $_($_, flags: $_, $_): $$$' # Test 4: Search for the EnableAmendment class definition echo "EnableAmendment class definition:" ast-grep --lang python --pattern 'class EnableAmendment($_): $$$'Length of output: 5906
xrpl/models/transactions/oracle_set.py (2)
23-26: LGTM!Explicitly casting
EPOCH_OFFSETto an integer improves type clarity and safety without altering the underlying value or logic.
149-149: LGTM!Updating the error message to use consistent terminology improves clarity and readability without altering the underlying meaning.
xrpl/models/requests/ledger_entry.py (2)
259-259: LGTM!The change to include
intas an additional valid type forxchain_claim_idattribute inLedgerEntryclass looks good. It aligns with the type annotation update in theXChainClaimIDclass and enhances the flexibility of theLedgerEntryclass in handling different data types for claims.
261-261: LGTM!The change to include
intas an additional valid type forxchain_create_account_claim_idattribute inLedgerEntryclass looks good. It aligns with the type annotation update in theXChainCreateAccountClaimIDclass and enhances the flexibility of theLedgerEntryclass in handling different data types for account claim IDs.tests/unit/models/transactions/test_oracle_set.py (1)
329-329: LGTM!The updated error message improves clarity by using consistent terminology for the ripple epoch reference. This change enhances readability without altering the test's functionality.
CHANGELOG.md (1)
21-21: Excellent addition to model validation!Checking parameter types as part of the validation process aligns perfectly with the PR objective. This enhancement can help catch type mismatches early, reducing potential runtime errors and improving overall data integrity.
xrpl/models/transactions/transaction.py (2)
Line range hint
267-267:
Line range hint
368-383: LGTM!The added exception handling for invalid
self.flagstype enhances the method's reliability and error reporting.tests/unit/models/test_base_model.py (7)
87-95: Effective test for invalid types intest_bad_typeThe test correctly verifies that the
Paymentmodel raises anXRPLModelExceptionwhen provided with incorrect types for theaccount,amount, anddestinationfields.
96-104: Validation offlagsfield type intest_bad_type_flagsThis test appropriately checks that passing a string instead of an integer to the
flagsfield results in anXRPLModelException, enhancing type safety.
106-114:test_bad_type_enumaccurately tests invalid enum valuesThe test ensures that providing an invalid value to the
subcommandenum in thePathFindmodel raises anXRPLModelException, which is essential for input validation.
425-437: Detection of nonexistent fields intest_nonexistent_fieldThis test correctly checks that the
TrustSetmodel raises anXRPLModelExceptionwhen an unexpected field (bad_field) is included, reinforcing model strictness.
440-440: Please address previous comment regarding clarity intest_bad_literalA previous reviewer suggested adding a comment to highlight the
bad_literalattribute in this test case for clarity. Please consider incorporating this to improve understandability.
821-843: Tests for input case sensitivity infrom_xrplmethods are comprehensiveThe added test
test_request_input_from_xrpl_accepts_camel_casecorrectly ensures thatBaseModel.from_xrplonly accepts inputs with camelCase or PascalCase keys, raising exceptions for snake_case keys. This reinforces consistency and correctness in input data handling.
845-862: Verification of key case sensitivity inTransaction.from_xrplThe test
test_transaction_input_from_xrpl_accepts_only_camel_caseappropriately verifies thatTransaction.from_xrplrejects inputs with snake_case keys, aligning with expected input formats and preventing potential parsing issues.
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
| @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ### 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
| @@ -340,6 +404,6 @@ def __eq__(self: Self, other: object) -> bool: | |||
| return isinstance(other, BaseModel) and self.to_dict() == other.to_dict() | |||
|
|
|||
| 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 |
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.