uppy icon indicating copy to clipboard operation
uppy copied to clipboard

How to use multiple S3 buckets

Open baba43 opened this issue 2 years ago • 9 comments

Hello, I'm trying to connect a second S3 bucket to my Node.js companion server.

After looking at the configuration, I did not find any option to specify more than one bucket for the same instance, so I thought about adding a second instance on another route.

So far so good, but as soon as I start the server, I get this error:

TypeError: Cannot add property 1, object is not extensible
    at Array.push (<anonymous>)
    at ..\node_modules\@uppy\companion\lib\server\logger.js:14:22
    at Array.forEach (<anonymous>)
    at Object.exports.setMaskables (..\node_modules\@uppy\companion\lib\server\logger.js:13:15)
    at maskLogger (..\node_modules\@uppy\companion\lib\companion.js:181:12)
    at Object.module.exports.app (..\node_modules\@uppy\companion\lib\companion.js:61:5)
    at createUppyCompanion (..\src\routes\uppy\createUppyEndpoint.ts:43:20)
    at Object.createUppyEndpoint (..\src\routes\uppy\createUppyEndpoint.ts:16:14)
    at Object.<anonymous> (..\src\routes\uppy\uppyArchiveRoute.ts:4:33)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)

I can temporary fix this error by commenting out this line:

exports.setMaskables = (maskables) => {
    maskables.forEach((i) => {
        valuesToMask.push(escapeStringRegexp(i));
    });
    //Object.freeze(valuesToMask);
};

Now, my question is, if this is a bug or how to support multiple buckets otherwise?

baba43 avatar Sep 21 '21 12:09 baba43

@mifi are multiple buckets supported? Should that be possible if not?

Murderlon avatar Sep 28 '21 15:09 Murderlon

Uploading each file to multiple destinations

I assume what you wanted to do was to upload each file to multiple S3 buckets instead of one. I'm not sure if we want to implement this because it would change the uppy/companion APIs. i.e. each uploaded file would have to return an array of upload success objects or trigger multiple success events each.

Maybe you can add a lambda trigger on the s3 bucket to clone it to a different bucket?

Multiple companion instances

I think companion was never designed to be instantiated many times. Although judging companion's API (companion.app() looks like a function that could be called many times to get many independent instances), I agree we should let the developer instantiate multiple companions, unless it requires a huge rewrite. If not, we should at least throw a better error if the developer tries to call companion.app() multiple times.

I think the error you're getting is because of the Object.freeze(valuesToMask), so it cannot be called again. I'm not sure why the object needs to be frozen. https://github.com/transloadit/uppy/blob/5cbc9981fdae45c7758ca4f30bc9d122a347bd48/packages/%40uppy/companion/src/server/logger.js#L17

I think that in order for multiple instances of companion to be supported, the components that use globals/singletons need to be rewritten. A quick search reveals that these modules may have to be rewritten to support this:

  • https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/logger.js
  • https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/emitter/default-emitter.js
  • https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/emitter/index.js
  • https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/jobs.js
  • https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/redis.js

mifi avatar Oct 06 '21 16:10 mifi

Thanks for your time @mifi.

Yes, I also found out that Object.freeze causes this error and at least for my use case the basic stuff seems to work when I just comment that line out. FYI

baba43 avatar Oct 06 '21 16:10 baba43

Cool. Out of curiosity, may you share what is your use case for uploading to multiple s3 buckets?

mifi avatar Oct 06 '21 16:10 mifi

Cool. Out of curiosity, may you share what is your use case for uploading to multiple s3 buckets?

Well, our system includes several functions, some of which operate with different buckets for the purpose of organization, permissions, etc.

For me, the use of multiple buckets was nothing special. There are several reasons why a server needs to support multiple buckets or even multiple AWS accounts.

So I think it would at least make sense if we could use multiple Uppy instances in the long run, even if they might not run optimized then. If my workaround hadn't worked, my only option would have been to set up a second server.

baba43 avatar Oct 06 '21 17:10 baba43

@mifi @kvz do you feel we should implement this at some point or can we say we likely never implement this and close the issue?

Murderlon avatar Dec 02 '21 13:12 Murderlon

@mifi @kvz do you feel we should implement this at some point or can we say we likely never implement this and close the issue?

I think it would be great if we could at least fix this error. You could say that it is not officially supported, but for now the workaround above works for us, so I guess people should be using it at their own risk?

baba43 avatar Dec 02 '21 13:12 baba43

I think just removing object.freeze is not the right way to do it, because then two app instances created with different configs will compete to overwrite this singleton variable, as well as cause issues with the other singletons/globals that are created. I think it boils down to whether we want to do the effort of rewriting all the singleton global state and instead place it inside the companion instance / closure.

if not ,then I think we should throw a better error message if someone tries to initialize the companion app twice.

mifi avatar Dec 06 '21 07:12 mifi

It's hard for me to tell whether it's common to want multiple app instances in this setup and therefore worth the rewrite. I'll leave that choice to you and/or @kvz. If not then I agree a specific error message is a nice quick improvement.

Murderlon avatar Dec 06 '21 09:12 Murderlon

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Dec 10 '22 06:12 stale[bot]

closing this but i created a new feature request for allowing people to implement their own uploaders: #4390

mifi avatar Mar 30 '23 13:03 mifi