From 972af78fce446cdcc982f5d3c54cf77771fe264e Mon Sep 17 00:00:00 2001 From: Kristaps Fabians Geikins Date: Fri, 27 Sep 2024 14:02:29 +0300 Subject: [PATCH] chore(server): core IoC 13 - getCommit(s)Factory --- .../automate/graph/resolvers/automate.ts | 4 +- packages/server/modules/automate/index.ts | 7 +- .../modules/automate/services/tracking.ts | 4 +- .../modules/automate/services/trigger.ts | 8 +- .../modules/automate/tests/trigger.spec.ts | 14 +++- .../modules/core/domain/commits/operations.ts | 14 ++++ .../modules/core/domain/commits/types.ts | 6 ++ packages/server/modules/core/loaders.ts | 3 +- .../modules/core/repositories/commits.ts | 81 +++++++++---------- .../services/commit/batchCommitActions.ts | 4 +- .../core/services/commit/management.ts | 10 ++- .../modules/core/tests/batchCommits.spec.ts | 3 +- 12 files changed, 95 insertions(+), 63 deletions(-) create mode 100644 packages/server/modules/core/domain/commits/operations.ts diff --git a/packages/server/modules/automate/graph/resolvers/automate.ts b/packages/server/modules/automate/graph/resolvers/automate.ts index f0ba3cd5b..ad986d0bd 100644 --- a/packages/server/modules/automate/graph/resolvers/automate.ts +++ b/packages/server/modules/automate/graph/resolvers/automate.ts @@ -109,6 +109,7 @@ import { db } from '@/db/knex' import { AutomationsEmitter } from '@/modules/automate/events/automations' import { AutomateRunsEmitter } from '@/modules/automate/events/runs' import { createAppToken } from '@/modules/core/services/tokens' +import { getCommitFactory } from '@/modules/core/repositories/commits' const { FF_AUTOMATE_MODULE_ENABLED } = getFeatureFlags() @@ -557,7 +558,8 @@ export = (FF_AUTOMATE_MODULE_ENABLED getAutomationToken, upsertAutomationRun, getFullAutomationRevisionMetadata, - getBranchLatestCommits + getBranchLatestCommits, + getCommit: getCommitFactory({ db }) }), validateStreamAccess }) diff --git a/packages/server/modules/automate/index.ts b/packages/server/modules/automate/index.ts index 222c48926..001ed2158 100644 --- a/packages/server/modules/automate/index.ts +++ b/packages/server/modules/automate/index.ts @@ -32,7 +32,6 @@ import { setupRunFinishedTrackingFactory } from '@/modules/automate/services/tra import authGithubAppRest from '@/modules/automate/rest/authGithubApp' import { getFeatureFlags } from '@/modules/shared/helpers/envHelper' import { getUserById } from '@/modules/core/services/users' -import { getCommit } from '@/modules/core/repositories/commits' import { TokenScopeData } from '@/modules/shared/domain/rolesAndScopes/types' import db from '@/db/knex' import { AutomationsEmitter } from '@/modules/automate/events/automations' @@ -40,6 +39,7 @@ import { publish } from '@/modules/shared/utils/subscriptions' import { AutomateRunsEmitter } from '@/modules/automate/events/runs' import { createAppToken } from '@/modules/core/services/tokens' import { getBranchLatestCommitsFactory } from '@/modules/core/repositories/branches' +import { getCommitFactory } from '@/modules/core/repositories/commits' const { FF_AUTOMATE_MODULE_ENABLED } = getFeatureFlags() let quitListeners: Optional<() => void> = undefined @@ -86,7 +86,8 @@ const initializeEventListeners = () => { getAutomationToken: getAutomationTokenFactory({ db }), upsertAutomationRun: upsertAutomationRunFactory({ db }), getFullAutomationRevisionMetadata, - getBranchLatestCommits: getBranchLatestCommitsFactory({ db }) + getBranchLatestCommits: getBranchLatestCommitsFactory({ db }), + getCommit: getCommitFactory({ db }) }) const setupStatusUpdateSubscriptionsInvoke = setupStatusUpdateSubscriptionsFactory({ getAutomationRunFullTriggers, @@ -101,7 +102,7 @@ const initializeEventListeners = () => { const setupRunFinishedTrackingInvoke = setupRunFinishedTrackingFactory({ getFullAutomationRevisionMetadata, getUserById, - getCommit, + getCommit: getCommitFactory({ db }), getFullAutomationRunById: getFullAutomationRunByIdFactory({ db }), automateRunsEventListener: AutomateRunsEmitter.listen }) diff --git a/packages/server/modules/automate/services/tracking.ts b/packages/server/modules/automate/services/tracking.ts index 0b6376862..385b5dd5e 100644 --- a/packages/server/modules/automate/services/tracking.ts +++ b/packages/server/modules/automate/services/tracking.ts @@ -16,7 +16,7 @@ import { RunTriggerSource } from '@/modules/automate/helpers/types' import { InsertableAutomationRun } from '@/modules/automate/repositories/automations' -import { getCommit } from '@/modules/core/repositories/commits' +import { GetCommit } from '@/modules/core/domain/commits/operations' import { getUserById } from '@/modules/core/services/users' import { mixpanel } from '@/modules/shared/utils/mixpanel' import { throwUncoveredError } from '@speckle/shared' @@ -36,7 +36,7 @@ const isFinished = (runStatus: AutomationRunStatus) => { export type AutomateTrackingDeps = { getFullAutomationRevisionMetadata: GetFullAutomationRevisionMetadata getFullAutomationRunById: GetFullAutomationRunById - getCommit: typeof getCommit + getCommit: GetCommit getUserById: typeof getUserById } diff --git a/packages/server/modules/automate/services/trigger.ts b/packages/server/modules/automate/services/trigger.ts index aca6e6735..bc18b14de 100644 --- a/packages/server/modules/automate/services/trigger.ts +++ b/packages/server/modules/automate/services/trigger.ts @@ -10,7 +10,6 @@ import { LiveAutomation, RunTriggerSource } from '@/modules/automate/helpers/types' -import { getCommit } from '@/modules/core/repositories/commits' import { createAppToken } from '@/modules/core/services/tokens' import { Roles, Scopes } from '@speckle/shared' import cryptoRandomString from 'crypto-random-string' @@ -48,6 +47,7 @@ import { UpsertAutomationRun } from '@/modules/automate/domain/operations' import { GetBranchLatestCommits } from '@/modules/core/domain/branches/operations' +import { GetCommit } from '@/modules/core/domain/commits/operations' export type OnModelVersionCreateDeps = { getAutomation: GetAutomation @@ -216,6 +216,7 @@ export type TriggerAutomationRevisionRunDeps = { upsertAutomationRun: UpsertAutomationRun automateRunsEmitter: AutomateRunsEventsEmitter getFullAutomationRevisionMetadata: GetFullAutomationRevisionMetadata + getCommit: GetCommit } & CreateAutomationRunDataDeps & ComposeTriggerDataDeps @@ -235,7 +236,8 @@ export const triggerAutomationRevisionRunFactory = createAppToken, upsertAutomationRun, automateRunsEmitter, - getFullAutomationRevisionMetadata + getFullAutomationRevisionMetadata, + getCommit } = deps const { revisionId, manifest, source = RunTriggerSource.Automatic } = params @@ -328,7 +330,7 @@ export const triggerAutomationRevisionRunFactory = export const ensureRunConditionsFactory = (deps: { revisionGetter: GetFullAutomationRevisionMetadata - versionGetter: typeof getCommit + versionGetter: GetCommit automationTokenGetter: GetAutomationToken }) => async (params: { diff --git a/packages/server/modules/automate/tests/trigger.spec.ts b/packages/server/modules/automate/tests/trigger.spec.ts index 5973951b8..e64bbae4c 100644 --- a/packages/server/modules/automate/tests/trigger.spec.ts +++ b/packages/server/modules/automate/tests/trigger.spec.ts @@ -77,6 +77,7 @@ import { db } from '@/db/knex' import { AutomateRunsEmitter } from '@/modules/automate/events/runs' import { createAppToken } from '@/modules/core/services/tokens' import { validateStreamAccess } from '@/modules/core/services/streams/streamAccessService' +import { getCommitFactory } from '@/modules/core/repositories/commits' const { FF_AUTOMATE_MODULE_ENABLED } = getFeatureFlags() @@ -97,6 +98,7 @@ const getFullAutomationRevisionMetadata = getFullAutomationRevisionMetadataFacto const updateAutomationRevision = updateAutomationRevisionFactory({ db }) const updateAutomationRun = updateAutomationRunFactory({ db }) const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) +const getCommit = getCommitFactory({ db }) ;(FF_AUTOMATE_MODULE_ENABLED ? describe : describe.skip)( 'Automate triggers @automate', @@ -322,7 +324,8 @@ const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) getAutomationToken, upsertAutomationRun, getFullAutomationRevisionMetadata, - getBranchLatestCommits + getBranchLatestCommits, + getCommit })({ revisionId: cryptoRandomString({ length: 10 }), manifest: { @@ -415,7 +418,8 @@ const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) getAutomationToken, upsertAutomationRun, getFullAutomationRevisionMetadata, - getBranchLatestCommits + getBranchLatestCommits, + getCommit })({ revisionId: automationRevisionId, manifest: { @@ -514,7 +518,8 @@ const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) getAutomationToken, upsertAutomationRun, getFullAutomationRevisionMetadata, - getBranchLatestCommits + getBranchLatestCommits, + getCommit })({ revisionId: automationRevisionId, manifest: { @@ -997,7 +1002,8 @@ const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) getAutomationToken, upsertAutomationRun, getFullAutomationRevisionMetadata, - getBranchLatestCommits + getBranchLatestCommits, + getCommit }), validateStreamAccess, ...(overrides || {}) diff --git a/packages/server/modules/core/domain/commits/operations.ts b/packages/server/modules/core/domain/commits/operations.ts new file mode 100644 index 000000000..d8e04de9c --- /dev/null +++ b/packages/server/modules/core/domain/commits/operations.ts @@ -0,0 +1,14 @@ +import { CommitWithStreamBranchMetadata } from '@/modules/core/domain/commits/types' +import { Optional } from '@speckle/shared' + +export type GetCommits = ( + commitIds: string[], + options?: Partial<{ + streamId: string + }> +) => Promise + +export type GetCommit = ( + commitId: string, + options?: Partial<{ streamId: string }> +) => Promise> diff --git a/packages/server/modules/core/domain/commits/types.ts b/packages/server/modules/core/domain/commits/types.ts index f18485c0a..931d60578 100644 --- a/packages/server/modules/core/domain/commits/types.ts +++ b/packages/server/modules/core/domain/commits/types.ts @@ -4,3 +4,9 @@ export type Commit = CommitRecord export type BranchLatestCommit = Commit & { branchId: string } + +export type CommitWithStreamBranchMetadata = Commit & { + streamId: string + branchId: string + branchName: string +} diff --git a/packages/server/modules/core/loaders.ts b/packages/server/modules/core/loaders.ts index d56c0aec1..ee97e3aed 100644 --- a/packages/server/modules/core/loaders.ts +++ b/packages/server/modules/core/loaders.ts @@ -24,7 +24,7 @@ import { Nullable } from '@/modules/shared/helpers/typeHelper' import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' import { getCommitBranches, - getCommits, + getCommitsFactory, getSpecificBranchCommits, getStreamCommitCounts, getUserAuthoredCommitCounts, @@ -110,6 +110,7 @@ const getStreamBranchesByName = getStreamBranchesByNameFactory({ db }) const getBranchLatestCommits = getBranchLatestCommitsFactory({ db }) const getStreamBranchCounts = getStreamBranchCountsFactory({ db }) const getBranchCommitCounts = getBranchCommitCountsFactory({ db }) +const getCommits = getCommitsFactory({ 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 64b6fca03..3d38ad9cb 100644 --- a/packages/server/modules/core/repositories/commits.ts +++ b/packages/server/modules/core/repositories/commits.ts @@ -14,13 +14,6 @@ import { StreamCommitRecord } from '@/modules/core/helpers/types' import { clamp, uniq, uniqBy, reduce, keyBy, mapValues } from 'lodash' - -const CommitWithStreamBranchMetadataFields = [ - ...Commits.cols, - StreamCommits.col.streamId, - BranchCommits.col.branchId, - `${Branches.col.name} as branchName` -] import crs from 'crypto-random-string' import { BatchedSelectOptions, @@ -28,51 +21,55 @@ import { } from '@/modules/shared/helpers/dbHelper' import { Knex } from 'knex' import { Nullable, Optional } from '@speckle/shared' +import { CommitWithStreamBranchMetadata } from '@/modules/core/domain/commits/types' +import { GetCommit, GetCommits } from '@/modules/core/domain/commits/operations' + +const tables = { + commits: (db: Knex) => db(Commits.name) +} export const generateCommitId = () => crs({ length: 10 }) -export type CommitWithStreamBranchMetadata = CommitRecord & { - streamId: string - branchId: string - branchName: string -} - /** * Get commits with their stream and branch IDs */ -export async function getCommits( - commitIds: string[], - options?: Partial<{ streamId: string }> -) { - const { streamId } = options || {} +export const getCommitsFactory = + (deps: { db: Knex }): GetCommits => + async (commitIds: string[], options?: Partial<{ streamId: string }>) => { + const { streamId } = options || {} - const q = Commits.knex() - .select(CommitWithStreamBranchMetadataFields) - .whereIn(Commits.col.id, commitIds) - .leftJoin(StreamCommits.name, StreamCommits.col.commitId, Commits.col.id) - .leftJoin(BranchCommits.name, BranchCommits.col.commitId, Commits.col.id) - .innerJoin(Branches.name, Branches.col.id, BranchCommits.col.branchId) + const q = tables + .commits(deps.db) + .select([ + ...Commits.cols, + StreamCommits.col.streamId, + BranchCommits.col.branchId, + `${Branches.col.name} as branchName` + ]) + .whereIn(Commits.col.id, commitIds) + .leftJoin(StreamCommits.name, StreamCommits.col.commitId, Commits.col.id) + .leftJoin(BranchCommits.name, BranchCommits.col.commitId, Commits.col.id) + .innerJoin(Branches.name, Branches.col.id, BranchCommits.col.branchId) - if (streamId) { - q.andWhere(StreamCommits.col.streamId, streamId) + if (streamId) { + q.andWhere(StreamCommits.col.streamId, streamId) + } + + const rows = await q + + // in case the join tables have multiple values for each commit + // (shouldnt happen, but the schema allows for it) + const uniqueRows = uniqBy(rows, (r) => r.id) + + return uniqueRows } - const rows = await q - - // in case the join tables have multiple values for each commit - // (shouldnt happen, but the schema allows for it) - const uniqueRows = uniqBy(rows, (r) => r.id) - - return uniqueRows -} - -export async function getCommit( - commitId: string, - options?: Partial<{ streamId: string }> -) { - const [commit] = await getCommits([commitId], options) - return commit as Optional -} +export const getCommitFactory = + (deps: { db: Knex }): GetCommit => + async (commitId: string, options?: Partial<{ streamId: string }>) => { + const [commit] = await getCommitsFactory(deps)([commitId], options) + return commit as Optional + } /** * Move all commits to the specified branch diff --git a/packages/server/modules/core/services/commit/batchCommitActions.ts b/packages/server/modules/core/services/commit/batchCommitActions.ts index 7616e0aa4..4536315d2 100644 --- a/packages/server/modules/core/services/commit/batchCommitActions.ts +++ b/packages/server/modules/core/services/commit/batchCommitActions.ts @@ -20,7 +20,7 @@ import { } from '@/modules/core/repositories/branches' import { deleteCommits, - getCommits, + getCommitsFactory, moveCommitsToBranch } from '@/modules/core/repositories/commits' import { getStreams } from '@/modules/core/repositories/streams' @@ -48,7 +48,7 @@ async function validateBatchBaseRules(params: CommitBatchInput, userId: string) throw new CommitBatchUpdateError('No commits specified') } - const commits = await getCommits(commitIds) + const commits = await getCommitsFactory({ db })(commitIds) const foundCommitIds = commits.map((c) => c.id) if ( commitIds.length !== foundCommitIds.length || diff --git a/packages/server/modules/core/services/commit/management.ts b/packages/server/modules/core/services/commit/management.ts index d4aee3271..923148937 100644 --- a/packages/server/modules/core/services/commit/management.ts +++ b/packages/server/modules/core/services/commit/management.ts @@ -27,8 +27,8 @@ import { import { createCommit, deleteCommit, - getCommit, getCommitBranch, + getCommitFactory, insertBranchCommits, insertStreamCommits, switchCommitBranch, @@ -58,7 +58,9 @@ export async function markCommitReceivedAndNotify(params: { } : input - const commit = await getCommit(oldInput.commitId, { streamId: oldInput.streamId }) + const commit = await getCommitFactory({ db })(oldInput.commitId, { + streamId: oldInput.streamId + }) if (!commit) { throw new CommitReceiveError( `Failed to find commit with id ${oldInput.commitId} in stream ${oldInput.streamId}.`, @@ -247,7 +249,7 @@ export async function updateCommitAndNotify( } const [commit, stream] = await Promise.all([ - getCommit(commitId), + getCommitFactory({ db })(commitId), streamId ? getStream({ streamId, userId }) : getCommitStream({ commitId, userId }) ]) if (!commit) { @@ -321,7 +323,7 @@ export async function deleteCommitAndNotify( streamId: string, userId: string ) { - const commit = await getCommit(commitId) + const commit = await getCommitFactory({ db })(commitId) if (!commit) { throw new CommitDeleteError("Couldn't delete nonexistant commit", { info: { commitId, streamId, userId } diff --git a/packages/server/modules/core/tests/batchCommits.spec.ts b/packages/server/modules/core/tests/batchCommits.spec.ts index 8bb2afcb2..98098b268 100644 --- a/packages/server/modules/core/tests/batchCommits.spec.ts +++ b/packages/server/modules/core/tests/batchCommits.spec.ts @@ -3,7 +3,7 @@ import { db } from '@/db/knex' import { Commits, Streams, Users } from '@/modules/core/dbSchema' import { Roles } from '@/modules/core/helpers/mainConstants' import { createBranchFactory } from '@/modules/core/repositories/branches' -import { getCommits } from '@/modules/core/repositories/commits' +import { getCommitsFactory } from '@/modules/core/repositories/commits' import { addOrUpdateStreamCollaborator } from '@/modules/core/services/streams/streamAccessService' import { BasicTestUser, createTestUsers } from '@/test/authHelper' import { deleteCommits, moveCommits } from '@/test/graphql/commits' @@ -25,6 +25,7 @@ enum BatchActionType { } const createBranch = createBranchFactory({ db }) +const getCommits = getCommitsFactory({ db }) const cleanup = async () => { await truncateTables([Streams.name, Users.name, Commits.name])