fix: improve error handling in datasource URL parsing #18
Conversation
📝 WalkthroughWalkthroughThis pull request bumps the package version from 0.2.2 to 0.2.3 and refactors error handling in the Prisma configuration parser. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/zmodel-parser.ts (1)
114-138: Consider handling inline comments in URL extraction.The regex
url\s*=\s*([^\n]+)captures everything until newline. If the schema contains an inline comment like:url = env("DATABASE_URL") // connection stringThe captured value would include the comment, causing evaluation to fail. The
removeCommentsfunction only strips line-start comments (/^\s*\/\/.*/), not inline ones.Additionally, the
envhelper (lines 122-128) is duplicated fromloadPrismaConfig(lines 49-55). Consider extracting to a shared helper.♻️ Suggested refactor
Extract shared helper and handle trailing content:
+/** + * Create env helper that throws CliError for missing variables + */ +function createEnvHelper() { + return (varName: string) => { + const value = process.env[varName] + if (!value) { + throw new CliError(`Environment variable ${varName} is not set`) + } + return value + } +}For the regex, consider trimming trailing comments:
- const urlMatch = datasourceBlock.match(/url\s*=\s*([^\n]+)/) + const urlMatch = datasourceBlock.match(/url\s*=\s*([^\n]+?)(?:\s*\/\/.*)?$/)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/zmodel-parser.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/zmodel-parser.ts (1)
src/cli-error.ts (1)
CliError(4-4)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (4)
package.json (1)
3-3: LGTM!Patch version bump is appropriate for the error handling improvements in the parser.
src/zmodel-parser.ts (3)
74-76: LGTM!Clean simplification with proper optional chaining for safe property access.
139-147: LGTM!Good fallback logic with clear, actionable error message guiding users to either configure
prisma.config.tsor use the-doption.
149-151: LGTM!Good safety net catching edge cases where the URL expression evaluates to an empty or falsy value, complementing the earlier
nullcheck.
There was a problem hiding this comment.
Pull request overview
This PR improves error handling in datasource URL parsing by consolidating the URL evaluation logic and removing redundant try-catch blocks. The changes aim to provide clearer error messages and a more unified approach to parsing datasource URLs from both schema files and Prisma configuration files.
Changes:
- Simplified error handling in
loadPrismaConfigby removing an inner try-catch block - Unified URL parsing in
parseDatasourceto use a single regex pattern and Function constructor evaluation for all URL formats - Updated error messages to be more consistent and guide users toward the
-doption
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/zmodel-parser.ts | Refactored URL parsing logic to consolidate evaluation approach and improve error messages |
| package.json | Version bump from 0.2.2 to 0.2.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If still no URL found, throw error | ||
| if (url == null) { | ||
| throw new CliError( | ||
| 'No datasource URL found. For Prisma 7, ensure prisma.config.ts exists with datasource configuration, or provide the URL via -d option.' | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a logic issue with the dual error checking. When urlMatch succeeds (line 118), url is assigned from the evaluation (line 133), then the check at line 149 validates if it's truthy. However, when urlMatch fails (line 139), the code loads from loadPrismaConfig, checks if it's null at line 142, but then checks again at line 149. This means when coming from the else branch, if url is null, it throws at line 142, making the check at line 149 unreachable for that path. If coming from the if branch, the evaluation could theoretically return an empty string or other falsy value that's not caught by the try-catch. The logic should be restructured to have a single validation point after both paths complete, or the check at line 149 should be removed if line 142 is sufficient for all cases.
| // If still no URL found, throw error | |
| if (url == null) { | |
| throw new CliError( | |
| 'No datasource URL found. For Prisma 7, ensure prisma.config.ts exists with datasource configuration, or provide the URL via -d option.' | |
| ) | |
| } | |
| } | |
| } | |
| if (url == null) { | |
| throw new CliError( | |
| 'No datasource URL found. For Prisma 7, ensure prisma.config.ts exists with datasource configuration, or provide the URL via -d option.' | |
| ) | |
| } |
| // Extract url from schema | ||
| let urlMatch = datasourceBlock.match(/url\s*=\s*['"]([^'"]+)['"]/) | ||
| // Extract url value using single regex (could be string literal, env() call, or expression) | ||
| const urlMatch = datasourceBlock.match(/url\s*=\s*([^\n]+)/) |
There was a problem hiding this comment.
The regex pattern ([^\n]+) will capture everything up to the newline, including trailing whitespace, semicolons, and potentially inline comments. For example, with url = "postgres://..." // comment, this will capture "postgres://..." // comment. While the .trim() on line 119 removes leading/trailing whitespace, it won't handle inline comments or trailing semicolons that might be part of urlValueStr before evaluation. This could cause the Function constructor evaluation to fail unexpectedly. Consider using a more specific pattern like ([^\n;/]+) to stop at semicolons or comment markers, or add additional sanitization of the captured value.
| 'Could not evaluate datasource url from schema, you could provide it via -d option.' | ||
| ) | ||
| } | ||
| } else { | ||
| url = loadPrismaConfig(schemaDir) | ||
| // If still no URL found, throw error | ||
| if (url == null) { | ||
| throw new CliError( | ||
| 'No datasource URL found. For Prisma 7, ensure prisma.config.ts exists with datasource configuration, or provide the URL via -d option.' |
There was a problem hiding this comment.
Inconsistent error message format and option naming. Line 136 uses "-d option" while line 144 uses "-d option" but previously the code mentioned "--datasource-url option" (visible in the diff context). The error messages should consistently reference the same option format. Additionally, line 136 says "Could not evaluate datasource url" while line 144 says "No datasource URL found" - these should follow a consistent capitalization style for "datasource URL" vs "datasource url".
| throw new CliError( | ||
| 'No datasource URL found. For Prisma 7, ensure prisma.config.ts exists with datasource configuration, or provide the URL via --datasource-url option.' | ||
| ) | ||
| throw new CliError('datasource url has no value, you could provide it via -d option.') |
There was a problem hiding this comment.
The error message uses inconsistent capitalization and grammar. It should be "Datasource URL has no value, you could provide it via -d option." with "Datasource" capitalized to match the style of other error messages, or follow a consistent lowercase style throughout. Additionally, consider adding a period after "option" for consistency with other error messages in the codebase.
| throw new CliError('datasource url has no value, you could provide it via -d option.') | |
| throw new CliError('Datasource URL has no value, you could provide it via -d option.') |
| } | ||
| const configFn = new Function('env', `return ${configObjectStr}`) | ||
| const config = configFn(env) | ||
| return config?.datasource?.url |
There was a problem hiding this comment.
The return type signature of loadPrismaConfig is string | null, but line 76 can potentially return undefined if config?.datasource?.url is undefined. While functionally similar, this creates a type inconsistency. Consider explicitly handling the undefined case by changing to return config?.datasource?.url || null or return config?.datasource?.url ?? null to ensure the return value strictly matches the declared type signature.
| return config?.datasource?.url | |
| return config?.datasource?.url ?? null |
| try { | ||
| // Use Function constructor to evaluate the url value | ||
| const urlFn = new Function('env', `return ${urlValueStr}`) | ||
| url = urlFn(env) |
There was a problem hiding this comment.
Using the Function constructor with user-controlled input (schema file content) poses a security risk. While the code attempts to sandbox by only passing the env function, the evaluated code can still execute arbitrary JavaScript. An attacker with control over the schema file could potentially inject malicious code. Consider adding input validation to ensure urlValueStr only contains expected patterns (quotes, env() calls, etc.) before evaluation, or use a safer alternative like a purpose-built parser for Prisma schema syntax.
| try { | |
| // Use Function constructor to evaluate the url value | |
| const urlFn = new Function('env', `return ${urlValueStr}`) | |
| url = urlFn(env) | |
| // Safely evaluate limited URL expressions without using the Function constructor. | |
| // Supported forms: | |
| // - String literal: "postgres://..." or 'postgres://...' | |
| // - env("VAR") or env('VAR') | |
| // - Concatenations using + of the above, e.g. env("URL") + "/shadow" | |
| const evaluateUrlExpression = (expr: string): string => { | |
| const parts = expr.split('+').map((p) => p.trim()).filter((p) => p.length > 0) | |
| if (parts.length === 0) { | |
| throw new CliError( | |
| 'Could not evaluate datasource url from schema, you could provide it via -d option.' | |
| ) | |
| } | |
| let result = '' | |
| for (const part of parts) { | |
| // Match simple string literal | |
| const stringMatch = part.match(/^(['"])(.*)\1$/) | |
| if (stringMatch) { | |
| result += stringMatch[2] | |
| continue | |
| } | |
| // Match env("VAR") or env('VAR') | |
| const envMatch = part.match(/^env\(\s*(['"])([A-Za-z_][A-Za-z0-9_]*)\1\s*\)$/) | |
| if (envMatch) { | |
| result += env(envMatch[2]) | |
| continue | |
| } | |
| // Any other pattern is considered unsupported for security reasons | |
| throw new CliError( | |
| 'Unsupported datasource URL expression in schema; only string literals, env() calls, and "+" concatenations are allowed.' | |
| ) | |
| } | |
| return result | |
| } | |
| try { | |
| url = evaluateUrlExpression(urlValueStr) |
| const configFn = new Function('env', `return ${configObjectStr}`) | ||
| const config = configFn(env) | ||
| return config?.datasource?.url |
There was a problem hiding this comment.
The removal of the inner try-catch block around the Function constructor evaluation in loadPrismaConfig means that any errors during evaluation (other than environment variable errors) will now propagate up and be caught by the outer catch block at line 77. However, this changes the behavior: previously, evaluation errors would result in returning null with a warning, now they will log a warning and return null (same outcome). But if the Function constructor itself throws an error during construction (syntax error in the config object string), this will now be handled differently. Ensure this behavioral change is intentional and doesn't break existing error handling expectations.
| const configFn = new Function('env', `return ${configObjectStr}`) | |
| const config = configFn(env) | |
| return config?.datasource?.url | |
| try { | |
| const configFn = new Function('env', `return ${configObjectStr}`) | |
| const config = configFn(env) | |
| return config?.datasource?.url | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Environment variable')) { | |
| // Propagate environment variable errors so they can be handled upstream | |
| throw error | |
| } | |
| console.warn(`Warning: Failed to parse prisma.config.ts: ${error}`) | |
| return null | |
| } |
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.