Skip to content

Conversation

nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Sep 22, 2025

Summary by CodeRabbit

  • New Features
    • Role names now support the “@” character, making it easier to align with common naming conventions (e.g., email-like identifiers).
    • Validation continues to allow alphanumeric characters and select special characters while still preventing consecutive special characters, preserving existing safeguards during role creation and renaming.

Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

Walkthrough

Expanded validation in src/validator.rs to allow '@' in user_role_name while keeping existing rules: alphanumerics allowed, no consecutive special characters. No public API or signature changes.

Changes

Cohort / File(s) Change Summary
Validator logic
src/validator.rs
Added '@' to allowed special characters for user_role_name; maintained existing checks for alphanumerics and rejection of consecutive special chars; no API/signature changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled on code by moonlit gleam,
Slipped “@” into the validator stream—
No double-sparkles, keep it clean,
Alphas march where they have been.
Hop, tap, pass: a tidy theme. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning No pull request description was provided but the repository's template requires a 'Fixes #XXXX' reference, a Description describing the goal, rationale and key changes, and completion of the testing/documentation checklist; these required sections are missing, so reviewers lack necessary context to evaluate the change. Because the template was not followed and essential information about intent and testing is absent, the description check fails. Please populate the PR template: add a 'Fixes #...' line if applicable, write a short Description explaining why '@' should be allowed and summarize the key code changes (e.g., src/validator.rs), and mark or complete the checklist showing how the change was tested and whether documentation was updated. After updating the description and checklist, re-request review so reviewers can validate intent and testing.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: allow @ in the username" succinctly and accurately summarizes the primary change in this PR — the validator was updated to permit the '@' character in usernames (see src/validator.rs). It is concise, specific, and uses a conventional commit-style prefix appropriate for a small behavioral fix.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/validator.rs (1)

121-126: Update error messages and external validators to include '@'

Validator logic permits '@' (src/validator.rs ~lines 121–126) but the error strings still say "underscore, hyphen and dot" (src/validator.rs:188, :196). Update those messages and any UI/docs/CLI regexes/patterns (e.g. [A-Za-z0-9_.-]) to include '@'.

🧹 Nitpick comments (3)
src/validator.rs (3)

187-190: Error messages now outdated; add '@' to allowed list

Messages still say “underscore, hyphen and dot” but code allows '@'. Update both variants.

Apply:

-            "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen and dot) are allowed"
+            "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen, dot, and '@') are allowed"

(Do this for both SpecialChar and InvalidCharacter.)

Also applies to: 195-198


112-132: Start‑char rule is undocumented/inconsistent

There’s an InvalidStartChar variant, but no start‑char check. Either enforce it or remove/deprecate the variant.

Option A (enforce start alnum):

     // Check last character (must be alphanumeric)
     if let Some(last_char) = chars.last()
         && !last_char.is_ascii_alphanumeric()
     {
         return Err(UsernameValidationError::InvalidEndChar);
     }

+    // Check first character (must be alphanumeric)
+    if let Some(first_char) = chars.first()
+        && !first_char.is_ascii_alphanumeric()
+    {
+        return Err(UsernameValidationError::InvalidStartChar);
+    }

Option B (drop the rule): deprecate or remove InvalidStartChar.

Also applies to: 191-193


184-203: Duplicate/unused error variant

SpecialChar and InvalidCharacter carry the same meaning; SpecialChar appears unused. Consider deprecating to avoid API confusion.

Apply:

-    #[error(
+    #[deprecated(note = "Use InvalidCharacter instead")]
+    #[error(
             "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen, dot, and '@') are allowed"
         )]
     SpecialChar,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4306250 and 4b71f65.

📒 Files selected for processing (1)
  • src/validator.rs (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: coverage
🔇 Additional comments (2)
src/validator.rs (2)

121-126: Do we want to cap '@' to a single occurrence?

Current logic permits multiple '@' if separated by alnum (e.g., a@b@c). If the intent is “email‑like usernames,” consider restricting to at most one '@'. Otherwise, ignore.


94-96: Length check: confirm system limits

3–64 is reasonable, but please verify this matches DB column limits, auth provider constraints, and UI validations to avoid cross‑layer rejects.

@nitisht nitisht merged commit 61eb9f9 into parseablehq:main Sep 22, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants