storyblok / storyblok-cli

Storyblok CLI
https://www.npmjs.com/package/storyblok
MIT License
31 stars 30 forks source link

storyblok push-components aborts with "Error: Too Many Requests" #64

Open eggnstone opened 1 year ago

eggnstone commented 1 year ago

Current behavior: I'm uploading my components.json (with 21 components) to an existing space: storyblok push-components --space 12345 components.json After a few successful updates it reliably aborts like this:

- Executing push-components task
...
- Updating component XYZ...
X An error occurred when load components file from api
X An error occurred when executing the push-components task: Error: Too Many Requests

Expected behavior: Successful execution of the command without an error.

Steps to reproduce: storyblok push-components --space 12345 components.json

Other information: This is probably due to being on a limited plan where the API calls have a limited rate. That rate should be adhered to or at least be made configurable.

Edit storyblok-cli version: 3.25.0 It used to work with earlier version.

Edit2 It used to work with earlier versions like 3.24.1 where subtasks could fail, too, but it did not abort:

- Updating component LinkedBox...
- Checking target presets to sync...
X An error occurred when update component LinkedBox
Request failed with status code 429
- Get presets from component PageTemplate
- Updating component PageTemplate...
Hit rate limit. Retrying in 1 seconds.
- Checking target presets to sync...
✓ Component PageTemplate has been updated in Storyblok!
ademarCardoso commented 1 year ago

Hi @eggnstone thanks to reporting the problem, could you check again ? We had an update on our API's

If you test and the problem persist, please can you send me the current plan of the source and target spaces, to check if it's a limitation by the plan.

I say this because this 429 usually only happens if you exceed the request limit within a certain period of time, but to be sure, I'll need to know the plans, but only if the problem is still happening.

eggnstone commented 1 year ago

Yes, still happening with 3.25.1 Plan: Development Space: 188619

But even if it's a limitation by the plan, the CLI should adjust to that and not abort with error.

ademarCardoso commented 1 year ago

Thanks to confirm the problem, i will investigate that.

ademarCardoso commented 1 year ago

Hello again my friend @eggnstone i added some changes and published the version 3.25.2, could you check to see if this version it's working ?

eggnstone commented 1 year ago

Same error:

- Updating component BlogPosts...
X An error occurred when load components file from api
X An error occurred when executing the push-components task: Too Many Requests
  at new Promise (<anonymous>)
    at w._statusHandler (C:\Users\Mark\AppData\Roaming\npm\node_modules\storyblok\node_modules\storyblok-js-client\dist\index.umd.js:24:5206)
    at w._methodHandler (C:\Users\Mark\AppData\Roaming\npm\node_modules\storyblok\node_modules\storyblok-js-client\dist\index.umd.js:24:5083)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async push (C:\Users\Mark\AppData\Roaming\npm\node_modules\storyblok\src\tasks\push-components.js:164:11)
    at async Command.<anonymous> (C:\Users\Mark\AppData\Roaming\npm\node_modules\storyblok\src\cli.js:170:7)

I always add

// sleep to avoid rate limit
await new Promise(resolve => setTimeout(resolve, 250));

in push-components.js at line 169 before

        await api.put(`components/${id}`, {
            component: components[i]
          })
ademarCardoso commented 1 year ago

Hi @eggnstone ,this error doesn't happen to me (I tried to push 500 components), I opened a PR draft could you take a look and try to use this approach and let me know if it works or not.

If that doesn't work, I should go with your idea, I didn't go with it the first time because it can add a certain amount of time to push many components, but it's a solution.

Thanks a lot for the help.

eggnstone commented 1 year ago

I'm no JS expert but the PR looks to me like you are collecting the push results and then wait for them at the end, but because you await each result before adding it to the array, I don't see any difference. If you would add to the array without awaiting it, the problem would even increase, I think.

I don't see how this can address the "too many requests" which basically means "too many calls per second" because I am on a limited plan.

Is there a way to determine the plan? Then based on that plan a delay could be added before each api.put()

marcesengel commented 1 year ago

