diff --git a/packages/server/modules/core/domain/commits/operations.ts b/packages/server/modules/core/domain/commits/operations.ts index c477b039b..3c71a3d13 100644 --- a/packages/server/modules/core/domain/commits/operations.ts +++ b/packages/server/modules/core/domain/commits/operations.ts @@ -1,3 +1,4 @@ +import { Branch } from '@/modules/core/domain/branches/types' import { BranchCommit, CommitWithStreamBranchMetadata, @@ -5,8 +6,10 @@ import { CommitBranch } from '@/modules/core/domain/commits/types' import { + CommitsMoveInput, CommitUpdateInput, ModelVersionsFilter, + MoveVersionsInput, UpdateVersionInput } from '@/modules/core/graph/generated/graphql' import { BranchCommitRecord, StreamCommitRecord } from '@/modules/core/helpers/types' @@ -208,3 +211,13 @@ export type GetPaginatedBranchCommits = ( items: Commit[] cursor: string | null }> + +export type MoveCommitsToBranch = ( + commitIds: string[], + branchId: string +) => Promise + +export type ValidateAndBatchMoveCommits = ( + params: CommitsMoveInput | MoveVersionsInput, + userId: string +) => Promise diff --git a/packages/server/modules/core/graph/resolvers/commits.js b/packages/server/modules/core/graph/resolvers/commits.js index 51bf47462..061b220ef 100644 --- a/packages/server/modules/core/graph/resolvers/commits.js +++ b/packages/server/modules/core/graph/resolvers/commits.js @@ -29,8 +29,8 @@ const { getRateLimitResult } = require('@/modules/core/services/ratelimiter') const { - batchMoveCommits, - batchDeleteCommits + batchDeleteCommits, + batchMoveCommitsFactory } = require('@/modules/core/services/commit/batchCommitActions') const { validateStreamAccess @@ -50,23 +50,28 @@ const { updateCommitFactory, getSpecificBranchCommitsFactory, getPaginatedBranchCommitsItemsFactory, - getBranchCommitsTotalCountFactory + getBranchCommitsTotalCountFactory, + getCommitsFactory, + moveCommitsToBranchFactory } = require('@/modules/core/repositories/commits') const { db } = require('@/db/knex') const { markCommitStreamUpdated, getStream, - getCommitStream + getCommitStream, + getStreams } = require('@/modules/core/repositories/streams') const { markCommitBranchUpdatedFactory, getBranchByIdFactory, - getStreamBranchByNameFactory + getStreamBranchByNameFactory, + createBranchFactory } = require('@/modules/core/repositories/branches') const { addCommitDeletedActivity, addCommitCreatedActivity, - addCommitUpdatedActivity + addCommitUpdatedActivity, + addCommitMovedActivity } = require('@/modules/activitystream/services/commitActivity') const { getObject } = require('@/modules/core/repositories/objects') const { VersionsEmitter } = require('@/modules/core/events/versionsEmitter') @@ -121,6 +126,15 @@ const getPaginatedBranchCommits = getPaginatedBranchCommitsFactory({ getBranchCommitsTotalCount: getBranchCommitsTotalCountFactory({ db }) }) +const batchMoveCommits = batchMoveCommitsFactory({ + getCommits: getCommitsFactory({ db }), + getStreams, + getStreamBranchByName: getStreamBranchByNameFactory({ db }), + createBranch: createBranchFactory({ db }), + moveCommitsToBranch: moveCommitsToBranchFactory({ db }), + addCommitMovedActivity +}) + /** * @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 bed6b0da8..28aea20ec 100644 --- a/packages/server/modules/core/graph/resolvers/versions.ts +++ b/packages/server/modules/core/graph/resolvers/versions.ts @@ -8,7 +8,7 @@ import { import { getServerOrigin } from '@/modules/shared/helpers/envHelper' import { batchDeleteCommits, - batchMoveCommits + batchMoveCommitsFactory } from '@/modules/core/services/commit/batchCommitActions' import { CommitUpdateError } from '@/modules/core/errors/commit' import { @@ -25,14 +25,17 @@ import { createCommitFactory, getCommitBranchFactory, getCommitFactory, + getCommitsFactory, insertBranchCommitsFactory, insertStreamCommitsFactory, + moveCommitsToBranchFactory, switchCommitBranchFactory, updateCommitFactory } from '@/modules/core/repositories/commits' import { db } from '@/db/knex' import { getObject } from '@/modules/core/repositories/objects' import { + createBranchFactory, getBranchByIdFactory, getStreamBranchByNameFactory, markCommitBranchUpdatedFactory @@ -40,11 +43,13 @@ import { import { getCommitStream, getStream, + getStreams, markCommitStreamUpdated } from '@/modules/core/repositories/streams' import { VersionsEmitter } from '@/modules/core/events/versionsEmitter' import { addCommitCreatedActivity, + addCommitMovedActivity, addCommitUpdatedActivity } from '@/modules/activitystream/services/commitActivity' @@ -73,6 +78,15 @@ const updateCommitAndNotify = updateCommitAndNotifyFactory({ markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }) }) +const batchMoveCommits = batchMoveCommitsFactory({ + getCommits: getCommitsFactory({ db }), + getStreams, + getStreamBranchByName: getStreamBranchByNameFactory({ db }), + createBranch: createBranchFactory({ db }), + moveCommitsToBranch: moveCommitsToBranchFactory({ db }), + addCommitMovedActivity +}) + export = { Project: { async version(parent, args, ctx) { diff --git a/packages/server/modules/core/repositories/commits.ts b/packages/server/modules/core/repositories/commits.ts index 3a6edb3e0..8211a859c 100644 --- a/packages/server/modules/core/repositories/commits.ts +++ b/packages/server/modules/core/repositories/commits.ts @@ -48,7 +48,8 @@ import { PaginatedBranchCommitsBaseParams, PaginatedBranchCommitsParams, GetPaginatedBranchCommitsItems, - GetBranchCommitsTotalCount + GetBranchCommitsTotalCount, + MoveCommitsToBranch } from '@/modules/core/domain/commits/operations' const tables = { @@ -107,25 +108,30 @@ export const getCommitFactory = * same stream etc. THIS DOESN'T DO ANY VALIDATION! * @returns The amount of commits that were moved */ -export async function moveCommitsToBranch(commitIds: string[], branchId: string) { - if (!commitIds?.length) return +export const moveCommitsToBranchFactory = + (deps: { db: Knex }): MoveCommitsToBranch => + async (commitIds: string[], branchId: string) => { + if (!commitIds?.length) return - // delete old branch commits - await BranchCommits.knex().whereIn(BranchCommits.col.commitId, commitIds).del() + // delete old branch commits + await tables + .branchCommits(deps.db) + .whereIn(BranchCommits.col.commitId, commitIds) + .del() - // insert new ones - const inserts = await BranchCommits.knex().insert( - commitIds.map( - (cId): BranchCommitRecord => ({ - branchId, - commitId: cId - }) - ), - '*' - ) + // insert new ones + const inserts = await tables.branchCommits(deps.db).insert( + commitIds.map( + (cId): BranchCommitRecord => ({ + branchId, + commitId: cId + }) + ), + '*' + ) - return inserts.length -} + return inserts.length + } export const deleteCommitsFactory = (deps: { db: Knex }): DeleteCommits => diff --git a/packages/server/modules/core/services/commit/batchCommitActions.ts b/packages/server/modules/core/services/commit/batchCommitActions.ts index bf9a1611e..83bac46f4 100644 --- a/packages/server/modules/core/services/commit/batchCommitActions.ts +++ b/packages/server/modules/core/services/commit/batchCommitActions.ts @@ -3,6 +3,15 @@ import { addCommitDeletedActivity, addCommitMovedActivity } from '@/modules/activitystream/services/commitActivity' +import { + GetStreamBranchByName, + StoreBranch +} from '@/modules/core/domain/branches/operations' +import { + GetCommits, + MoveCommitsToBranch, + ValidateAndBatchMoveCommits +} from '@/modules/core/domain/commits/operations' import { CommitInvalidAccessError, CommitBatchUpdateError @@ -14,14 +23,9 @@ import { MoveVersionsInput } from '@/modules/core/graph/generated/graphql' import { Roles } from '@/modules/core/helpers/mainConstants' -import { - createBranchFactory, - getStreamBranchByNameFactory -} from '@/modules/core/repositories/branches' import { deleteCommitsFactory, - getCommitsFactory, - moveCommitsToBranch + getCommitsFactory } from '@/modules/core/repositories/commits' import { getStreams } from '@/modules/core/repositories/streams' import { ensureError } from '@/modules/shared/helpers/errorHelper' @@ -32,94 +36,107 @@ type CommitBatchInput = OldBatchInput | MoveVersionsInput | DeleteVersionsInput const isOldBatchInput = (i: CommitBatchInput): i is OldBatchInput => has(i, 'commitIds') +type ValidateBatchBaseRulesDeps = { + getCommits: GetCommits + getStreams: typeof getStreams +} + /** * Do base validation that's going to be appropriate for all batch actions and return * the DB entities that were tested */ -async function validateBatchBaseRules(params: CommitBatchInput, userId: string) { - const commitIds = isOldBatchInput(params) ? params.commitIds : params.versionIds +const validateBatchBaseRulesFactory = + (deps: ValidateBatchBaseRulesDeps) => + async (params: CommitBatchInput, userId: string) => { + const commitIds = isOldBatchInput(params) ? params.commitIds : params.versionIds - if (!userId) { - throw new CommitInvalidAccessError( - 'User must be authenticate to operate with commits' - ) - } - if (!commitIds?.length) { - throw new CommitBatchUpdateError('No commits specified') - } - - const commits = await getCommitsFactory({ db })(commitIds) - const foundCommitIds = commits.map((c) => c.id) - if ( - commitIds.length !== foundCommitIds.length || - difference(commitIds, foundCommitIds).length > 0 - ) { - throw new CommitBatchUpdateError('At least one of the commits does not exist') - } - - const streamGroups = groupBy(commits, (c) => c.streamId) - const streamIds = Object.keys(streamGroups) - const streams = await getStreams(streamIds, { userId }) - - if ( - streamIds.length !== streams.length || - difference( - streamIds, - streams.map((s) => s.id) - ).length > 0 - ) { - throw new CommitBatchUpdateError("At least one commit stream wasn't found") - } - - const streamsById = keyBy(streams, (s) => s.id) - const commitsWithStreams = commits.map((c) => ({ - commit: c, - stream: streamsById[c.streamId] - })) - - for (const { commit, stream } of commitsWithStreams) { - if (stream.role !== Roles.Stream.Owner && commit.author !== userId) { + if (!userId) { throw new CommitInvalidAccessError( - 'To operate on these commits you must either own them or their streams' + 'User must be authenticate to operate with commits' ) } + if (!commitIds?.length) { + throw new CommitBatchUpdateError('No commits specified') + } + + const commits = await deps.getCommits(commitIds) + const foundCommitIds = commits.map((c) => c.id) + if ( + commitIds.length !== foundCommitIds.length || + difference(commitIds, foundCommitIds).length > 0 + ) { + throw new CommitBatchUpdateError('At least one of the commits does not exist') + } + + const streamGroups = groupBy(commits, (c) => c.streamId) + const streamIds = Object.keys(streamGroups) + const streams = await deps.getStreams(streamIds, { userId }) + + if ( + streamIds.length !== streams.length || + difference( + streamIds, + streams.map((s) => s.id) + ).length > 0 + ) { + throw new CommitBatchUpdateError("At least one commit stream wasn't found") + } + + const streamsById = keyBy(streams, (s) => s.id) + const commitsWithStreams = commits.map((c) => ({ + commit: c, + stream: streamsById[c.streamId] + })) + + for (const { commit, stream } of commitsWithStreams) { + if (stream.role !== Roles.Stream.Owner && commit.author !== userId) { + throw new CommitInvalidAccessError( + 'To operate on these commits you must either own them or their streams' + ) + } + } + + return { commitsWithStreams, commits, streams } } - return { commitsWithStreams, commits, streams } +type ValidateCommitsMoveDeps = ValidateBatchBaseRulesDeps & { + getStreamBranchByName: GetStreamBranchByName } /** * Validate batch move params */ -async function validateCommitsMove( - params: CommitsMoveInput | MoveVersionsInput, - userId: string -) { - const targetBranch = isOldBatchInput(params) - ? params.targetBranch - : params.targetModelName - const { streams, commitsWithStreams } = await validateBatchBaseRules(params, userId) - - if (streams.length > 1) { - throw new CommitBatchUpdateError('Commits belong to different streams') - } - - const stream = streams[0] - const branch = await getStreamBranchByNameFactory({ db })(stream.id, targetBranch) - - if ( - !branch && - !([Roles.Stream.Contributor, Roles.Stream.Owner]).includes( - stream.role || '' +const validateCommitsMoveFactory = + (deps: ValidateCommitsMoveDeps) => + async (params: CommitsMoveInput | MoveVersionsInput, userId: string) => { + const targetBranch = isOldBatchInput(params) + ? params.targetBranch + : params.targetModelName + const { streams, commitsWithStreams } = await validateBatchBaseRulesFactory(deps)( + params, + userId ) - ) { - throw new CommitBatchUpdateError( - 'Non-existant target branch referenced and active user does not have the rights to create a new one' - ) - } - return { stream, branch, commitsWithStreams } -} + if (streams.length > 1) { + throw new CommitBatchUpdateError('Commits belong to different streams') + } + + const stream = streams[0] + const branch = await deps.getStreamBranchByName(stream.id, targetBranch) + + if ( + !branch && + !([Roles.Stream.Contributor, Roles.Stream.Owner]).includes( + stream.role || '' + ) + ) { + throw new CommitBatchUpdateError( + 'Non-existant target branch referenced and active user does not have the rights to create a new one' + ) + } + + return { stream, branch, commitsWithStreams } + } /** * Validate batch delete params @@ -128,54 +145,62 @@ async function validateCommitsDelete( params: CommitsDeleteInput | DeleteVersionsInput, userId: string ) { + const validateBatchBaseRules = validateBatchBaseRulesFactory({ + getCommits: getCommitsFactory({ db }), + getStreams + }) return await validateBatchBaseRules(params, userId) } /** * Move a batch of commits belonging to the same stream to another branch */ -export async function batchMoveCommits( - params: CommitsMoveInput | MoveVersionsInput, - userId: string -) { - const { commitIds, targetBranch } = isOldBatchInput(params) - ? params - : { commitIds: params.versionIds, targetBranch: params.targetModelName } +export const batchMoveCommitsFactory = + ( + deps: ValidateCommitsMoveDeps & { + createBranch: StoreBranch + moveCommitsToBranch: MoveCommitsToBranch + addCommitMovedActivity: typeof addCommitMovedActivity + } + ): ValidateAndBatchMoveCommits => + async (params: CommitsMoveInput | MoveVersionsInput, userId: string) => { + const { commitIds, targetBranch } = isOldBatchInput(params) + ? params + : { commitIds: params.versionIds, targetBranch: params.targetModelName } - const { branch, stream, commitsWithStreams } = await validateCommitsMove( - params, - userId - ) + const { branch, stream, commitsWithStreams } = await validateCommitsMoveFactory( + deps + )(params, userId) - try { - const finalBranch = - branch || - (await createBranchFactory({ db })({ - name: targetBranch, - streamId: stream.id, - authorId: userId, - description: null - })) - - await moveCommitsToBranch(commitIds, finalBranch.id) - await Promise.all( - commitsWithStreams.map(({ commit, stream }) => - addCommitMovedActivity({ - commitId: commit.id, + try { + const finalBranch = + branch || + (await deps.createBranch({ + name: targetBranch, streamId: stream.id, - userId, - commit, - originalBranchId: commit.branchId, - newBranchId: finalBranch.id - }) + authorId: userId, + description: null + })) + + await deps.moveCommitsToBranch(commitIds, finalBranch.id) + await Promise.all( + commitsWithStreams.map(({ commit, stream }) => + deps.addCommitMovedActivity({ + commitId: commit.id, + streamId: stream.id, + userId, + commit, + originalBranchId: commit.branchId, + newBranchId: finalBranch.id + }) + ) ) - ) - return finalBranch - } catch (e) { - const err = ensureError(e) - throw new CommitBatchUpdateError('Batch commit move failed', { cause: err }) + return finalBranch + } catch (e) { + const err = ensureError(e) + throw new CommitBatchUpdateError('Batch commit move failed', { cause: err }) + } } -} /** * Delete a batch of commits