From 67ab1607dcf71eb0ac6c1cfa0749c5f82dea5d72 Mon Sep 17 00:00:00 2001 From: Fabis Date: Tue, 1 Oct 2024 15:28:02 +0100 Subject: [PATCH] chore(server): core IoC 18 - updateCommitAndNotifyFactory --- .../modules/core/domain/commits/operations.ts | 27 ++- .../modules/core/domain/commits/types.ts | 3 + .../modules/core/graph/resolvers/commits.js | 31 ++- .../modules/core/graph/resolvers/versions.ts | 33 ++- packages/server/modules/core/loaders.ts | 3 +- .../modules/core/repositories/commits.ts | 85 ++++---- .../core/services/commit/management.ts | 203 +++++++++--------- .../server/modules/core/services/commits.js | 9 - .../server/modules/core/tests/commits.spec.js | 45 +++- 9 files changed, 273 insertions(+), 166 deletions(-) diff --git a/packages/server/modules/core/domain/commits/operations.ts b/packages/server/modules/core/domain/commits/operations.ts index eae40d20b..7d31c47ce 100644 --- a/packages/server/modules/core/domain/commits/operations.ts +++ b/packages/server/modules/core/domain/commits/operations.ts @@ -1,8 +1,13 @@ import { BranchCommit, CommitWithStreamBranchMetadata, - Commit + Commit, + CommitBranch } from '@/modules/core/domain/commits/types' +import { + CommitUpdateInput, + UpdateVersionInput +} from '@/modules/core/graph/generated/graphql' import { BranchCommitRecord, StreamCommitRecord } from '@/modules/core/helpers/types' import { MaybeNullOrUndefined, Nullable, Optional } from '@speckle/shared' import { Knex } from 'knex' @@ -86,3 +91,23 @@ export type InsertStreamCommits = ( trx: Knex.Transaction }> ) => Promise + +export type UpdateCommitAndNotify = ( + params: CommitUpdateInput | UpdateVersionInput, + userId: string +) => Promise + +export type GetCommitBranches = (commitIds: string[]) => Promise + +export type GetCommitBranch = (commitId: string) => Promise> + +export type SwitchCommitBranch = ( + commitId: string, + newBranchId: string, + oldBranchId?: string +) => Promise + +export type UpdateCommit = ( + commitId: string, + commit: Partial +) => Promise diff --git a/packages/server/modules/core/domain/commits/types.ts b/packages/server/modules/core/domain/commits/types.ts index f0dc3d5e5..c3bcdd7ab 100644 --- a/packages/server/modules/core/domain/commits/types.ts +++ b/packages/server/modules/core/domain/commits/types.ts @@ -1,3 +1,4 @@ +import { Branch } from '@/modules/core/domain/branches/types' import { CommitRecord } from '@/modules/core/helpers/types' export type Commit = CommitRecord @@ -11,3 +12,5 @@ export type CommitWithStreamBranchMetadata = Commit & { branchId: string branchName: string } + +export type CommitBranch = Branch & { commitId: string } diff --git a/packages/server/modules/core/graph/resolvers/commits.js b/packages/server/modules/core/graph/resolvers/commits.js index 51084315c..000e61b32 100644 --- a/packages/server/modules/core/graph/resolvers/commits.js +++ b/packages/server/modules/core/graph/resolvers/commits.js @@ -16,11 +16,11 @@ const { getPaginatedBranchCommits } = require('@/modules/core/services/commit/retrieval') const { - updateCommitAndNotify, markCommitReceivedAndNotify, deleteCommitAndNotifyFactory, createCommitByBranchIdFactory, - createCommitByBranchNameFactory + createCommitByBranchNameFactory, + updateCommitAndNotifyFactory } = require('@/modules/core/services/commit/management') const { RateLimitError } = require('@/modules/core/errors/ratelimit') @@ -44,10 +44,17 @@ const { deleteCommitFactory, createCommitFactory, insertStreamCommitsFactory, - insertBranchCommitsFactory + insertBranchCommitsFactory, + getCommitBranchFactory, + switchCommitBranchFactory, + updateCommitFactory } = require('@/modules/core/repositories/commits') const { db } = require('@/db/knex') -const { markCommitStreamUpdated } = require('@/modules/core/repositories/streams') +const { + markCommitStreamUpdated, + getStream, + getCommitStream +} = require('@/modules/core/repositories/streams') const { markCommitBranchUpdatedFactory, getBranchByIdFactory, @@ -55,7 +62,8 @@ const { } = require('@/modules/core/repositories/branches') const { addCommitDeletedActivity, - addCommitCreatedActivity + addCommitCreatedActivity, + addCommitUpdatedActivity } = require('@/modules/activitystream/services/commitActivity') const { getObject } = require('@/modules/core/repositories/objects') const { VersionsEmitter } = require('@/modules/core/events/versionsEmitter') @@ -91,6 +99,19 @@ const createCommitByBranchName = createCommitByBranchNameFactory({ getBranchById: getBranchByIdFactory({ db }) }) +const updateCommitAndNotify = updateCommitAndNotifyFactory({ + getCommit: getCommitFactory({ db }), + getStream, + getCommitStream, + getStreamBranchByName: getStreamBranchByNameFactory({ db }), + getCommitBranch: getCommitBranchFactory({ db }), + switchCommitBranch: switchCommitBranchFactory({ db }), + updateCommit: updateCommitFactory({ db }), + addCommitUpdatedActivity, + markCommitStreamUpdated, + markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }) +}) + /** * @param {boolean} publicOnly * @param {string} userId diff --git a/packages/server/modules/core/graph/resolvers/versions.ts b/packages/server/modules/core/graph/resolvers/versions.ts index 697912c67..bed6b0da8 100644 --- a/packages/server/modules/core/graph/resolvers/versions.ts +++ b/packages/server/modules/core/graph/resolvers/versions.ts @@ -14,7 +14,7 @@ import { CommitUpdateError } from '@/modules/core/errors/commit' import { createCommitByBranchIdFactory, markCommitReceivedAndNotify, - updateCommitAndNotify + updateCommitAndNotifyFactory } from '@/modules/core/services/commit/management' import { getRateLimitResult, @@ -23,18 +23,30 @@ import { import { RateLimitError } from '@/modules/core/errors/ratelimit' import { createCommitFactory, + getCommitBranchFactory, + getCommitFactory, insertBranchCommitsFactory, - insertStreamCommitsFactory + insertStreamCommitsFactory, + switchCommitBranchFactory, + updateCommitFactory } from '@/modules/core/repositories/commits' import { db } from '@/db/knex' import { getObject } from '@/modules/core/repositories/objects' import { getBranchByIdFactory, + getStreamBranchByNameFactory, markCommitBranchUpdatedFactory } from '@/modules/core/repositories/branches' -import { markCommitStreamUpdated } from '@/modules/core/repositories/streams' +import { + getCommitStream, + getStream, + markCommitStreamUpdated +} from '@/modules/core/repositories/streams' import { VersionsEmitter } from '@/modules/core/events/versionsEmitter' -import { addCommitCreatedActivity } from '@/modules/activitystream/services/commitActivity' +import { + addCommitCreatedActivity, + addCommitUpdatedActivity +} from '@/modules/activitystream/services/commitActivity' const createCommitByBranchId = createCommitByBranchIdFactory({ createCommit: createCommitFactory({ db }), @@ -48,6 +60,19 @@ const createCommitByBranchId = createCommitByBranchIdFactory({ addCommitCreatedActivity }) +const updateCommitAndNotify = updateCommitAndNotifyFactory({ + getCommit: getCommitFactory({ db }), + getStream, + getCommitStream, + getStreamBranchByName: getStreamBranchByNameFactory({ db }), + getCommitBranch: getCommitBranchFactory({ db }), + switchCommitBranch: switchCommitBranchFactory({ db }), + updateCommit: updateCommitFactory({ db }), + addCommitUpdatedActivity, + markCommitStreamUpdated, + markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }) +}) + export = { Project: { async version(parent, args, ctx) { diff --git a/packages/server/modules/core/loaders.ts b/packages/server/modules/core/loaders.ts index b285b2feb..6d099a731 100644 --- a/packages/server/modules/core/loaders.ts +++ b/packages/server/modules/core/loaders.ts @@ -23,7 +23,7 @@ import { import { Nullable } from '@/modules/shared/helpers/typeHelper' import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' import { - getCommitBranches, + getCommitBranchesFactory, getCommitsFactory, getSpecificBranchCommitsFactory, getStreamCommitCounts, @@ -112,6 +112,7 @@ const getStreamBranchCounts = getStreamBranchCountsFactory({ db }) const getBranchCommitCounts = getBranchCommitCountsFactory({ db }) const getCommits = getCommitsFactory({ db }) const getSpecificBranchCommits = getSpecificBranchCommitsFactory({ db }) +const getCommitBranches = getCommitBranchesFactory({ db }) /** * TODO: Lazy load DataLoaders to reduce memory usage diff --git a/packages/server/modules/core/repositories/commits.ts b/packages/server/modules/core/repositories/commits.ts index 53b56879b..7081aa080 100644 --- a/packages/server/modules/core/repositories/commits.ts +++ b/packages/server/modules/core/repositories/commits.ts @@ -30,7 +30,11 @@ import { GetCommits, GetSpecificBranchCommits, InsertBranchCommits, - InsertStreamCommits + InsertStreamCommits, + GetCommitBranches, + GetCommitBranch, + SwitchCommitBranch, + UpdateCommit } from '@/modules/core/domain/commits/operations' const tables = { @@ -321,47 +325,54 @@ export async function getBranchCommitsTotalCount( return parseInt(res?.count || '0') } -export async function getCommitBranches(commitIds: string[]) { - if (!commitIds?.length) return [] +export const getCommitBranchesFactory = + (deps: { db: Knex }): GetCommitBranches => + async (commitIds: string[]) => { + if (!commitIds?.length) return [] - const q = BranchCommits.knex() - .select>([ - ...Branches.cols, - knex.raw(`?? as "commitId"`, [BranchCommits.col.commitId]) - ]) - .innerJoin(Branches.name, Branches.col.id, BranchCommits.col.branchId) - .whereIn(BranchCommits.col.commitId, commitIds) + const q = tables + .branchCommits(deps.db) + .select>([ + ...Branches.cols, + knex.raw(`?? as "commitId"`, [BranchCommits.col.commitId]) + ]) + .innerJoin(Branches.name, Branches.col.id, BranchCommits.col.branchId) + .whereIn(BranchCommits.col.commitId, commitIds) - return await q -} - -export async function getCommitBranch(commitId: string) { - const [commit] = await getCommitBranches([commitId]) - return commit as Optional -} - -export async function switchCommitBranch( - commitId: string, - newBranchId: string, - oldBranchId?: string -) { - const q = BranchCommits.knex() - .where(BranchCommits.col.commitId, commitId) - .update(BranchCommits.withoutTablePrefix.col.branchId, newBranchId) - - if (oldBranchId) { - q.andWhere(BranchCommits.col.branchId, oldBranchId) + return await q } - await q -} +export const getCommitBranchFactory = + (deps: { db: Knex }): GetCommitBranch => + async (commitId: string) => { + const [commit] = await getCommitBranchesFactory(deps)([commitId]) + return commit as Optional + } -export async function updateCommit(commitId: string, commit: Partial) { - const [newCommit] = (await Commits.knex() - .where(Commits.col.id, commitId) - .update(commit, '*')) as CommitRecord[] - return newCommit -} +export const switchCommitBranchFactory = + (deps: { db: Knex }): SwitchCommitBranch => + async (commitId: string, newBranchId: string, oldBranchId?: string) => { + const q = tables + .branchCommits(deps.db) + .where(BranchCommits.col.commitId, commitId) + .update(BranchCommits.withoutTablePrefix.col.branchId, newBranchId) + + if (oldBranchId) { + q.andWhere(BranchCommits.col.branchId, oldBranchId) + } + + await q + } + +export const updateCommitFactory = + (deps: { db: Knex }): UpdateCommit => + async (commitId: string, commit: Partial) => { + const [newCommit] = (await tables + .commits(deps.db) + .where(Commits.col.id, commitId) + .update(commit, '*')) as CommitRecord[] + return newCommit + } export const createCommitFactory = (deps: { db: Knex }): StoreCommit => diff --git a/packages/server/modules/core/services/commit/management.ts b/packages/server/modules/core/services/commit/management.ts index 6a5205868..79f0c528c 100644 --- a/packages/server/modules/core/services/commit/management.ts +++ b/packages/server/modules/core/services/commit/management.ts @@ -16,9 +16,13 @@ import { DeleteCommit, DeleteCommitAndNotify, GetCommit, + GetCommitBranch, InsertBranchCommits, InsertStreamCommits, - StoreCommit + StoreCommit, + SwitchCommitBranch, + UpdateCommit, + UpdateCommitAndNotify } from '@/modules/core/domain/commits/operations' import { CommitCreateError, @@ -37,16 +41,7 @@ import { UpdateVersionInput } from '@/modules/core/graph/generated/graphql' import { CommitRecord } from '@/modules/core/helpers/types' -import { - getStreamBranchByNameFactory, - markCommitBranchUpdatedFactory -} from '@/modules/core/repositories/branches' -import { - getCommitBranch, - getCommitFactory, - switchCommitBranch, - updateCommit -} from '@/modules/core/repositories/commits' +import { getCommitFactory } from '@/modules/core/repositories/commits' import { getObject } from '@/modules/core/repositories/objects' import { getCommitStream, @@ -255,99 +250,111 @@ const isOldVersionUpdateInput = ( i: CommitUpdateInput | UpdateVersionInput ): i is CommitUpdateInput => has(i, 'streamId') -export async function updateCommitAndNotify( - params: CommitUpdateInput | UpdateVersionInput, - userId: string -) { - const { - message, - newBranchName, - streamId, - id: commitId - } = isOldVersionUpdateInput(params) - ? params - : { - message: params.message, - id: params.versionId, - streamId: null, - newBranchName: null - } +export const updateCommitAndNotifyFactory = + (deps: { + getCommit: GetCommit + getStream: typeof getStream + getCommitStream: typeof getCommitStream + getStreamBranchByName: GetStreamBranchByName + getCommitBranch: GetCommitBranch + switchCommitBranch: SwitchCommitBranch + updateCommit: UpdateCommit + addCommitUpdatedActivity: typeof addCommitUpdatedActivity + markCommitStreamUpdated: typeof markCommitStreamUpdated + markCommitBranchUpdated: MarkCommitBranchUpdated + }): UpdateCommitAndNotify => + async (params: CommitUpdateInput | UpdateVersionInput, userId: string) => { + const { + message, + newBranchName, + streamId, + id: commitId + } = isOldVersionUpdateInput(params) + ? params + : { + message: params.message, + id: params.versionId, + streamId: null, + newBranchName: null + } - if (!message && !newBranchName) { - throw new CommitUpdateError('Nothing to update', { - info: { ...params, userId } - }) - } - - const [commit, stream] = await Promise.all([ - getCommitFactory({ db })(commitId), - streamId ? getStream({ streamId, userId }) : getCommitStream({ commitId, userId }) - ]) - if (!commit) { - throw new CommitUpdateError("Can't update nonexistant commit", { - info: { ...params, userId } - }) - } - if (!stream) { - throw new CommitUpdateError("Can't resolve commit stream", { - info: { ...params, userId } - }) - } - if (commit.author !== userId && stream.role !== Roles.Stream.Owner) { - throw new CommitUpdateError( - 'Only the author of a commit or a stream owner may update it', - { - info: { ...params, userId, streamRole: stream.role } - } - ) - } - - if (newBranchName) { - try { - const [newBranch, oldBranch] = await Promise.all([ - getStreamBranchByNameFactory({ db })(streamId, newBranchName), - getCommitBranch(commitId) - ]) - - if (!newBranch || !oldBranch) { - throw new Error("Couldn't resolve branch") - } - if (!commit) { - throw new Error("Couldn't find commit") - } - - await switchCommitBranch(commitId, newBranch.id, oldBranch.id) - } catch (e) { - throw new CommitUpdateError('Failed to update commit branch', { - cause: ensureError(e), - info: params + if (!message && !newBranchName) { + throw new CommitUpdateError('Nothing to update', { + info: { ...params, userId } }) } - } - let newCommit: CommitRecord = commit - if (message) { - newCommit = await updateCommit(commitId, { message }) - } - - if (commit) { - await addCommitUpdatedActivity({ - commitId, - streamId: stream.id, - userId, - originalCommit: commit, - update: params, - newCommit - }) - - await Promise.all([ - markCommitStreamUpdated(commit.id), - markCommitBranchUpdatedFactory({ db })(commit.id) + const [commit, stream] = await Promise.all([ + deps.getCommit(commitId), + streamId + ? deps.getStream({ streamId, userId }) + : deps.getCommitStream({ commitId, userId }) ]) - } + if (!commit) { + throw new CommitUpdateError("Can't update nonexistant commit", { + info: { ...params, userId } + }) + } + if (!stream) { + throw new CommitUpdateError("Can't resolve commit stream", { + info: { ...params, userId } + }) + } + if (commit.author !== userId && stream.role !== Roles.Stream.Owner) { + throw new CommitUpdateError( + 'Only the author of a commit or a stream owner may update it', + { + info: { ...params, userId, streamRole: stream.role } + } + ) + } - return newCommit -} + if (newBranchName) { + try { + const [newBranch, oldBranch] = await Promise.all([ + deps.getStreamBranchByName(streamId, newBranchName), + deps.getCommitBranch(commitId) + ]) + + if (!newBranch || !oldBranch) { + throw new Error("Couldn't resolve branch") + } + if (!commit) { + throw new Error("Couldn't find commit") + } + + await deps.switchCommitBranch(commitId, newBranch.id, oldBranch.id) + } catch (e) { + throw new CommitUpdateError('Failed to update commit branch', { + cause: ensureError(e), + info: params + }) + } + } + + let newCommit: CommitRecord = commit + if (message) { + newCommit = await deps.updateCommit(commitId, { message }) + } + + if (commit) { + await deps.addCommitUpdatedActivity({ + commitId, + streamId: stream.id, + userId, + originalCommit: commit, + update: params, + newCommit + }) + + await Promise.all([ + deps.markCommitStreamUpdated(commit.id), + deps.markCommitBranchUpdated(commit.id) + ]) + } + + return newCommit + } export const deleteCommitAndNotifyFactory = (deps: { diff --git a/packages/server/modules/core/services/commits.js b/packages/server/modules/core/services/commits.js index faf975d3b..8044c6281 100644 --- a/packages/server/modules/core/services/commits.js +++ b/packages/server/modules/core/services/commits.js @@ -10,7 +10,6 @@ const { getPaginatedBranchCommits, getBranchCommitsTotalCount } = require('@/modules/core/repositories/commits') -const { updateCommitAndNotify } = require('@/modules/core/services/commit/management') const { clamp } = require('lodash') const getCommitsByUserIdBase = ({ userId, publicOnly, streamIdWhitelist }) => { @@ -47,14 +46,6 @@ const getCommitsByUserIdBase = ({ userId, publicOnly, streamIdWhitelist }) => { } module.exports = { - /** - * @deprecated Use 'updateCommitAndNotify()' - */ - async updateCommit({ streamId, id, message, newBranchName, userId }) { - await updateCommitAndNotify({ streamId, id, message, newBranchName }, userId) - return true - }, - /** * @deprecated Use `getBranchCommitsTotalCount()` instead */ diff --git a/packages/server/modules/core/tests/commits.spec.js b/packages/server/modules/core/tests/commits.spec.js index 9935be106..db97fd808 100644 --- a/packages/server/modules/core/tests/commits.spec.js +++ b/packages/server/modules/core/tests/commits.spec.js @@ -8,7 +8,6 @@ const { createStream } = require('../services/streams') const { createObject } = require('../services/objects') const { - updateCommit, getCommitsTotalCountByBranchName, getCommitsByBranchName, getCommitsByStreamId, @@ -34,17 +33,26 @@ const { deleteCommitFactory, createCommitFactory, insertStreamCommitsFactory, - insertBranchCommitsFactory + insertBranchCommitsFactory, + getCommitBranchFactory, + switchCommitBranchFactory, + updateCommitFactory } = require('@/modules/core/repositories/commits') const { deleteCommitAndNotifyFactory, createCommitByBranchIdFactory, - createCommitByBranchNameFactory + createCommitByBranchNameFactory, + updateCommitAndNotifyFactory } = require('@/modules/core/services/commit/management') -const { markCommitStreamUpdated } = require('@/modules/core/repositories/streams') +const { + markCommitStreamUpdated, + getCommitStream, + getStream +} = require('@/modules/core/repositories/streams') const { addCommitDeletedActivity, - addCommitCreatedActivity + addCommitCreatedActivity, + addCommitUpdatedActivity } = require('@/modules/activitystream/services/commitActivity') const { getObject } = require('@/modules/core/repositories/objects') const { VersionsEmitter } = require('@/modules/core/events/versionsEmitter') @@ -82,6 +90,19 @@ const createCommitByBranchName = createCommitByBranchNameFactory({ getBranchById: getBranchByIdFactory({ db }) }) +const updateCommitAndNotify = updateCommitAndNotifyFactory({ + getCommit: getCommitFactory({ db }), + getStream, + getCommitStream, + getStreamBranchByName: getStreamBranchByNameFactory({ db }), + getCommitBranch: getCommitBranchFactory({ db }), + switchCommitBranch: switchCommitBranchFactory({ db }), + updateCommit: updateCommitFactory({ db }), + addCommitUpdatedActivity, + markCommitStreamUpdated, + markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }) +}) + describe('Commits @core-commits', () => { const user = { name: 'Dimitrie Stefanescu', @@ -255,12 +276,14 @@ describe('Commits @core-commits', () => { }) it('Should update a commit', async () => { - const res = await updateCommit({ - id: commitId1, - message: 'FIRST COMMIT YOOOOOO', - userId: user.id, - streamId: stream.id - }) + const res = await updateCommitAndNotify( + { + id: commitId1, + message: 'FIRST COMMIT YOOOOOO', + streamId: stream.id + }, + user.id + ) expect(res).to.equal(true) })