fix(webapp): strip secure param from query ClickHouse URL#3204
fix(webapp): strip secure param from query ClickHouse URL#3204edosrecki wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
The `initializeQueryClickhouseClient()` function was missing the
`url.searchParams.delete("secure")` call that the other two sibling
ClickHouse client init functions already had. This caused a startup
crash (`Error: Unknown URL parameters: secure`) when QUERY_CLICKHOUSE_URL
fell back to CLICKHOUSE_URL which contains `?secure=false`.
Fixes triggerdotdev#3184
|
|
Hi @edosrecki, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request documents and implements a fix to strip the "secure" query parameter from QUERY_CLICKHOUSE_URL before passing it to the ClickHouse client within the initializeQueryClickhouseClient function. The change brings the query client behavior into alignment with the existing main and logs ClickHouse clients, preventing a startup crash caused by unknown URL parameters. The fix consists of a documentation file and a three-line code modification that removes the parameter from the URL. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Hi @edosrecki, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Remove secure param | ||
| url.searchParams.delete("secure"); |
There was a problem hiding this comment.
🚩 Pre-existing: runs replication ClickHouse clients also missing secure param stripping
While this PR correctly fixes the missing secure param deletion for the query ClickHouse client, two other call sites have the same pre-existing issue:
apps/webapp/app/services/runsReplicationInstance.server.ts:25-26passesenv.RUN_REPLICATION_CLICKHOUSE_URLdirectly tonew ClickHouse()without strippingsecure.apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts:80-81does the same.
These use a different env var (RUN_REPLICATION_CLICKHOUSE_URL) which may or may not contain the secure query parameter depending on the deployment. If it does, these clients would also hit the same startup crash described in this PR's changelog. Worth a follow-up fix for consistency.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Closes #3184
✅ Checklist
Changelog
The
initializeQueryClickhouseClient()function was missing theurl.searchParams.delete("secure")call that the other two sibling ClickHouse client init functions already had. This caused a startup crash (Error: Unknown URL parameters: secure) when QUERY_CLICKHOUSE_URL fell back to CLICKHOUSE_URL which contains?secure=false.