feature: tenancy #51
feature: tenancy #51
Conversation
| async getProjectById(id: number) { | ||
| const result = await this.db | ||
| .select() | ||
| .from(this.scope) |
geclos
force-pushed
the
feature/tenancy
branch
12 times, most recently
from
1ba3456 to
91afaac
Compare
| const document = await getDocumentByPath({ | ||
| commitId: input.commitId, | ||
| commit, |
There was a problem hiding this comment.
as a general rule make all db readers/writers receive objects instead of identifiers. This will force the consumer to ensure tenancy and will reduce the amount of redundants roundtrips to the database. It results in fatter controllers but that is ok, tenancy is ensured at the entrypoints of our backend.
| projectId: input.projectId, | ||
| workspaceId: ctx.workspace.id, | ||
| }) | ||
| await projectScope.getProjectById(input.projectId) |
There was a problem hiding this comment.
Would be nice that this is a class call it find when is by ID
There was a problem hiding this comment.
🤷🏼 can change if we prefer it
| .handler(async ({ input, ctx }) => { | ||
| const commitsScope = new CommitsRepository(ctx.project.workspaceId) | ||
| const commit = await commitsScope | ||
| .getCommitById(input.commitId) | ||
| .then((r) => r.unwrap()) |
There was a problem hiding this comment.
Why do we need to use the workspaceId to get the commitScope? Shouldn't we use the commitId instead?
There was a problem hiding this comment.
nein, commitId alone is not scoped by tenant, you have to first scope by tenant (the workspace) before you can do any search of commits
| @@ -60,7 +57,7 @@ export async function getCurrentUserFromDB({ | |||
| }: { | |||
| userId: string | undefined | |||
| }): PromisedResult<SessionData, NotFoundError> { | |||
| const user = await getUser(userId) | |||
| const user = await unsafelyGetUser(userId) | |||
There was a problem hiding this comment.
From the PR description:
All data access should happen via repositories so to ensure tenancy. In the cases that tenancy must not be enforced data accessors have been prepended with unsafely to make things clear
| "eslint": "9.x" | ||
| "react": ">=18.x", | ||
| "react-dom": ">=18.x", | ||
| "eslint": ">=8.x" |
| .where(eq(this.scope.id, documentId)) | ||
|
|
||
| // NOTE: I hate this | ||
| const document = res[0] |
There was a problem hiding this comment.
this.db.documentVersions.findFirst({
where: eq(this.scope.id, documentId)
})There was a problem hiding this comment.
nein, that's one of the negative effects of using subqueries you cannot use drizzle query dsl
| documentUuid: documentVersions.documentUuid, | ||
| mergedAt: max(commits.mergedAt).as('maxMergedAt'), | ||
| }) | ||
| .from(documentVersions) |
There was a problem hiding this comment.
This is not using the scope no?
| const documentsFromMergedCommits = await this.db | ||
| .with(lastVersionOfEachDocument) | ||
| .select(getTableColumns(documentVersions)) | ||
| .from(documentVersions) |
There was a problem hiding this comment.
This is also not using the scope I think
There was a problem hiding this comment.
Please don't fix it before I merge my PR. I'm sick of resolving conflicts 🙏
Refactor data accessors to repositories which enforce tenancy. There's two guiding principles:
unsafelyto make things clearservicesfolder – should receive object instances instead of object ids. This is to ensure consumers have to enforce tenancy before writing to database.