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

Rate limiting updates #141

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

abaevbog
Copy link
Contributor

@abaevbog abaevbog commented Jun 5, 2023

  1. Using $this->end() instead of exit in controllers. It ensures that concurrency rate limiter counts requests as finished.
  2. Make client wait for full capacity once rate limit is reached.

@abaevbog abaevbog requested a review from dstillman June 5, 2023 16:58
@abaevbog abaevbog changed the title Using $this->end() instead of exit in controllers Rate limiting updates Jun 5, 2023
@dstillman
Copy link
Member

So, getting rid of exit is good for other reasons, but apparently @mrtcode fixed this 6 years ago just by moving finishConcurrentRequest to a shutdown handler. Now merged.

@@ -34,7 +34,9 @@ public function mappings() {
$locale = !empty($_GET['locale'])
? \Zotero\Schema::resolveLocale($_GET['locale'])
: 'en-US';

unset($this->queryParams['format']);
header("Content-Type: application/json");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these lines necessary? format defaults to JSON, so I would think that outputContentType() in end() would get it right. I think we were only calling header() because we weren't using end().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these 2 lines can actually be removed.
The same 2 lines in the newItem function are still needed to get the right application/json content type for API v2



unset($this->queryParams['format']);
header("Content-Type: application/json");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just set 'json' as the default format for the mappings actions for v1/v2 in API.inc.php?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That is definitely cleaner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants