Risky business

Sometimes you have to build code which introduces risk.

I recently had to write some code which clears the contents of an S3 directory (I know I know, S3 doesn't have directories...), before copying over the new contents.

Here's what that looks like:

**
 * Empties all keys in s3 starting with the given prefix (directory).
 *
 * Skips if the directory provided is the root.
 *
 * WARNING: Use this wisely!!!
 *
 * @param s3
 * @param directory
 */
export const emptyDirectory = async ({
  s3,
  directory,
}: {
  s3: ReturnType<typeof awsS3Connector>
  directory: string
}): Promise<DeleteObjectOutput | undefined> => {
  const directoryWithoutTrailingSlash = directory.endsWith('/')
    ? directory.slice(0, -1)
    : directory

  if (!directoryWithoutTrailingSlash) {
    return
  }

  const objects = await s3
    .client()
    .listObjectsV2({
      Bucket: s3.getBucket(),
      Prefix: `${directoryWithoutTrailingSlash}/`,
    })
    .promise()

  if (!objects.Contents?.length) {
    return
  }

  const deleteObjectsParams = objects.Contents.reduce(
    (carry, object) => {
      if (!object.Key) {
        return carry
      }
      return {
        ...carry,
        Delete: {
          ...carry.Delete,
          Objects: [...carry.Delete.Objects, ...[{ Key: object.Key }]],
        },
      }
    },
    { Bucket: s3.getBucket(), Delete: { Objects: [] } },
  )

  return s3.client().deleteObjects(deleteObjectsParams).promise()
}

Note the "WARNING: Use this wisely!!!". This is one way to communicate to the developers that this is a scary method and should be used with care.

But who reads code comments anyway?

When reviewing the code changes, Warren suggested following React's approach which I thought was a great idea.

If you're familiar with React, you'll know they have the method dangerouslySetInnerHTML.

This looks and sounds verbose, but I think it's a really nice naming convention. It gives the user an API but tells them to tread carefully when using it.

dangerouslySetInnerHTML should only be used when dealing with mark up which you're certain is clean and doesn't contain any vulnerabilities.

Similarly, emptyDirectory should be used with as much care.

I followed Warren's suggestion and updated it to dangerouslyEmptyDirectory.

As a side note, I also wrote a bunch of tests for this, testing all kinds of implementation details which I normally avoid.

But sometimes brittle tests are worth it for the peace of mind.

Here they are:

  describe('dangerouslyEmptyDirectory', () => {
    const listObjectsV2Spy = jest.fn()
    const deleteObjectsSpy = jest.fn()

    beforeEach(() => {
      jest.clearAllMocks()

      // @ts-ignore
      jest.spyOn(AWS, 'S3').mockImplementation(() => ({
        listObjectsV2: listObjectsV2Spy,
        deleteObjects: deleteObjectsSpy,
      }))
    })

    it('does not call deleteObjects if no data returned from listObjectsV2', async () => {
      const s3 = new AWS.S3()
      const connector = awsS3Connector(s3, 'bucket')
      const directory = 'some/directory'

      listObjectsV2Spy.mockReturnValue({
        promise: async () => {
          return {
            Contents: null,
          }
        },
      })

      deleteObjectsSpy.mockReturnValue({
        promise: async () => {
          return {}
        },
      })

      await dangerouslyEmptyDirectory({ s3: connector, directory })

      expect(listObjectsV2Spy).toHaveBeenCalledTimes(1)
      expect(listObjectsV2Spy).toHaveBeenCalledWith({
        Bucket: 'bucket',
        Prefix: 'some/directory/',
      })

      expect(deleteObjectsSpy).toHaveBeenCalledTimes(0)
    })

    it('does not call deleteObjects if the directory is the root', async () => {
      const s3 = new AWS.S3()
      const connector = awsS3Connector(s3, 'bucket')
      const directory = '/'

      listObjectsV2Spy.mockReturnValue({
        promise: async () => {
          return {
            Contents: [
              { Key: `some/directory/a` },
              { Key: `some/directory/b` },
              { Key: `some/directory/c/d` },
            ],
          }
        },
      })

      deleteObjectsSpy.mockReturnValue({
        promise: async () => {
          return {}
        },
      })

      await dangerouslyEmptyDirectory({ s3: connector, directory })

      expect(listObjectsV2Spy).toHaveBeenCalledTimes(0)
      expect(deleteObjectsSpy).toHaveBeenCalledTimes(0)
    })

    it('calls deleteObjects with each key found in the result of listObjectsV2', async () => {
      const s3 = new AWS.S3()
      const connector = awsS3Connector(s3, 'bucket')
      const directory = 'some/directory/'

      listObjectsV2Spy.mockReturnValue({
        promise: async () => {
          return {
            Contents: [
              { Key: `some/directory/a` },
              { Key: `some/directory/b` },
              { Key: `some/directory/c/d` },
            ],
          }
        },
      })

      deleteObjectsSpy.mockReturnValue({
        promise: async () => {
          return {}
        },
      })

      await dangerouslyEmptyDirectory({ s3: connector, directory })

      expect(listObjectsV2Spy).toHaveBeenCalledTimes(1)
      expect(listObjectsV2Spy).toHaveBeenCalledWith({
        Bucket: 'bucket',
        Prefix: 'some/directory/',
      })

      expect(deleteObjectsSpy).toHaveBeenCalledTimes(1)
      expect(deleteObjectsSpy).toHaveBeenCalledWith({
        Bucket: 'bucket',
        Delete: {
          Objects: [
            {
              Key: 'some/directory/a',
            },
            {
              Key: 'some/directory/b',
            },
            {
              Key: 'some/directory/c/d',
            },
          ],
        },
      })
    })
  })