Add Allow arbitrary character length in nanoid #4004
Conversation
WalkthroughThis pull request updates the nanoid validation functionality. A new test case for a 64-character nanoid and custom error messages was added in two test suites. The type definitions and method for nanoid validation were modified: the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator as ZodString/nanoid
participant RegexGen as nanoidRegex()
participant CheckHandler as _addCheck()
User->>Validator: Provide parameters (length/message/object)
Validator->>RegexGen: Generate regex based on specified length
RegexGen-->>Validator: Return dynamic regex
Validator->>CheckHandler: Add nanoid check with length and error message
CheckHandler-->>Validator: Return validation outcome
Validator-->>User: Return parsed result or validation error
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
✅ Deploy Preview for guileless-rolypoly-866f8a ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Nitpick comments (3)
deno/lib/types.ts (1)
884-885: Add length boundary checks when constructing the nanoid regex.
Although the regex works whencheck.valueis valid, there’s no safeguard against non-integer or negative values. Consider rejecting length values below 1 or above a reasonable upper bound to prevent unexpected behavior.src/__tests__/string.test.ts (1)
383-400: LGTM! Well-structured test implementation.The test case effectively validates the new functionality for arbitrary character length nanoid validation, including custom error message handling.
Consider adding more test cases for comprehensive coverage.
Consider adding tests for:
- Maximum length validation
- Invalid character validation
- Edge cases (empty string, null, undefined)
Apply this diff to add more test cases:
test("custom character length nanoid", () => { const nanoid64 = z.string().nanoid(64); const valid64 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"; nanoid64.parse(valid64); + // Test maximum length validation + const nanoid128 = z.string().nanoid(128); + const valid128 = valid64 + valid64; + nanoid128.parse(valid128); + + // Test invalid characters + const invalidChars = valid64.replace("a", "@"); + expect(() => nanoid64.parse(invalidChars)).toThrow(); + + // Test edge cases + expect(() => nanoid64.parse("")).toThrow(); + expect(() => nanoid64.parse(null as any)).toThrow(); + expect(() => nanoid64.parse(undefined as any)).toThrow(); const nanoid64WithCustomError = z .string() .nanoid(64, { message: "custom error" }); nanoid64WithCustomError.parse(valid64); const shortId = "abc123"; const result = nanoid64WithCustomError.safeParse(shortId); expect(result.success).toEqual(false); if (!result.success) { expect(result.error.issues[0].message).toEqual("custom error"); } });deno/lib/__tests__/string.test.ts (1)
384-401: LGTM! Consider adding more test cases for comprehensive coverage.The test implementation is well-structured and covers the basic validation paths. However, consider adding test cases for:
- Invalid characters in a 64-character string
- Edge cases like empty string, null, undefined
- Boundary values (63 and 65 characters)
Here's a suggested expansion of the test cases:
test("custom character length nanoid", () => { const nanoid64 = z.string().nanoid(64); const valid64 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"; nanoid64.parse(valid64); const nanoid64WithCustomError = z .string() .nanoid(64, { message: "custom error" }); nanoid64WithCustomError.parse(valid64); const shortId = "abc123"; const result = nanoid64WithCustomError.safeParse(shortId); expect(result.success).toEqual(false); if (!result.success) { expect(result.error.issues[0].message).toEqual("custom error"); } + + // Test invalid characters + const invalidChars = valid64.replace("a", "@"); + expect(nanoid64.safeParse(invalidChars).success).toEqual(false); + + // Test boundary values + const shortBy1 = valid64.slice(0, -1); + expect(nanoid64.safeParse(shortBy1).success).toEqual(false); + + const longBy1 = valid64 + "A"; + expect(nanoid64.safeParse(longBy1).success).toEqual(false); + + // Test edge cases + expect(nanoid64.safeParse("").success).toEqual(false); + expect(nanoid64.safeParse(undefined).success).toEqual(false); + expect(nanoid64.safeParse(null).success).toEqual(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deno/lib/__tests__/string.test.ts(1 hunks)deno/lib/types.ts(4 hunks)src/__tests__/string.test.ts(1 hunks)src/types.ts(4 hunks)
🔇 Additional comments (4)
deno/lib/types.ts (1)
595-595: Recommend validating thevaluefield for non-negative lengths.
Currently, there's no explicit check to ensure thatvalue(representing the nanoid length) is positive. Consider validating that it meets a minimum requirement, e.g.,>= 1, to avoid unintended behaviors.src/types.ts (3)
595-595: LGTM! Type definition updated to support arbitrary character length.The addition of the
valueproperty to the nanoid check type aligns with the PR objective and follows the pattern used by other string checks.
644-646: LGTM! Well-implemented dynamic regex generator.The regex pattern correctly validates:
- Alphanumeric characters plus underscore and hyphen
- Case-insensitive matching
- Exact length enforcement
1122-1146: LGTM! Excellent implementation with backward compatibility.The implementation successfully:
- Maintains backward compatibility with existing code using string parameter
- Adds support for arbitrary length specification
- Provides clear parameter overloads
- Uses a sensible default length of 21
| function nanoidRegex(length?: number): RegExp { | ||
| return new RegExp(`^[a-z0-9_-]{${length}}$`, "i"); | ||
| } |
There was a problem hiding this comment.
Handle undefined or invalid length in regex generation.
If length is undefined or negative, the template string produces an invalid quantifier (e.g., ^[a-z0-9_-]{undefined}$). This will lead to a runtime SyntaxError. Consider defaulting to a safe value (e.g., 21) or throwing a descriptive error when length is missing or invalid.
| nanoid( | ||
| arg1?: number | string | { length?: number; message?: string }, | ||
| arg2?: { message?: string } | ||
| ) { | ||
| let length = 21; | ||
| let message: string | undefined; | ||
|
|
||
| // This workaround ensures backward compatibility for custom error message (existing calls still work) | ||
| // while enabling a new way to specify a custom length and message. | ||
| // old: nanoid("custom error") & new: nanoid(64, { message: "custom error" }) | ||
| if (typeof arg1 === "string") { | ||
| message = arg1; | ||
| } else if (typeof arg1 === "number") { | ||
| length = arg1; | ||
| if (arg2?.message) { | ||
| message = arg2.message; | ||
| } | ||
| } | ||
|
|
||
| return this._addCheck({ | ||
| kind: "nanoid", | ||
| value: length, | ||
| ...errorUtil.errToObj(message), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extend handling for object-based arguments in the nanoid(...) method.
The new signature allows passing an object { length?: number; message?: string }, but the code only updates length/message when arg1 is a string or a number. To ensure consistency with the documented usage, consider adding an else if (typeof arg1 === "object") block, for example:
+ } else if (typeof arg1 === "object" && arg1 !== null) {
+ length = arg1.length ?? 21;
+ message = arg1.message;
+ }📝 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.
| nanoid( | |
| arg1?: number | string | { length?: number; message?: string }, | |
| arg2?: { message?: string } | |
| ) { | |
| let length = 21; | |
| let message: string | undefined; | |
| // This workaround ensures backward compatibility for custom error message (existing calls still work) | |
| // while enabling a new way to specify a custom length and message. | |
| // old: nanoid("custom error") & new: nanoid(64, { message: "custom error" }) | |
| if (typeof arg1 === "string") { | |
| message = arg1; | |
| } else if (typeof arg1 === "number") { | |
| length = arg1; | |
| if (arg2?.message) { | |
| message = arg2.message; | |
| } | |
| } | |
| return this._addCheck({ | |
| kind: "nanoid", | |
| value: length, | |
| ...errorUtil.errToObj(message), | |
| }); | |
| nanoid( | |
| arg1?: number | string | { length?: number; message?: string }, | |
| arg2?: { message?: string } | |
| ) { | |
| let length = 21; | |
| let message: string | undefined; | |
| // This workaround ensures backward compatibility for custom error message (existing calls still work) | |
| // while enabling a new way to specify a custom length and message. | |
| // old: nanoid("custom error") & new: nanoid(64, { message: "custom error" }) | |
| if (typeof arg1 === "string") { | |
| message = arg1; | |
| } else if (typeof arg1 === "number") { | |
| length = arg1; | |
| if (arg2?.message) { | |
| message = arg2.message; | |
| } | |
| } else if (typeof arg1 === "object" && arg1 !== null) { | |
| length = arg1.length ?? 21; | |
| message = arg1.message; | |
| } | |
| return this._addCheck({ | |
| kind: "nanoid", | |
| value: length, | |
| ...errorUtil.errToObj(message), | |
| }); | |
| } |
I hope you don't mind, but I've gone ahead and implemented it.
#3954.
Changes Introduced:
・Allow arbitrary character length in nanoid.
・Since the existing implementation assumed the first argument to nanoid would be a custom error message, I made sure not to break that behavior.
・I added an interface for arguments such as min, max, and length.
Issue Reference:
Closes #3954.
Summary by CodeRabbit
New Features
Tests