refactor(backend): fix tests for #8714 (#8716)

This fixes the tests for #8714.

The Images type now has the callbacks only for backwards compatibility with local uploads. Next step is to remove the obsolete code and make S3 configuration parameters required.

backend/src/graphql/resolvers/images/imagesS3.spec.ts

@@ -1,11 +1,10 @@
-/* eslint-disable @typescript-eslint/require-await */
-
 /* eslint-disable @typescript-eslint/no-unsafe-return */
-/* eslint-disable @typescript-eslint/restrict-template-expressions */
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable promise/prefer-await-to-callbacks */
+import { DeleteObjectCommand } from '@aws-sdk/client-s3'
+import { Upload } from '@aws-sdk/lib-storage'
 import { UserInputError } from 'apollo-server'
 
 import Factory, { cleanDatabase } from '@db/factories'
@@ -17,11 +16,30 @@ import { images } from './imagesS3'
 import type { ImageInput } from './images'
 import type { FileUpload } from 'graphql-upload'
 
+jest.mock('@aws-sdk/client-s3', () => {
+  return {
+    S3Client: jest.fn().mockImplementation(() => ({
+      send: jest.fn(),
+    })),
+    ObjectCannedACL: { public_read: 'public_read' },
+    DeleteObjectCommand: jest.fn().mockImplementation(() => ({})),
+  }
+})
+
+jest.mock('@aws-sdk/lib-storage', () => {
+  return {
+    Upload: jest.fn().mockImplementation(({ params: { Key } }: { params: { Key: string } }) => ({
+      done: () => Promise.resolve({ Location: `http://your-objectstorage.com/bucket/${Key}` }),
+    })),
+  }
+})
+
+const mockUpload = jest.mocked(Upload)
+const mockDeleteObjectCommand = jest.mocked(DeleteObjectCommand)
+
 const driver = getDriver()
 const neode = getNeode()
 const uuid = '[0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12}'
