Error handling in collection hooks.

default discord avatar
richardvanbergen
2 years ago
1 3

I'm trying to upload to a DigitalOcean spaces instance and I'm implementing the recommended hooks as specified in the documentation.

https://payloadcms.com/docs/production/deployment

I've implemented an upload hook (see attached) but I've got two comments.

  1. Is there any way to inform the use if something goes wrong?
  2. There may be a bug in regards to duplicate filenames, or possibly just a bad implementation on my part. The beforeChange hook doesn't pass the filename that it's going to be saved as. For instance if I upload image.jpg and that filename already exists twice then Payload will save it down as image-2.jpg but only after the beforeChange hook has executed, and data.filename will be image-1.jpg. So it gets saved down in the storage with that name as the key.
const uploadHook: BeforeChangeHook = async ({ data, req }) => {
  try {
    const s3 = new AWS.S3({
      endpoint: (new AWS.Endpoint(process.env.SPACES_REGION)),
      accessKeyId: process.env.SPACES_KEY,
      secretAccessKey: process.env.SPACES_SECRET
    })

    const response = await s3.putObject({
      Bucket: process.env.SPACES_NAME,
      Key: String(data.filename),
      Body: req.file,
      ACL: "private"
    }).promise()

    console.log(response)

    return data
  } catch (e) {
    console.error(e)

    return {}
  }
}
  • discord user avatar
    jmikrut
    Payload Team
    2 years ago

    Hey @richardvanbergen I love it. I'd be pumped to add this as a tutorial / guide for Payload. We have had this on our list to write about but haven't gotten to it yet.

    Couple answers:

    Is there any way to inform the use if something goes wrong?

    Yes, just throw an error and the error's message will bubble up through the API(s). You should be able to then see it in a response. Give it a shot! We have APIError exposed within payload/errors which can be leveraged if you want for status codes, etc. but you could also just throw any error you want.

    There may be a bug in regards to duplicate filenames, or possibly just a bad implementation on my part. The beforeChange hook doesn't pass the filename that it's going to be saved as. For instance if I upload image.jpg and that filename already exists twice then Payload will save it down as image-2.jpg but only after the beforeChange hook has executed, and data.filename will be image-1.jpg. So it gets saved down in the storage with that name as the key.

    You're right - the filename gets generated in our create operation after the beforeChange hook is fired, but it doesn't have to be this way. We should be able to modify accordingly 👍 pretty quickly actually. Would this benefit you?

    6 replies
  • default discord avatar
    richardvanbergen
    2 years ago

    Yes, just throw an error and the error's message will bubble up through the API(s).

    So is it OK to just allow the S3 errors to bubble up then? Cool!

    We should be able to modify accordingly 👍 pretty quickly actually. Would this benefit you?

    Unless there's a use case I'm not thinking about - I think it makes more sense.

    It should be how it's going to be saved onto the file system. As it stands the only accurate way for me to get the filename is use the AfterChangeHook which means that if there's an error in the S3 upload then it still gets saved to the database, meaning we might not be able to fetch it later.

    I'd be pumped to add this as a tutorial / guide for Payload. We have had this on our list to write about but haven't gotten to it yet.

    No problem. I was actually thinking about tidying it up, adding some tests and docs and throwing it up on npm. You're also welcome to steal any code you like from me in the meantime. (Though, you might want to fix it before you use it 😄 )

    Here is my prototype: https://gist.github.com/richardvanbergen/a21c701632a2f24a013d8e17495f3e2c

    Used like so:

    const s3Hooks = withS3Storage(
      {
        endpoint: (new AWS.Endpoint(`${process.env.SPACES_REGION}.digitaloceanspaces.com`)),
        accessKeyId: process.env.SPACES_KEY,
        secretAccessKey: process.env.SPACES_SECRET,
      },
      {
        bucket: process.env.SPACES_NAME
      },
      {
        beforeValidate: [
          // ...
        ]
      } // <- combine with other hooks here
    )
    
  • default discord avatar
    richardvanbergen
    2 years ago

    One more question, can I alter the data that's going to be saved in the beforeChange hook? Like adding an extra field? I was thinking about updating the file on S3 based on the MD5 hash, then all the filename problems go away.

  • discord user avatar
    jmikrut
    Payload Team
    2 years ago

    We'll get a new version published that offers up the filename into beforeChange hooks exactly how it will be saved to the database. Will get to that in the next few days here. 👍

    One more question, can I alter the data that's going to be saved in the beforeChange hook? Like adding an extra field? I was thinking about updating the file on S3 based on the MD5 hash, then all the filename problems go away.

    Yes you absolutely can modify the data in the beforeChange hook. You may be thinking about the same thing I just commented elsewhere on this Discussion:

    #98 (reply in thread)

    You could create a "virtual" field with field hooks and have it be nice and tidy. Is this what you mean?

  • default discord avatar
    richardvanbergen
    2 years ago

    Yes! The virtual field works quite nicely! I'll be looking forward to the beforeChange change too.

    I've just had one more thought, what about the admin thumbnails? If the storage is ephemeral you could end up in a situation with the frontend being fine but the admin breaking. Because as far as I can tell, there's no way to specify the thumbnail source?

  • discord user avatar
    jmikrut
    Payload Team
    2 years ago

    Great call.

    Just merged a PR that extends the upload.adminThumbnail property by, in addition to only allowing it to specify a size, it now also allows a function that returns a full URL to use as a thumbnail. Updated the docs as well with these changes. You can use this function to set an adminThumbnail however you'd like.

    #104

    Keep these thoughts coming! Really appreciate the feedback.

  • discord user avatar
    DanRibbens
    Payload Team
    2 years ago

    I opened a new discussion specific to integrating file hosting with Payload uploads. @richardvanbergen if you have an answer for what worked in your case that would be awesome to share. 😃
    #220

  • default discord avatar
    richardvanbergen
    2 years ago

    Also, could we re-export the collection hooks from payload/types?

    2 replies
    discord user avatar
    jmikrut
    Payload Team
    2 years ago

    You mean the collection hook types? Absolutely. On it!

    discord user avatar
    jmikrut
    Payload Team
    2 years ago

    This has been merged to master and is awaiting release in the next version (0.4.6). Should be deployed later today.

  • default discord avatar
    richardvanbergen
    2 years ago

    Sorry one more question/bug. I'm altering the filename as suggested. But the URL that's being returned is prepended with /media/images. For example: /media/images/https://brightvision.fra1.digitaloceanspaces.com.cdn.digitaloceanspaces.com/contact-image-10-8.jpg

    const readHook: AfterReadHook = async ({ doc }) => {
      try {
        console.log(doc)
        return {
          ...doc,
          filename: `https://${process.env.SPACES_NAME}.${process.env.SPACES_REGION}.cdn.digitaloceanspaces.com/${doc.filename}`
        }
      } catch (e) {
        return doc;
      }
    }
    
    3 replies
    discord user avatar
    jmikrut
    Payload Team
    2 years ago

    Now this one has stumped me. You are altering filename as suggested? Can you show the code that you are using for that? There might be a bug in that implementation. Might need a bit more info on this one!

    default discord avatar
    richardvanbergen
    2 years ago

    Sorry I'm confused. Aren't I supposed to be altering and returning the filename using the AfterReadHook as I posted above? That seems to be what the documentation is suggesting.

    Should I be altering it elsewhere?

    discord user avatar
    jmikrut
    Payload Team
    2 years ago

    I have no idea why, but I totally did not realize you were providing the hook code. You can alter filename through hooks, and we do mention that in the docs. If you did that though, you would want to make sure to "un-modify" it through a beforeChange hook or similar, because the filename will have been "read-in" as your modified value, and you may end up with some unintended consequences.

    To clarify, every time a document is read, even before it's updated, the hooks are run. So an afterRead hook should be careful to not create a modification "loop" or similar.

    I would say that personally I would add a different field to your Collection, maybe one that doesn't ever have a value saved to the database and is more considered a "virtual". Say...:

    {
      name: 's3URL',
      type: 'text',
      admin: {
        readOnly: true,
      },
      hooks: {
        beforeChange: [
          () => null,
        ],
        afterRead: [
          ({ data }) => `https://${process.env.SPACES_NAME}.${process.env.SPACES_REGION}.cdn.digitaloceanspaces.com/${data.filename}`
        ]
      }
    }

    That way, this field is never really saved in your database, and you can treat it like a "virtual".

    What do you think? PS, I'll update the docs to be more descriptive as they are currently only half-accurate.

Open the post
Continue the discussion in GitHub
Like what we're doing?
Star us on GitHub!

Star

Connect with the Payload Community on Discord

Discord

online

Can't find what you're looking for?

Get help straight from the Payload team with an Enterprise License.