Merge pull request #3323 from specklesystems/fabians/core-ioc-85

chore(server): core IoC #85 - legacyGetPaginatedStreamCommits

packages/server/modules/core/domain/commits/operations.ts

@@ -5,13 +5,15 @@ import {
   Commit,
   CommitBranch,
   CommitWithStreamId,
-  LegacyUserCommit
+  LegacyUserCommit,
+  LegacyStreamCommit
 } from '@/modules/core/domain/commits/types'
 import {
   CommitsMoveInput,
   CommitUpdateInput,
   ModelVersionsFilter,
   MoveVersionsInput,
+  StreamCommitsArgs,
   UpdateVersionInput
 } from '@/modules/core/graph/generated/graphql'
 import { BranchCommitRecord, StreamCommitRecord } from '@/modules/core/helpers/types'
@@ -254,3 +256,22 @@ export type LegacyGetPaginatedUserCommitsTotalCount = ({
   publicOnly?: MaybeNullOrUndefined<boolean>
   streamIdWhitelist?: MaybeNullOrUndefined<string[]>
 }) => Promise<number>
+
+export type LegacyGetPaginatedStreamCommitsPage = (params: {
+  streamId: string
+  limit?: MaybeNullOrUndefined<number>
+  cursor?: MaybeNullOrUndefined<string>
+  ignoreGlobalsBranch?: MaybeNullOrUndefined<boolean>
+}) => Promise<{
+  commits: LegacyStreamCommit[]
+  cursor: Nullable<string>
+}>
+
+export type LegacyGetPaginatedStreamCommits = (
+  streamId: string,
+  params: StreamCommitsArgs
+) => Promise<{
+  items: LegacyStreamCommit[]
+  cursor: Nullable<string>
+  totalCount: number
+}>

packages/server/modules/core/graph/resolvers/commitsNew.ts

@@ -2,10 +2,9 @@ import { CommitNotFoundError } from '@/modules/core/errors/commit'
 import { publish } from '@/modules/shared/utils/subscriptions'
 import { authorizeResolver } from '@/modules/shared'
 