@ademarCardoso would you be open to accept a PR introducing exponential back-off? We're having the same issue (strangely with very few components, I counted 7). The space currently is on the community plan, that should be the easiest way to reproduce the issue because of it having the lowest rate limits.

Screenshot

image

marcesengel commented 1 year ago

After digging a bit deeper, I've found this official post concerning the management api rate limits. There it says

Customers using Storyblok’s official nodejs SDK “storyblok-js-client” are not affected by this change as the module already handles throttling the requests to the right level.

As this CLI tool is utilising the SDK, I now am left wondering if this is an upstream error?

PhiFry commented 1 year ago

I think the issue lies in https://github.com/storyblok/storyblok-cli/blob/master/src/utils/api.js

image

This function creates a new instance of Storyblok js sdk everytime the CLI wants to do a request to the REST API. So if my push file has more than 3 or 6 components and my internet connection is fast then I dont think it throttles itself.

Proposed solution

make getClient() cache a Storyblok instance using lazy loading.

PhiFry commented 1 year ago

Can confirm my solution worked, modified script locally and now my push components command works and it is clearly throttling now.

Screenshot 2023-09-11 at 09 26 05
marcesengel commented 1 year ago

@PhiFry yeah that'd make sense - however the push function itself is passed the api as a parameter, see https://github.com/storyblok/storyblok-cli/blob/master/src/tasks/push-components.js#L79... So I'm assuming that where the function is invoked multiple clients are used?

PhiFry commented 1 year ago

@marcesengel The api object is indeed passed and not created multiple times, the issue is the Storyblok object (which is the Storyblok js SDK object) is being re-created for each component when it calls the put and post commands to upload the component to Storyblok and thus the internal throttle mechanism gets reset each time.

marcesengel commented 1 year ago

@PhiFry ah I see 🙌 do you think you could provide a quick PR so we can install the CLI from your fork instead of from npm? That'd be very kind 🙈

marcesengel commented 1 year ago

@eggnstone @PhiFry I've created a fork (+ PR, see above) implementing @PhiFry's findings. Feel free to give it a try using npm i -D cashy-at/storyblok-cli#fix/request-throttling.

mikewoudenberg commented 9 months ago

Hi, Doesn't this fix (https://github.com/storyblok/storyblok-js-client/pull/732) also fix the underlying issue here? If so, could you roll out a new version with the updated dependency?

dojscart commented 7 months ago

Hi @ademarCardoso, is there any chance this could be fixed? That way, Storyblok's enterprise customers that have to use SSO logins can use the official Storyblok CLI rather than a fork.

Many thanks!

christianzoppi commented 4 months ago

Hi everyone! Thanks for sharing the details We recently merged #72 created by @marcesengel, which aims to mitigate the issue. Since v3.31.1 you shouldn't face this issue anymore, at least reaching the limit should be harder. If you still face this issue with that version please let us know.

alvarosabu commented 1 month ago

Hi all, I'm gonna close this as completed since it was released on v3.31.1, if you still encounter this issue please feel free to re-open.

oe-josh-martin commented 3 weeks ago

@christianzoppi @alvarosabu unfortunately we're still seeing this issue with 3.32.2.

We have a script that allows people to pull from a centralised space and push out to their own spaces. They enter their space ID from a readline prompt ➜ Please enter your Storyblok Space ID (e.g. 245488): 144190.

The centralised space is 240757, and the consumer space in this instance is 144190.

const componentsPath = path.join(
    __dirname,
    '../types/generated/storyblok/components.240757.json'
);

storyblok push-components --space ${SPACE_ID} ${componentsPath} && storyblok sync --type datasources --source 240757 --target ${SPACE_ID}`

However, we're still being met with the Too Many Requests error:

➜ Please enter your Storyblok Space ID (e.g. 245488): 144190
stderr: X An error occurred when update component coralBlokChip
Too Many Requests

In our implementation we catch any stderr early and exit the task, which is why there's only one error listed before the task exits.

exec(command, (error, stdout, stderr) => {
    if (error) {
        return console.log(`Error: ${error.message}`);
    } else if (stderr) {
        return console.log(`stderr: ${stderr}`);
    } else {
        console.log(stdout);
        console.log('🎉 Done!');
    }
});