-let uploadCallback
-let deleteCallback
 
 const config: S3Configured = {
   AWS_ACCESS_KEY_ID: 'AWS_ACCESS_KEY_ID',
@@ -41,16 +59,10 @@ afterAll(async () => {
   await driver.close()
 })
 
-beforeEach(async () => {
-  uploadCallback = jest.fn(
-    ({ uniqueFilename }) => `http://your-objectstorage.com/bucket/${uniqueFilename}`,
-  )
-  deleteCallback = jest.fn()
-})
-
 // TODO: avoid database clean after each test in the future if possible for performance and flakyness reasons by filling the database step by step, see issue https://github.com/Ocelot-Social-Community/Ocelot-Social/issues/4543
 afterEach(async () => {
   await cleanDatabase()
+  jest.clearAllMocks()
 })
 
 describe('deleteImage', () => {
@@ -73,13 +85,13 @@ describe('deleteImage', () => {
 
     it('deletes `Image` node', async () => {
       await expect(neode.all('Image')).resolves.toHaveLength(1)
-      await deleteImage(user, 'AVATAR_IMAGE', { deleteCallback })
+      await deleteImage(user, 'AVATAR_IMAGE')
       await expect(neode.all('Image')).resolves.toHaveLength(0)
     })
 
     it('calls deleteCallback', async () => {
-      await deleteImage(user, 'AVATAR_IMAGE', { deleteCallback })
+      await deleteImage(user, 'AVATAR_IMAGE')
-      expect(deleteCallback).toHaveBeenCalled()
+      expect(mockDeleteObjectCommand).toHaveBeenCalled()
     })
 
     describe('given a transaction parameter', () => {
@@ -89,7 +101,6 @@ describe('deleteImage', () => {
         try {
           someString = await session.writeTransaction(async (transaction) => {
             await deleteImage(user, 'AVATAR_IMAGE', {
-              deleteCallback,
               transaction,
             })
             const txResult = await transaction.run('RETURN "Hello" as result')
@@ -109,7 +120,6 @@ describe('deleteImage', () => {
         try {
           await session.writeTransaction(async (transaction) => {
             await deleteImage(user, 'AVATAR_IMAGE', {
-              deleteCallback,
               transaction,
             })
             throw new Error('Ouch!')
@@ -169,22 +179,20 @@ describe('mergeImage', () => {
       })
 
       it('returns new image', async () => {
-        await expect(
+        await expect(mergeImage(post, 'HERO_IMAGE', imageInput)).resolves.toMatchObject({
-          mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback }),
-        ).resolves.toMatchObject({
           url: expect.any(String),
           alt: 'A description of the new image',
         })
       })
 
       it('calls upload callback', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
-        expect(uploadCallback).toHaveBeenCalled()
+        expect(mockUpload).toHaveBeenCalled()
       })
 
       it('creates `:Image` node', async () => {
         await expect(neode.all('Image')).resolves.toHaveLength(0)
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
         await expect(neode.all('Image')).resolves.toHaveLength(1)
       })
 
@@ -195,11 +203,9 @@ describe('mergeImage', () => {
         const upload = await imageInput.upload
         upload.filename = '/path/to/awkward?/ file-location/?foo- bar-avatar.jpg'
         imageInput.upload = Promise.resolve(upload)
-        await expect(
+        await expect(mergeImage(post, 'HERO_IMAGE', imageInput)).resolves.toMatchObject({
-          mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback }),
-        ).resolves.toMatchObject({
           url: expect.stringMatching(
-            new RegExp(`^http://your-objectstorage.com/bucket/${uuid}-foo-bar-avatar.jpg`),
+            new RegExp(`^http://your-objectstorage.com/bucket/original/${uuid}-foo-bar-avatar.jpg`),
           ),
         })
       })
@@ -210,26 +216,25 @@ describe('mergeImage', () => {
           S3_PUBLIC_GATEWAY: 'http://s3-public-gateway.com',
         })
 
-        // eslint-disable-next-line jest/no-disabled-tests
+        it('changes the domain of the URL to a server that could e.g. apply image transformations', async () => {
-        it.skip('changes the domain of the URL to a server that could e.g. apply image transformations', async () => {
           if (!imageInput.upload) {
             throw new Error('Test imageInput was not setup correctly.')
           }
           const upload = await imageInput.upload
           upload.filename = '/path/to/file-location/foo-bar-avatar.jpg'
           imageInput.upload = Promise.resolve(upload)
-          await expect(
+          await expect(mergeImage(post, 'HERO_IMAGE', imageInput)).resolves.toMatchObject({
-            mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback }),
-          ).resolves.toMatchObject({
             url: expect.stringMatching(
-              new RegExp(`^http://s3-public-gateway.com/bucket/${uuid}-foo-bar-avatar.jpg`),
+              new RegExp(
+                `^http://s3-public-gateway.com/bucket/original/${uuid}-foo-bar-avatar.jpg`,
+              ),
             ),
           })
         })
       })
 
       it('connects resource with image via given image type', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
         const result = await neode.cypher(
           `MATCH(p:Post {id: "p99"})-[:HERO_IMAGE]->(i:Image) RETURN i,p`,
           {},
@@ -241,7 +246,7 @@ describe('mergeImage', () => {
       })
 
       it('sets metadata', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
         const image = await neode.first<typeof Image>('Image', {}, undefined)
         await expect(image.toJson()).resolves.toMatchObject({
           alt: 'A description of the new image',
@@ -256,8 +261,6 @@ describe('mergeImage', () => {
           try {
             await session.writeTransaction(async (transaction) => {
               const image = await mergeImage(post, 'HERO_IMAGE', imageInput, {
-                uploadCallback,
-                deleteCallback,
                 transaction,
               })
               return transaction.run(
@@ -287,8 +290,6 @@ describe('mergeImage', () => {
           try {
             await session.writeTransaction(async (transaction) => {
               const image = await mergeImage(post, 'HERO_IMAGE', imageInput, {
-                uploadCallback,
-                deleteCallback,
                 transaction,
               })
               return transaction.run('Ooops invalid cypher!', { image })
@@ -314,18 +315,18 @@ describe('mergeImage', () => {
         })
 
         it('calls deleteCallback', async () => {
-          await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+          await mergeImage(post, 'HERO_IMAGE', imageInput)
-          expect(deleteCallback).toHaveBeenCalled()
+          expect(mockDeleteObjectCommand).toHaveBeenCalled()
         })
 
-        it('calls uploadCallback', async () => {
+        it('calls Upload', async () => {
-          await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+          await mergeImage(post, 'HERO_IMAGE', imageInput)
-          expect(uploadCallback).toHaveBeenCalled()
+          expect(mockUpload).toHaveBeenCalled()
         })
 
         it('updates metadata of existing image node', async () => {
           await expect(neode.all('Image')).resolves.toHaveLength(1)
-          await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+          await mergeImage(post, 'HERO_IMAGE', imageInput)
           await expect(neode.all('Image')).resolves.toHaveLength(1)
           const image = await neode.first<typeof Image>('Image', {}, undefined)
           await expect(image.toJson()).resolves.toMatchObject({
@@ -375,17 +376,17 @@ describe('mergeImage', () => {
       })
 
       it('does not call deleteCallback', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
-        expect(deleteCallback).not.toHaveBeenCalled()
+        expect(mockDeleteObjectCommand).not.toHaveBeenCalled()
       })
 
-      it('does not call uploadCallback', async () => {
+      it('does not call Upload', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
-        expect(uploadCallback).not.toHaveBeenCalled()
+        expect(mockUpload).not.toHaveBeenCalled()
       })
 
       it('updates metadata', async () => {
-        await mergeImage(post, 'HERO_IMAGE', imageInput, { uploadCallback, deleteCallback })
+        await mergeImage(post, 'HERO_IMAGE', imageInput)
         const images = await neode.all('Image')
         expect(images).toHaveLength(1)
         await expect(images.first().toJson()).resolves.toMatchObject({

backend/src/graphql/resolvers/images/imagesS3.ts

@@ -7,7 +7,6 @@ import { v4 as uuid } from 'uuid'
 
 import type { S3Configured } from '@config/index'
 import { s3Service } from '@src/uploads/s3Service'
-import { FileUploadCallback } from '@src/uploads/types'
 
 import { wrapTransaction } from './wrapTransaction'
 
@@ -17,7 +16,7 @@ export const images = (config: S3Configured) => {
   const s3 = s3Service(config, 'original')
 
   const deleteImage: Images['deleteImage'] = async (resource, relationshipType, opts = {}) => {
-    const { transaction, deleteCallback = s3.deleteFile } = opts
+    const { transaction } = opts
     if (!transaction) return wrapTransaction(deleteImage, [resource, relationshipType], opts)
     const txResult = await transaction.run(
       `
@@ -34,7 +33,7 @@ export const images = (config: S3Configured) => {
     // of an error (so throw an error). If we bulk delete an image, it
     // could very well be that there is no image for the resource.
     if (image) {
-      await deleteCallback(image.url)
+      await s3.deleteFile(image.url)
     }
     return image
   }
@@ -47,7 +46,7 @@ export const images = (config: S3Configured) => {
   ) => {
     if (typeof imageInput === 'undefined') return
     if (imageInput === null) return deleteImage(resource, relationshipType, opts)
-    const { transaction, uploadCallback, deleteCallback = s3.deleteFile } = opts
+    const { transaction } = opts
     if (!transaction)
       return wrapTransaction(mergeImage, [resource, relationshipType, imageInput], opts)
 
@@ -62,9 +61,9 @@ export const images = (config: S3Configured) => {
     const { upload } = imageInput
     if (!(existingImage || upload)) throw new UserInputError('Cannot find image for given resource')
     if (existingImage && upload) {
-      await deleteCallback(existingImage.url)
+      await s3.deleteFile(existingImage.url)
     }
-    const url = await uploadImageFile(upload, uploadCallback)
+    const url = await uploadImageFile(upload)
     const { alt, sensitive, aspectRatio, type } = imageInput
     const image = { alt, sensitive, aspectRatio, url, type }
     txResult = await transaction.run(
@@ -82,15 +81,12 @@ export const images = (config: S3Configured) => {
     return mergedImage
   }
 
-  const uploadImageFile = async (
+  const uploadImageFile = async (uploadPromise: Promise<FileUpload> | undefined) => {
-    uploadPromise: Promise<FileUpload> | undefined,
-    uploadCallback: FileUploadCallback | undefined = s3.uploadFile,
-  ) => {
     if (!uploadPromise) return undefined
     const upload = await uploadPromise
     const { name, ext } = path.parse(upload.filename)
     const uniqueFilename = `${uuid()}-${slug(name)}${ext}`
-    return await uploadCallback({ ...upload, uniqueFilename })
+    return await s3.uploadFile({ ...upload, uniqueFilename })
   }
 
   const images: Images = {