Improve phone number validator type validation - #3721
Conversation
📝 WalkthroughWalkthroughChangesPhone number validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@care/utils/models/validators.py`:
- Around line 116-119: The validator constructor’s unsupported-type handling
must consistently raise ValueError for non-string elements, including unhashable
values such as [] and hashable values such as 1. Update the validation flow
around unsupported_types and self.regex_map to validate each element’s type
before lookup, and format invalid values safely without str.join type errors.
Add regression tests covering both cases.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7634c81d-b69c-4f34-8657-4a9ad4befe95
📒 Files selected for processing (2)
care/utils/models/validators.pycare/utils/tests/test_phone_number_validator.py
There was a problem hiding this comment.
Pull request overview
This PR hardens PhoneNumberValidator initialization in care.utils.models.validators by validating the types argument more explicitly, replacing an implicit KeyError failure mode with clearer ValueErrors and adding regression tests for invalid inputs.
Changes:
- Added explicit checks for empty
typesand unsupported phone number types, raisingValueErrorwith clearer messages. - Added unit tests for invalid
typesvalues and unsupported type names. - Added a new test asserting generator input is accepted for
types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| care/utils/models/validators.py | Adds explicit types validation and clearer error handling for unsupported types. |
| care/utils/tests/test_phone_number_validator.py | Adds tests covering invalid types, unsupported types, and generator input. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
care/utils/models/validators.py:114
typesis validated as aCollection, which rejects generators/iterators. The PR description says generator inputs are accepted; if that is intended, the runtime check should allow general iterables and normalize viatuple(types)(catchingTypeError) before emptiness/unsupported-type checks.
def __init__(self, types: Collection[str], *args, **kwargs):
if not isinstance(types, Collection) or isinstance(types, str):
msg = "The `types` argument must be a non-empty collection."
raise ValueError(msg)
types = tuple(types)
if not types:
msg = "The `types` argument must be a non-empty collection."
raise ValueError(msg)
care/utils/tests/test_phone_number_validator.py:142
- This test currently treats a generator of types as invalid, which contradicts the PR description claiming generator inputs are accepted. If generator support is intended, remove it from the invalid list and add a positive test that a generator-based
typesworks end-to-end.
def test_types_must_be_non_empty_collection(self):
invalid_types = ["mobile", (), (type_ for type_ in ("mobile",))]
for types in invalid_types:
with self.assertRaisesMessage(
ValueError,
"The `types` argument must be a non-empty collection.",
):
PhoneNumberValidator(types=types)
eeshsaxena
left a comment
There was a problem hiding this comment.
Nice tightening. Two real improvements here:
- Materializing
types = tuple(types)before the emptiness check avoids the oldlen(types)blowing up on a generator (generators have no__len__), and requiringCollectionrather thanIterablemakes the empty check well-defined. - The unsupported-type loop is safe against unhashable inputs:
not isinstance(type_, str) or type_ not in self.regex_mapshort-circuits on the isinstance check, so a[]never reaches[] in self.regex_map, which would otherwise raiseTypeError: unhashable type. The[]and1test cases exercise that path.
Reads correct.
Proposed Changes
PhoneNumberValidatortypes.KeyError/TypeErrorfailures to clearValueErrormessages.typesto be a non-empty collection so one-shot iterators/generators are rejected for safer Django validator deconstruction.types, unsupported types, non-string values, unhashable values, and iterator input.Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Bug Fixes
Tests