Skip to content

chore: init basic ts repo #664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 27, 2025
Merged

chore: init basic ts repo #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c472ebc
chore: init basic ts repo
adityachoudhari26 Aug 26, 2025
f1deb53
add db implemetation
adityachoudhari26 Aug 26, 2025
b0b0c4d
deps fix
adityachoudhari26 Aug 26, 2025
20ae829
Remove RepositoryWithID
adityachoudhari26 Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/event-queue/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"with-env": "dotenv -e ../../.env --"
},
"dependencies": {
"@ctrlplane/db": "workspace:*",
"@ctrlplane/events": "workspace:*",
"@ctrlplane/logger": "workspace:*",
"@t3-oss/env-core": "catalog:",
Expand Down
65 changes: 65 additions & 0 deletions apps/event-queue/src/repository/db-resource-repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import type { Tx } from "@ctrlplane/db";

import { and, eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";

import type { Repository } from "./repository.js";

export class DbResourceRepository implements Repository<schema.Resource> {
private readonly db: Tx;
private readonly workspaceId: string;
constructor(workspaceId: string, tx?: Tx) {
this.db = tx ?? db;
this.workspaceId = workspaceId;
}

async get(id: string) {
return this.db
.select()
.from(schema.resource)
.where(
and(
eq(schema.resource.id, id),
eq(schema.resource.workspaceId, this.workspaceId),
),
)
.then(takeFirstOrNull);
}
async getAll() {
return this.db
.select()
.from(schema.resource)
.where(eq(schema.resource.workspaceId, this.workspaceId));
}
async create(entity: schema.Resource) {
return this.db
.insert(schema.resource)
.values({ ...entity, workspaceId: this.workspaceId })
.returning()
.then(takeFirst);
}
async update(entity: schema.Resource) {
return this.db
.update(schema.resource)
.set(entity)
.where(eq(schema.resource.id, entity.id))
.returning()
.then(takeFirst);
}
Comment on lines +42 to +49
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Scope update() to workspace and prevent workspaceId/id drift.

  • Current WHERE only matches by id, which can update a record in a different workspace.
  • .set(entity) may overwrite workspaceId (or attempt to update id).

Constrain by workspaceId and force workspaceId in the SET payload; avoid updating id.

Apply this diff:

-  async update(entity: schema.Resource) {
-    return this.db
+  async update(entity: schema.Resource) {
+    const { id, workspaceId: _ignored, ...rest } = entity;
+    return this.db
       .update(schema.resource)
-      .set(entity)
-      .where(eq(schema.resource.id, entity.id))
+      .set({ ...rest, workspaceId: this.workspaceId })
+      .where(
+        and(
+          eq(schema.resource.id, id),
+          eq(schema.resource.workspaceId, this.workspaceId),
+        ),
+      )
       .returning()
       .then(takeFirst);
   }
📝 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.

Suggested change
async update(entity: schema.Resource) {
return this.db
.update(schema.resource)
.set(entity)
.where(eq(schema.resource.id, entity.id))
.returning()
.then(takeFirst);
}
async update(entity: schema.Resource) {
const { id, workspaceId: _ignored, ...rest } = entity;
return this.db
.update(schema.resource)
.set({ ...rest, workspaceId: this.workspaceId })
.where(
and(
eq(schema.resource.id, id),
eq(schema.resource.workspaceId, this.workspaceId),
),
)
.returning()
.then(takeFirst);
}
🤖 Prompt for AI Agents 
In apps/event-queue/src/repository/db-resource-repository.ts around lines 42 to
49, the update() currently matches only by id and blindly .set(entity), which
can modify/attempt to modify id or update a row in another workspace; change the
WHERE to include workspaceId (e.g., where id = entity.id AND workspaceId =
entity.workspaceId), ensure the SET payload never contains id (omit id from the
object being set) and explicitly force workspaceId in the SET payload to
entity.workspaceId so workspaceId cannot drift.

Copy link
Contributor Author

@adityachoudhari26 adityachoudhari26 Aug 26, 2025

Choose a reason for hiding this comment

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

ID is globally unique in our postgres db

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

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

@adityachoudhari26 Thank you for that clarification! If IDs are globally unique in your PostgreSQL database, then you're absolutely right - there's no risk of updating records across different workspaces when matching by ID alone.

My suggestion to add workspace scoping to the WHERE clause is unnecessary overhead in your case. The current approach with eq(schema.resource.id, entity.id) is perfectly safe.

The only remaining consideration would be preventing accidental workspaceId changes in the SET payload, but given your database design, this is more of a defensive programming practice than a critical issue.

✏️ Learnings added 
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

async delete(id: string) {
return this.db
.delete(schema.resource)
.where(eq(schema.resource.id, id))
.returning()
.then(takeFirstOrNull);
}
Comment on lines +50 to +56
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Scope delete() to workspace.

Deleting by id without workspace scoping can remove records across tenants.

Apply this diff:

   async delete(id: string) {
     return this.db
       .delete(schema.resource)
-      .where(eq(schema.resource.id, id))
+      .where(
+        and(
+          eq(schema.resource.id, id),
+          eq(schema.resource.workspaceId, this.workspaceId),
+        ),
+      )
       .returning()
       .then(takeFirstOrNull);
   }
📝 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.

Suggested change
async delete(id: string) {
return this.db
.delete(schema.resource)
.where(eq(schema.resource.id, id))
.returning()
.then(takeFirstOrNull);
}
async delete(id: string) {
return this.db
.delete(schema.resource)
.where(
and(
eq(schema.resource.id, id),
eq(schema.resource.workspaceId, this.workspaceId),
),
)
.returning()
.then(takeFirstOrNull);
}

Copy link
Contributor Author

@adityachoudhari26 adityachoudhari26 Aug 26, 2025

Choose a reason for hiding this comment

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

ID is globally unique

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

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

@adityachoudhari26 Thank you for the clarification! You're absolutely right. Since resource IDs are globally unique across the entire database, workspace scoping in the WHERE clause is indeed unnecessary for the delete() method. My suggestion was based on the incorrect assumption that IDs might only be unique within workspace boundaries.

The current implementation is correct as-is - querying by a globally unique ID will never accidentally affect resources from other workspaces.

I appreciate the correction and will keep this architectural decision in mind for future reviews of this codebase.

🐰
✨ Ah, learning something new!
( ◡ ◡ )
\ ▽ /
-----

🧠 Learnings used 
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.

async exists(id: string) {
return this.db
.select()
.from(schema.resource)
.where(eq(schema.resource.id, id))
.then(takeFirstOrNull)
.then((r) => r != null);
}
Comment on lines +57 to +64
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Scope exists() to workspace and avoid selecting all columns.

exists() should be tenant-safe; also prefer selecting a minimal column.

Apply this diff:

   async exists(id: string) {
     return this.db
-      .select()
+      .select({ id: schema.resource.id })
       .from(schema.resource)
-      .where(eq(schema.resource.id, id))
+      .where(
+        and(
+          eq(schema.resource.id, id),
+          eq(schema.resource.workspaceId, this.workspaceId),
+        ),
+      )
       .then(takeFirstOrNull)
       .then((r) => r != null);
   }
📝 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.

Suggested change
async exists(id: string) {
return this.db
.select()
.from(schema.resource)
.where(eq(schema.resource.id, id))
.then(takeFirstOrNull)
.then((r) => r != null);
}
async exists(id: string) {
return this.db
.select({ id: schema.resource.id })
.from(schema.resource)
.where(
and(
eq(schema.resource.id, id),
eq(schema.resource.workspaceId, this.workspaceId),
),
)
.then(takeFirstOrNull)
.then((r) => r != null);
}
🤖 Prompt for AI Agents 
In apps/event-queue/src/repository/db-resource-repository.ts around lines 57 to
64, the exists() query currently selects all columns and is not scoped to the
workspace/tenant; update it to only select the minimal column (resource id) and
add a where clause restricting results to the current workspace (e.g., where
eq(schema.resource.workspaceId, this.workspaceId) or the appropriate workspace
id field on the resource), then return whether a row exists — this makes the
check tenant-safe and avoids fetching unnecessary columns.

Copy link
Contributor Author

@adityachoudhari26 adityachoudhari26 Aug 26, 2025

Choose a reason for hiding this comment

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

ID is globally unique

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

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

@adityachoudhari26, thank you for the clarification! You're absolutely right - I have this noted in my knowledge base from our previous interaction that resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping is not necessary when querying by ID.

My suggestion to add workspace scoping to the exists() method was incorrect. The current implementation is already correct in terms of scoping. The only potential optimization would be selecting minimal columns (select({ id: schema.resource.id }) instead of select()), but that's a minor performance consideration and not a correctness issue.

(つ˶ᵔ ᵕ ᵔ˶)つ curious rabbit learning

🧠 Learnings used 
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.

Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.

}
10 changes: 10 additions & 0 deletions apps/event-queue/src/repository/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
type Entity = { id: string };

export interface Repository<T extends Entity> {
get(id: string): Promise<T | null> | T | null;
getAll(): Promise<T[]> | T[];
create(entity: T): Promise<T> | T;
update(entity: T): Promise<T> | T;
delete(id: string): Promise<T | null> | T | null;
exists(id: string): Promise<boolean> | boolean;
}
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading