updated slug validation regex to make it url safe#3603
updated slug validation regex to make it url safe#3603
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughValidator regex tightened to require slugs to start and end with ASCII alphanumerics and allow only ASCII letters/digits, hyphen or underscore in the middle; questionnaire test fixtures were adjusted to lowercase slugs; a slug test was simplified to accept uppercase input (validator now allows uppercase boundaries). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR tightens the slug validation regex from the permissive Key observations:
Confidence Score: 5/5Safe to merge — the logic change is correct and the test suite is improved; remaining findings are minor coverage gaps. All findings are P2 (style/coverage suggestions). The regex itself is correct, the boundary conditions are properly enforced, and the existing tests are strengthened. No runtime errors, data-loss risk, or broken validation paths were identified. No files require special attention, though Important Files Changed
|
![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)
care/emr/utils/slug_type.py (2)
33-34: Please centralize the slug regex to avoid validator drift.Line [33] duplicates the same pattern from Line [15]. A shared constant keeps both validators aligned and easier to maintain.
♻️ Proposed refactor
import re from typing import Annotated from pydantic import AfterValidator, Field +SLUG_PATTERN = r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$" def slug_validator(value: str) -> str: @@ - pattern = r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$" - if not re.match(pattern, value): + if not re.match(SLUG_PATTERN, value): raise ValueError( @@ def extended_slug_validator(value: str) -> str: @@ - pattern = r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$" - if not re.match(pattern, value): + if not re.match(SLUG_PATTERN, value): raise ValueError(As per coding guidelines, "**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/utils/slug_type.py` around lines 33 - 34, Duplicate regex pattern in slug_type.py should be centralized: define a single module-level constant (e.g., SLUG_PATTERN or SLUG_REGEX) near the top of the file and replace both occurrences of the inline pattern (the local variable pattern used at the call to re.match in the validator) with that constant; update the validator(s) that reference pattern to use SLUG_REGEX and ensure imports (re) remain, keeping name uppercase and a brief docstring comment so both validators (the one around line 15 and the one around line 33) use the same source of truth.
15-16: Extract the repeated regex pattern into a module-level constant.The pattern
r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$"is defined identically in bothslug_validator(line 16) andextended_slug_validator(line 34). Duplicating regex patterns violates DRY and creates a maintainability risk if the pattern needs updating later. Define it once at module level and reference it in both validators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/utils/slug_type.py` around lines 15 - 16, The regex pattern r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$" is duplicated in slug_validator and extended_slug_validator; extract it into a module-level constant (e.g., SLUG_PATTERN) and update both functions to reference that constant instead of hardcoding the pattern so future changes are centralized and DRY is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/tests/test_slug_type.py`:
- Around line 43-44: Add explicit boundary-case assertions to the existing test
that currently checks uppercase rejection: after the TestModel(slug="UPPERCASE")
assertion, add additional with self.assertRaises(ValidationError) checks
constructing TestModel with slugs "-start", "_start", "end-", and "end_" to
ensure the model enforces “must start/end alphanumeric” behavior; reference the
TestModel constructor and ValidationError in care/emr/tests/test_slug_type.py
when adding these cases.
---
Nitpick comments:
In `@care/emr/utils/slug_type.py`:
- Around line 33-34: Duplicate regex pattern in slug_type.py should be
centralized: define a single module-level constant (e.g., SLUG_PATTERN or
SLUG_REGEX) near the top of the file and replace both occurrences of the inline
pattern (the local variable pattern used at the call to re.match in the
validator) with that constant; update the validator(s) that reference pattern to
use SLUG_REGEX and ensure imports (re) remain, keeping name uppercase and a
brief docstring comment so both validators (the one around line 15 and the one
around line 33) use the same source of truth.
- Around line 15-16: The regex pattern r"^[a-z0-9][a-z0-9_-]*[a-z0-9]$" is
duplicated in slug_validator and extended_slug_validator; extract it into a
module-level constant (e.g., SLUG_PATTERN) and update both functions to
reference that constant instead of hardcoding the pattern so future changes are
centralized and DRY is preserved.
🪄 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
Run ID: 7aeb0aa7-92c7-42b5-9cf3-b449bdaccb98
📒 Files selected for processing (3)
care/emr/tests/test_questionnaire_api.pycare/emr/tests/test_slug_type.pycare/emr/utils/slug_type.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3603 +/- ##
===========================================
- Coverage 77.20% 77.19% -0.01%
===========================================
Files 474 474
Lines 22421 22421
Branches 2348 2348
===========================================
- Hits 17310 17308 -2
- Misses 4531 4532 +1
- Partials 580 581 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Proposed Changes
Regex used -
^[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]$Only 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