Conversation
…t-github-app---internal-server-error fix: handle optional chaining for organization and user IDs in GitHub…
…pose-file-not-found-when-no-domains-configured refactor: simplify domain handling in Docker compose utility functions
…and and real tests
Fix/improve loading queries
dosubot
bot
added
the
size:L
|
Related Documentation Checked 7 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { dbUrl } from "@dokploy/server/db/constants"; | ||
| import * as schema from "@dokploy/server/db/schema"; | ||
| import { and, eq } from "drizzle-orm"; | ||
| import { drizzle, type PostgresJsDatabase } from "drizzle-orm/postgres-js"; | ||
| import postgres from "postgres"; | ||
|
|
||
| export { and, eq }; | ||
|
|
||
| type Database = PostgresJsDatabase<typeof schema>; | ||
| /** | ||
| * Evita problemas de redeclaración global en monorepos. | ||
| * No usamos `declare global`. | ||
| */ | ||
| const globalForDb = globalThis as unknown as { | ||
| db?: Database; | ||
| }; | ||
|
|
||
| let dbConnection: Database; | ||
|
|
||
| if (process.env.NODE_ENV === "production") { | ||
| // En producción no usamos global cache | ||
| dbConnection = drizzle(postgres(dbUrl), { | ||
| schema, | ||
| }); | ||
| } else { | ||
| // En desarrollo reutilizamos conexión para evitar múltiples conexiones | ||
| if (!globalForDb.db) { | ||
| globalForDb.db = drizzle(postgres(dbUrl), { | ||
| schema, | ||
| }); | ||
| } | ||
|
|
||
| dbConnection = globalForDb.db; | ||
| } | ||
|
|
||
| export const db: Database = dbConnection; | ||
|
|
||
| export { dbUrl }; |
There was a problem hiding this comment.
Duplicate database connection pool
This new file is nearly identical to packages/server/src/db/index.ts and will create a second, independent postgres(dbUrl) connection pool pointing to the same database. In a monorepo, if both apps/dokploy and packages/server load their respective db/index.ts, you'll have two separate connection pools in the same process, which can double the number of open database connections and risk hitting PostgreSQL's max_connections limit.
Additionally, packages/server/src/db/index.ts exports the schema via export * from "./schema", whereas this new file does not. If any code that previously imported from @dokploy/server/db is redirected to this local file (e.g., via a tsconfig path alias), those schema type imports would silently break.
If the intent is to fix the HMR duplicate-connection issue in Next.js development mode, consider reusing the globalForDb singleton from the shared package rather than duplicating the entire module.
| const TRUSTED_ORIGINS_CACHE_TTL_MS = 30 * 60_000; | ||
| let trustedOriginsCache: { data: string[]; expiresAt: number } | null = null; | ||
|
|
||
| export const getTrustedOrigins = async () => { | ||
| const members = await db.query.member.findMany({ | ||
| where: eq(member.role, "owner"), | ||
| with: { | ||
| user: true, | ||
| }, | ||
| }); | ||
| const runQuery = async () => { | ||
| const rows = await db | ||
| .select({ trustedOrigins: user.trustedOrigins }) | ||
| .from(member) | ||
| .innerJoin(user, eq(member.userId, user.id)) | ||
| .where(eq(member.role, "owner")); | ||
| return Array.from(new Set(rows.flatMap((r) => r.trustedOrigins ?? []))); | ||
| }; | ||
|
|
||
| if (members.length === 0) { | ||
| return []; | ||
| if (IS_CLOUD) { | ||
| const now = Date.now(); | ||
| if (trustedOriginsCache && now < trustedOriginsCache.expiresAt) { | ||
| return trustedOriginsCache.data; | ||
| } | ||
| const trustedOrigins = await runQuery(); | ||
| trustedOriginsCache = { | ||
| data: trustedOrigins, | ||
| expiresAt: now + TRUSTED_ORIGINS_CACHE_TTL_MS, | ||
| }; | ||
| return trustedOrigins; | ||
| } | ||
|
|
||
| const trustedOrigins = members.flatMap( | ||
| (member) => member.user.trustedOrigins || [], | ||
| ); | ||
|
|
||
| return Array.from(new Set(trustedOrigins)); | ||
| return runQuery(); | ||
| }; |
There was a problem hiding this comment.
Stale cache with no invalidation path for trusted origins
The new 30-minute in-memory cache for IS_CLOUD is a good performance improvement, but trustedOriginsCache has no invalidation mechanism. If an owner's trustedOrigins are removed or changed (e.g., a user revokes a previously trusted domain), the old origins will continue to be accepted for up to 30 minutes. From a security standpoint, this means a revoked origin remains trusted throughout the cache TTL.
Consider either:
- Exporting a
clearTrustedOriginsCache()function and calling it from the relevant user-update service, or - Shortening the TTL for security-sensitive operations like origin trust validation.
Additional Comments (1)
Before this PR, if (!domains.length) {
return "";
}This made the call a safe no-op when no domains were configured. Now that the guard is removed (and the matching
Callers that passed an empty |
1 participant
This PR promotes changes from
canarytomainfor version v0.28.2.🔍 Changes Include:
✅ Pre-merge Checklist:
Greptile Summary
This release PR (v0.28.2) promotes a set of canary fixes to main, covering GitHub OAuth hardening, a debug log cleanup, test mock updates, a
getTrustedOriginsrefactor with cloud caching, and a compose domain-writing behavioral change.Key changes:
undefinedfrom appearing in OAuth redirect URLs, and missingactiveOrganization/sessionvalues now disable the setup button.getTrustedOriginsrefactor: Switches from the ORM relational API to a raw Drizzle join query and introduces a 30-minute in-memory cache for cloud mode. The cache has no explicit invalidation path, which could leave revoked trusted origins active for up to 30 minutes.domainsis empty. Deployments with zero configured domains will now attempt to load and modify the compose file rather than silently skipping, which can cause a deployment failure if the compose file doesn't exist at that point in the pipeline.apps/dokploy/server/db/index.tsis added that mirrorspackages/server/src/db/index.ts, potentially creating two independent PostgreSQL connection pools and omitting the schema re-export present in the original.Confidence Score: 2/5
writeDomainsToComposebehavior change (no-op → error/modify when domains is empty) and the duplicatedb/index.tscreating a second connection pool are both substantive correctness risks that warrant verification before merging to main. The remaining changes (OAuth URL fix, log cleanup, test mocks, parallel fetch optimization) are clean and safe.packages/server/src/utils/docker/domain.ts(behavior change on empty domains) andapps/dokploy/server/db/index.ts(duplicate DB connection pool).Last reviewed commit: 8d56544