-import { getCommitsByStreamId } from '@/modules/core/services/commits'
 import {
-  getPaginatedStreamCommits,
-  getPaginatedBranchCommitsFactory
+  getPaginatedBranchCommitsFactory,
+  legacyGetPaginatedStreamCommits
 } from '@/modules/core/services/commit/retrieval'
 import {
   markCommitReceivedAndNotify,
@@ -43,7 +42,9 @@ import {
   getCommitsFactory,
   moveCommitsToBranchFactory,
   legacyGetPaginatedUserCommitsPage,
-  legacyGetPaginatedUserCommitsTotalCount
+  legacyGetPaginatedUserCommitsTotalCount,
+  legacyGetPaginatedStreamCommitsPageFactory,
+  getStreamCommitCountFactory
 } from '@/modules/core/repositories/commits'
 import { db } from '@/db/knex'
 import {
@@ -144,6 +145,11 @@ const batchMoveCommits = batchMoveCommitsFactory({
 const validateStreamAccess = validateStreamAccessFactory({ authorizeResolver })
 const getCommitsByUserId = legacyGetPaginatedUserCommitsPage({ db })
 const getCommitsTotalCountByUserId = legacyGetPaginatedUserCommitsTotalCount({ db })
+const getCommitsByStreamId = legacyGetPaginatedStreamCommitsPageFactory({ db })
+const getPaginatedStreamCommits = legacyGetPaginatedStreamCommits({
+  legacyGetPaginatedStreamCommitsPage: getCommitsByStreamId,
+  getStreamCommitCount: getStreamCommitCountFactory({ db })
+})
 
 const getAuthorId = (commit: CommitGraphQLReturn) => {
   if ('author' in commit) return commit.author
@@ -247,9 +253,7 @@ export = {
       if (!args.id) {
         const { commits } = await getCommitsByStreamId({
           streamId: parent.id,
-          limit: 1,
-          cursor: undefined,
-          ignoreGlobalsBranch: undefined
+          limit: 1
         })
         if (commits.length !== 0) return commits[0]
         throw new CommitNotFoundError(

packages/server/modules/core/graph/resolvers/models.ts

@@ -16,7 +16,7 @@ import { last } from 'lodash'
 import { getViewerResourceGroupsFactory } from '@/modules/core/services/commit/viewerResources'
 import {
   getPaginatedBranchCommitsFactory,
-  getPaginatedStreamCommits
+  legacyGetPaginatedStreamCommits
 } from '@/modules/core/services/commit/retrieval'
 import {
   filteredSubscribe,
@@ -44,7 +44,9 @@ import {
   getAllBranchCommitsFactory,
   getBranchCommitsTotalCountFactory,
   getPaginatedBranchCommitsItemsFactory,
-  getSpecificBranchCommitsFactory
+  getSpecificBranchCommitsFactory,
+  getStreamCommitCountFactory,
+  legacyGetPaginatedStreamCommitsPageFactory
 } from '@/modules/core/repositories/commits'
 import { db } from '@/db/knex'
 import {
@@ -108,6 +110,12 @@ const getPaginatedBranchCommits = getPaginatedBranchCommitsFactory({
   getPaginatedBranchCommitsItems: getPaginatedBranchCommitsItemsFactory({ db }),
   getBranchCommitsTotalCount: getBranchCommitsTotalCountFactory({ db })
 })
+const getPaginatedStreamCommits = legacyGetPaginatedStreamCommits({
+  legacyGetPaginatedStreamCommitsPage: legacyGetPaginatedStreamCommitsPageFactory({
+    db
+  }),
+  getStreamCommitCount: getStreamCommitCountFactory({ db })
+})
 
 export = {
   User: {

packages/server/modules/core/repositories/commits.ts

@@ -24,6 +24,7 @@ import { Knex } from 'knex'
 import { MaybeNullOrUndefined, Optional } from '@speckle/shared'
 import {
   CommitWithStreamBranchMetadata,
+  LegacyStreamCommit,
   LegacyUserCommit
 } from '@/modules/core/domain/commits/types'
 import {
@@ -55,7 +56,8 @@ import {
   GetBranchCommitsTotalCount,
   MoveCommitsToBranch,
   LegacyGetPaginatedUserCommitsPage,
-  LegacyGetPaginatedUserCommitsTotalCount
+  LegacyGetPaginatedUserCommitsTotalCount,
+  LegacyGetPaginatedStreamCommitsPage
 } from '@/modules/core/domain/commits/operations'
 
 const tables = {
@@ -538,6 +540,10 @@ export const getUserAuthoredCommitCountsFactory =
     return mapValues(keyBy(res, 'author'), (r) => parseInt(r.count))
   }
 
+/**
+ * @deprecated Deprecated because of the weird/messy commit structure. It should return CommitRecords
+ * without any joins, and let those be handled by GQL dataloaders
+ */
 const getCommitsByUserIdBaseFactory =
   (deps: { db: Knex }) =>
   ({
@@ -582,6 +588,10 @@ const getCommitsByUserIdBaseFactory =
     return query
   }
 
+/**
+ * @deprecated Deprecated because of the weird/messy commit structure. It should return CommitRecords
+ * without any joins, and let those be handled by GQL dataloaders
+ */
 export const legacyGetPaginatedUserCommitsPage =
   (deps: { db: Knex }): LegacyGetPaginatedUserCommitsPage =>
   async ({ userId, limit, cursor, publicOnly, streamIdWhitelist }) => {
@@ -605,6 +615,10 @@ export const legacyGetPaginatedUserCommitsPage =
     }
   }
 
+/**
+ * @deprecated Deprecated because of the weird/messy commit structure. It should return CommitRecords
+ * without any joins, and let those be handled by GQL dataloaders
+ */
 export const legacyGetPaginatedUserCommitsTotalCount =
   (deps: { db: Knex }): LegacyGetPaginatedUserCommitsTotalCount =>
   async ({ userId, publicOnly, streamIdWhitelist }) => {
@@ -619,3 +633,49 @@ export const legacyGetPaginatedUserCommitsTotalCount =
     const [res] = (await query) as Array<{ count: string }>
     return parseInt(res.count)
   }
+
+/**
+ * @deprecated Deprecated because of the weird/messy commit structure. It should return CommitRecords
+ * without any joins, and let those be handled by GQL dataloaders
+ */
+export const legacyGetPaginatedStreamCommitsPageFactory =
+  (deps: { db: Knex }): LegacyGetPaginatedStreamCommitsPage =>
+  async ({ streamId, limit, cursor, ignoreGlobalsBranch }) => {
+    limit = clamp(limit || 25, 0, 100)
+    if (!limit) return { commits: [], cursor: null }
+
+    const query = tables
+      .streamCommits(deps.db)
+      .columns([
+        { id: 'commits.id' },
+        'message',
+        'referencedObject',
+        'sourceApplication',
+        'totalChildrenCount',
+        'parents',
+        'commits.createdAt',
+        { branchName: 'branches.name' },
+        { authorName: 'users.name' },
+        { authorId: 'users.id' },
+        { authorAvatar: 'users.avatar' },
+        knex.raw(`?? as "author"`, ['users.id'])
+      ])
+      .select()
+      .join('commits', 'commits.id', 'stream_commits.commitId')
+      .join('branch_commits', 'commits.id', 'branch_commits.commitId')
+      .join('branches', 'branches.id', 'branch_commits.branchId')
+      .leftJoin('users', 'commits.author', 'users.id')
+      .where('stream_commits.streamId', streamId)
+
+    if (ignoreGlobalsBranch) query.andWhere('branches.name', '!=', 'globals')
+
+    if (cursor) query.andWhere('commits.createdAt', '<', cursor)
+
+    query.orderBy('commits.createdAt', 'desc').limit(limit)
+
+    const rows = (await query) as LegacyStreamCommit[]
+    return {
+      commits: rows,
+      cursor: rows.length > 0 ? rows[rows.length - 1].createdAt.toISOString() : null
+    }
+  }

packages/server/modules/core/services/commit/retrieval.ts

@@ -3,38 +3,41 @@ import {
   ModelVersionsFilter,
   StreamCommitsArgs
 } from '@/modules/core/graph/generated/graphql'
-import { getStreamCommitCountFactory } from '@/modules/core/repositories/commits'
-import { getCommitsByStreamId } from '@/modules/core/services/commits'
 import { BadRequestError } from '@/modules/shared/errors'
-import { db } from '@/db/knex'
 import {
   GetBranchCommitsTotalCount,
   GetPaginatedBranchCommits,
   GetPaginatedBranchCommitsItems,
   GetSpecificBranchCommits,
+  GetStreamCommitCount,
+  LegacyGetPaginatedStreamCommits,
+  LegacyGetPaginatedStreamCommitsPage,
   PaginatedBranchCommitsParams
 } from '@/modules/core/domain/commits/operations'
 
-export async function getPaginatedStreamCommits(
-  streamId: string,
-  params: StreamCommitsArgs
-) {
-  if (params.limit && params.limit > 100)
-    throw new BadRequestError(
-      'Cannot return more than 100 items, please use pagination.'
-    )
-  const { commits: items, cursor } = await getCommitsByStreamId({
-    streamId,
-    limit: params.limit,
-    cursor: params.cursor,
-    ignoreGlobalsBranch: true
-  })
-  const totalCount = await getStreamCommitCountFactory({ db })(streamId, {
-    ignoreGlobalsBranch: true
-  })
+export const legacyGetPaginatedStreamCommits =
+  (deps: {
+    legacyGetPaginatedStreamCommitsPage: LegacyGetPaginatedStreamCommitsPage
+    getStreamCommitCount: GetStreamCommitCount
+  }): LegacyGetPaginatedStreamCommits =>
+  async (streamId: string, params: StreamCommitsArgs) => {
+    if (params.limit && params.limit > 100)
+      throw new BadRequestError(
+        'Cannot return more than 100 items, please use pagination.'
+      )
 
-  return { items, cursor, totalCount }
-}
+    const { commits: items, cursor } = await deps.legacyGetPaginatedStreamCommitsPage({
+      streamId,
+      limit: params.limit,
+      cursor: params.cursor,
+      ignoreGlobalsBranch: true
+    })
+    const totalCount = await deps.getStreamCommitCount(streamId, {
+      ignoreGlobalsBranch: true
+    })
+
+    return { items, cursor, totalCount }
+  }
 
 export const getPaginatedBranchCommitsFactory =
   (deps: {

packages/server/modules/core/services/commits.js

@@ -6,9 +6,6 @@ const {
   getPaginatedBranchCommitsItemsFactory
 } = require('@/modules/core/repositories/commits')
 
-const StreamCommits = () => knex('stream_commits')
-const { clamp } = require('lodash')
-
 module.exports = {
   async getCommitsTotalCountByBranchName({ streamId, branchName }) {
     branchName = branchName.toLowerCase()
@@ -33,50 +30,5 @@ module.exports = {
       db: knex
     })
     return getPaginatedBranchCommits({ branchId: myBranch.id, limit, cursor })
-  },
-
-  /**
-   * @returns {Promise<{
-   *  commits: import('@/modules/core/helpers/types').CommitRecord[],
-   *  cursor: string | null
-   * }>}
-   */
-  async getCommitsByStreamId({ streamId, limit, cursor, ignoreGlobalsBranch }) {
-    limit = clamp(limit || 25, 0, 100)
-    if (!limit) return { commits: [], cursor: null }
-
-    const query = StreamCommits()
-      .columns([
-        { id: 'commits.id' },
-        'message',
-        'referencedObject',
-        'sourceApplication',
-        'totalChildrenCount',
-        'parents',
-        'commits.createdAt',
-        { branchName: 'branches.name' },
-        { authorName: 'users.name' },
-        { authorId: 'users.id' },
-        { authorAvatar: 'users.avatar' },
-        knex.raw(`?? as "author"`, ['users.id'])
-      ])
-      .select()
-      .join('commits', 'commits.id', 'stream_commits.commitId')
-      .join('branch_commits', 'commits.id', 'branch_commits.commitId')
-      .join('branches', 'branches.id', 'branch_commits.branchId')
-      .leftJoin('users', 'commits.author', 'users.id')
-      .where('stream_commits.streamId', streamId)
-
-    if (ignoreGlobalsBranch) query.andWhere('branches.name', '!=', 'globals')
-
-    if (cursor) query.andWhere('commits.createdAt', '<', cursor)
-
-    query.orderBy('commits.createdAt', 'desc').limit(limit)
-
-    const rows = await query
-    return {
-      commits: rows,
-      cursor: rows.length > 0 ? rows[rows.length - 1].createdAt.toISOString() : null
-    }
   }
 }

packages/server/modules/core/tests/commits.spec.js

@@ -7,8 +7,7 @@ const { createObject } = require('../services/objects')
 
 const {
   getCommitsTotalCountByBranchName,
-  getCommitsByBranchName,
-  getCommitsByStreamId
+  getCommitsByBranchName
 } = require('../services/commits')
 const {
   createBranchAndNotifyFactory
@@ -34,7 +33,8 @@ const {
   switchCommitBranchFactory,
   updateCommitFactory,
   getStreamCommitCountFactory,
-  legacyGetPaginatedUserCommitsPage
+  legacyGetPaginatedUserCommitsPage,
+  legacyGetPaginatedStreamCommitsPageFactory
 } = require('@/modules/core/repositories/commits')
 const {
   deleteCommitAndNotifyFactory,
@@ -237,6 +237,7 @@ const createUser = createUserFactory({
   usersEventsEmitter: UsersEmitter.emit
 })
 const getCommitsByUserId = legacyGetPaginatedUserCommitsPage({ db })
+const getCommitsByStreamId = legacyGetPaginatedStreamCommitsPageFactory({ db })
 
 describe('Commits @core-commits', () => {
   const user = {

packages/server/modules/core/tests/users.spec.js

@@ -9,7 +9,7 @@ const {
 
 const { getBranchesByStreamId } = require('../services/branches')
 
-const { getCommitsByBranchName, getCommitsByStreamId } = require('../services/commits')
+const { getCommitsByBranchName } = require('../services/commits')
 
 const { createObject } = require('../services/objects')
 const { beforeEachContext } = require('@/test/hooks')
@@ -26,7 +26,8 @@ const {
   getCommitFactory,
   createCommitFactory,
   insertStreamCommitsFactory,
-  insertBranchCommitsFactory
+  insertBranchCommitsFactory,
+  legacyGetPaginatedStreamCommitsPageFactory
 } = require('@/modules/core/repositories/commits')
 const {
   createCommitByBranchIdFactory,
@@ -285,6 +286,7 @@ const validateToken = validateTokenFactory({
   }),
   updateApiToken: updateApiTokenFactory({ db })
 })
+const getCommitsByStreamId = legacyGetPaginatedStreamCommitsPageFactory({ db })
 
 describe('Actors & Tokens @user-services', () => {
   const myTestActor = {

packages/server/modules/previews/index.ts

@@ -1,10 +1,7 @@
 /* istanbul ignore file */
 'use strict'
 import { validateScopes, authorizeResolver } from '@/modules/shared'
-import {
-  getCommitsByStreamId,
-  getCommitsByBranchName
-} from '@/modules/core/services/commits'
+import { getCommitsByBranchName } from '@/modules/core/services/commits'
 
 import { makeOgImage } from '@/modules/previews/ogImage'
 import { moduleLogger } from '@/logging/logging'
@@ -26,7 +23,8 @@ import {
 import { publish } from '@/modules/shared/utils/subscriptions'
 import {
   getCommitFactory,
-  getObjectCommitsWithStreamIdsFactory
+  getObjectCommitsWithStreamIdsFactory,
+  legacyGetPaginatedStreamCommitsPageFactory
 } from '@/modules/core/repositories/commits'
 import { SpeckleModule } from '@/modules/shared/helpers/typeHelper'
 import { getStreamFactory } from '@/modules/core/repositories/streams'
@@ -43,6 +41,7 @@ export const init: SpeckleModule['init'] = (app, isInitial) => {
     moduleLogger.info('📸 Init object preview module')
   }
 
+  const getCommitsByStreamId = legacyGetPaginatedStreamCommitsPageFactory({ db })
   const getStream = getStreamFactory({ db })
   const getObjectPreviewBufferOrFilepath = getObjectPreviewBufferOrFilepathFactory({
     getObject,