Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to use multiple S3 buckets #3214

Closed
baba43 opened this issue Sep 21, 2021 · 11 comments
Closed

How to use multiple S3 buckets #3214

baba43 opened this issue Sep 21, 2021 · 11 comments
Labels
AWS S3 Plugin that handles uploads to Amazon AWS S3 💬 Discussion Stale Old issues that haven't had activity recently

Comments

@baba43
Copy link

baba43 commented Sep 21, 2021

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?

@Murderlon
Copy link
Member

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

@Murderlon Murderlon added AWS S3 Plugin that handles uploads to Amazon AWS S3 💬 Discussion and removed Bug Triage labels Sep 28, 2021
@mifi
Copy link
Contributor

mifi commented Oct 6, 2021

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.

Object.freeze(valuesToMask)

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:

@baba43
Copy link
Author

baba43 commented Oct 6, 2021

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

@mifi
Copy link
Contributor

mifi commented Oct 6, 2021

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

@baba43
Copy link
Author

baba43 commented Oct 6, 2021

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.

@Murderlon
Copy link
Member

@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?

@baba43
Copy link
Author

baba43 commented Dec 2, 2021

@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?

@mifi
Copy link
Contributor

mifi commented Dec 6, 2021

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.

@Murderlon
Copy link
Member

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.

@stale
Copy link

stale bot commented Dec 10, 2022

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 stale bot added the Stale Old issues that haven't had activity recently label Dec 10, 2022
@mifi
Copy link
Contributor

mifi commented Mar 30, 2023

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

@mifi mifi closed this as completed Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS S3 Plugin that handles uploads to Amazon AWS S3 💬 Discussion Stale Old issues that haven't had activity recently
Projects
None yet
Development

No branches or pull requests

3 participants