fix(profile): enforce profile name validation and input constraints#15694
Open
carlh7777 wants to merge 2 commits into
Open
fix(profile): enforce profile name validation and input constraints#15694carlh7777 wants to merge 2 commits into
carlh7777 wants to merge 2 commits into
Conversation
- Introduced a new constant `NICKNAME_MAX_LENGTH` in `api/constants.py` to define the maximum length for user nicknames. - Implemented `validate_nickname` function in `api/utils/validation_utils.py` to enforce nickname rules, including character validation and length checks. - Updated `setting_user` and `user_add` functions in `user_api.py` to validate nicknames during user updates and additions. - Enhanced frontend validation in user settings to reflect nickname constraints, including error messages for invalid input. - Added unit tests to ensure nickname validation works as expected. This change improves user experience by enforcing consistent nickname rules across the application.
dosubot
Bot
added
size:M
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds nickname validation end-to-end: a 100-character max and allowed-character regex on backend and frontend, integrates validation into registration and profile update endpoints, includes unit tests, localized error messages, and minor UI/input attribute adjustments. ChangesNickname Validation Implementation
Sequence Diagram(s): sequenceDiagram
participant Client
participant FrontendForm
participant UserAPI
participant ValidationUtils
Client->>FrontendForm: Enter nickname
FrontendForm->>ValidationUtils: (client-side) validate via NICKNAME_PATTERN / maxLength
FrontendForm-->>Client: show client-side validation errors or submit
FrontendForm->>UserAPI: POST/PATCH with nickname
UserAPI->>ValidationUtils: validate_nickname(nickname)
ValidationUtils-->>UserAPI: (error_message, code) or (None, None)
UserAPI-->>FrontendForm: structured error response or success
Estimated code review effort: Suggested labels:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/unit_test/api/utils/test_nickname_validation.py (3)
24-37: ⚡ Quick winConsider adding a test case for leading/trailing whitespace.
The validation function strips whitespace before checking (line 1105 in validation_utils.py), so a nickname like
" John Doe "should pass validation. Adding this test case would verify the trimming behavior explicitly.🧪 Suggested test case
`@pytest.mark.parametrize`( "nickname", [ "John Doe", "张三", "O'Brien", "valid-name_123", "a" * NICKNAME_MAX_LENGTH, " trimmed ", # Leading/trailing whitespace should be accepted ], ) def test_validate_nickname_accepts_valid_values(nickname): message, code = validate_nickname(nickname) assert message is None assert code is 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 `@test/unit_test/api/utils/test_nickname_validation.py` around lines 24 - 37, Add a test case verifying leading/trailing whitespace is accepted by validate_nickname: update the pytest.parametrize list in test_validate_nickname_accepts_valid_values (file test_nickname_validation.py) to include a nickname like " trimmed " so the test asserts validate_nickname(str) returns (None, None) for inputs that are whitespace-trimmed by the validate_nickname function.
40-52: ⚡ Quick winConsider adding a test case for None input.
The
validate_nicknamefunction explicitly handlesNoneinput at line 1102 and returns"Nickname is required.". Adding a test case for this would improve coverage and document the expected behavior.🧪 Suggested test case
`@pytest.mark.parametrize`( "nickname, expected_message", [ (None, "Nickname is required."), ("", "Nickname cannot be empty."), (" ", "Nickname cannot be empty."), ("carh!@#$%^&*()_+WFAGD", "Nickname contains invalid characters."), ("a" * (NICKNAME_MAX_LENGTH + 1), f"Nickname must be at most {NICKNAME_MAX_LENGTH} characters."), ], ) def test_validate_nickname_rejects_invalid_values(nickname, expected_message): message, code = validate_nickname(nickname) assert message == expected_message assert code == RetCode.ARGUMENT_ERROR🤖 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 `@test/unit_test/api/utils/test_nickname_validation.py` around lines 40 - 52, Add a test case for None input to the existing test_validate_nickname_rejects_invalid_values parametrized test: include (None, "Nickname is required.") in the parametrized list so that calling validate_nickname(None) asserts the expected message and RetCode.ARGUMENT_ERROR; update the tuple list in test_validate_nickname_rejects_invalid_values in test_nickname_validation.py to include this case, keeping the test function name and assertions unchanged.
24-52: ⚡ Quick winAdd pytest priority markers per coding guidelines.
The coding guidelines specify "Use pytest with priority markers (p1/p2/p3) for Python testing." These test functions should be marked with an appropriate priority level.
📝 Suggested markers
+@pytest.mark.p1 `@pytest.mark.parametrize`( "nickname", [+@pytest.mark.p1 `@pytest.mark.parametrize`( "nickname, expected_message", [As per coding guidelines: "Use pytest with priority markers (p1/p2/p3) for Python testing."
🤖 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 `@test/unit_test/api/utils/test_nickname_validation.py` around lines 24 - 52, Add the required pytest priority markers to the two test functions: annotate test_validate_nickname_accepts_valid_values and test_validate_nickname_rejects_invalid_values with the appropriate priority marker (e.g., `@pytest.mark.p1`) immediately above each def so the tests comply with the project's pytest priority convention; ensure the marker is imported/available from pytest if needed.api/utils/validation_utils.py (1)
1094-1113: 💤 Low valueConsider returning the stripped nickname on success.
The function strips the nickname at line 1105 for validation but discards the result on success, forcing callers to strip again (see user_api.py lines 368 and 532). Returning the stripped value would eliminate this duplication.
♻️ Proposed refactor
-def validate_nickname(nickname: str | None) -> tuple[str | None, int | None]: +def validate_nickname(nickname: str | None) -> tuple[str | None, int | None, str | None]: """ Validate a user nickname/display name. Returns: - A tuple of (error_message, error_code) if validation fails, - or (None, None) if validation passes. + A tuple of (error_message, error_code, None) if validation fails, + or (None, None, stripped_nickname) if validation passes. """ if nickname is None: - return "Nickname is required.", RetCode.ARGUMENT_ERROR + return "Nickname is required.", RetCode.ARGUMENT_ERROR, None nickname = nickname.strip() if not nickname: - return "Nickname cannot be empty.", RetCode.ARGUMENT_ERROR + return "Nickname cannot be empty.", RetCode.ARGUMENT_ERROR, None if len(nickname) > NICKNAME_MAX_LENGTH: - return f"Nickname must be at most {NICKNAME_MAX_LENGTH} characters.", RetCode.ARGUMENT_ERROR + return f"Nickname must be at most {NICKNAME_MAX_LENGTH} characters.", RetCode.ARGUMENT_ERROR, None if not _NICKNAME_PATTERN.fullmatch(nickname): - return "Nickname contains invalid characters.", RetCode.ARGUMENT_ERROR - return None, None + return "Nickname contains invalid characters.", RetCode.ARGUMENT_ERROR, None + return None, None, nicknameThen update callers to use the returned stripped value instead of stripping again.
🤖 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 `@api/utils/validation_utils.py` around lines 1094 - 1113, Change validate_nickname to return the cleaned/stripped nickname on success so callers don't have to strip again: update the function signature/type annotations and docstring for validate_nickname to return (error_message: str | None, error_code: int | None, cleaned_nickname: str | None), perform nickname = nickname.strip() as you already do, and on success return (None, None, nickname); update callers (e.g., the places in user_api.py that currently call validate_nickname and then call .strip() again) to accept the third return value and use the returned cleaned_nickname instead of re-stripping.
🤖 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 `@api/utils/validation_utils.py`:
- Around line 1094-1113: Update validate_nickname to accept Optional strings and
defensively handle non-string input: change the parameter type from str to str |
None (or Optional[str]) and keep the existing None check, but add an
isinstance(nickname, str) guard that returns an ARGUMENT_ERROR message (e.g.,
"Nickname must be a string.") if the input is not a string; then proceed with
.strip(), length check against NICKNAME_MAX_LENGTH, and pattern check with
_NICKNAME_PATTERN, returning errors via RetCode where appropriate.
In `@web/src/pages/user-setting/profile/constants.ts`:
- Line 3: NICKNAME_PATTERN uses JS \w which is ASCII-only and causes a
frontend/backend mismatch; update the constant NICKNAME_PATTERN in
web/src/pages/user-setting/profile/constants.ts to use Unicode property escapes
(e.g. include \p{L}, \p{N}, and combining marks \p{M} plus whitespace and
allowed punctuation) with the u flag instead of \w so the pattern matches the
backend's Unicode-aware \w behavior; ensure the new regex still allows spaces,
periods, apostrophes and hyphens and add a quick test with non-Latin nicknames
to verify parity with api/utils/validation_utils.py.
---
Nitpick comments:
In `@api/utils/validation_utils.py`:
- Around line 1094-1113: Change validate_nickname to return the cleaned/stripped
nickname on success so callers don't have to strip again: update the function
signature/type annotations and docstring for validate_nickname to return
(error_message: str | None, error_code: int | None, cleaned_nickname: str |
None), perform nickname = nickname.strip() as you already do, and on success
return (None, None, nickname); update callers (e.g., the places in user_api.py
that currently call validate_nickname and then call .strip() again) to accept
the third return value and use the returned cleaned_nickname instead of
re-stripping.
In `@test/unit_test/api/utils/test_nickname_validation.py`:
- Around line 24-37: Add a test case verifying leading/trailing whitespace is
accepted by validate_nickname: update the pytest.parametrize list in
test_validate_nickname_accepts_valid_values (file test_nickname_validation.py)
to include a nickname like " trimmed " so the test asserts
validate_nickname(str) returns (None, None) for inputs that are
whitespace-trimmed by the validate_nickname function.
- Around line 40-52: Add a test case for None input to the existing
test_validate_nickname_rejects_invalid_values parametrized test: include (None,
"Nickname is required.") in the parametrized list so that calling
validate_nickname(None) asserts the expected message and RetCode.ARGUMENT_ERROR;
update the tuple list in test_validate_nickname_rejects_invalid_values in
test_nickname_validation.py to include this case, keeping the test function name
and assertions unchanged.
- Around line 24-52: Add the required pytest priority markers to the two test
functions: annotate test_validate_nickname_accepts_valid_values and
test_validate_nickname_rejects_invalid_values with the appropriate priority
marker (e.g., `@pytest.mark.p1`) immediately above each def so the tests comply
with the project's pytest priority convention; ensure the marker is
imported/available from pytest if needed.
🪄 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: b89bbe43-e546-4c14-b8af-2015a07caa2d
📒 Files selected for processing (9)
api/apps/restful_apis/user_api.pyapi/constants.pyapi/utils/validation_utils.pytest/testcases/test_web_api/test_user_app/test_user_app_unit.pytest/unit_test/api/utils/test_nickname_validation.pyweb/src/locales/en.tsweb/src/locales/zh.tsweb/src/pages/user-setting/profile/constants.tsweb/src/pages/user-setting/profile/index.tsx
- Updated `validate_nickname` function in `validation_utils.py` to accept None as a valid input and return an appropriate error message for non-string types. - Added unit tests in `test_nickname_validation.py` to cover cases for None and non-string inputs, ensuring robust validation behavior. - Modified the nickname pattern in `constants.ts` to allow Unicode letters and numbers, improving character validation for nicknames. These changes improve the reliability of nickname validation and enhance user feedback for invalid inputs.
Author
|
@JinHai-CN, @dcc123456 could you please check this PR? |
Author
|
@KevinHuSh @yingfeng could you please check this pr? |
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.
What problem does this PR solve?
The Profile Name field currently lacks application-level validation and allows users to save excessively long names and unsupported special characters.
While the database enforces a maximum length of 100 characters, neither the frontend nor backend validates nickname format before persistence. This can result in inconsistent user data, poor user experience, and UI layout issues when long names wrap across multiple lines.
This PR introduces consistent frontend and backend validation for profile names, enforces length and character constraints, provides clear validation feedback, and prevents invalid values from being saved.
Fixes #15693
Type of change
Screenshots