DRAFT: use web-search api-params - #125
Merged
Merged
Conversation
Update api-client github URL Discuss updating server API_PYTHON_CLIENT info
Contributor
Author
|
NOTE! The api-params call is currently only available from the staging web-search server! |
fixes mediacloud#123 * Use admin connection for higher rates * Force higher rates for directory (rates not enforced) * Keep API objects around in search tests to keep rate limiter state * Removed all sleeps!
rahulbot
self-requested a review
September 22, 2026 07:50
rahulbot
approved these changes
Sep 22, 2026
rahulbot
left a comment
rahulbot
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Clever approach to dynamically setting rate limit based on server allowance for user. I'm not sure about the changes from _search to _admin_search... is that for speed? Just want to make sure it doesn't leave us exposed to somehow breaking regular user searches. Otherwise typing additions look good too.
Contributor
Author
|
LGTM. Clever approach to dynamically setting rate limit based on server allowance for user.
Thanks!
I'm not sure about the changes from _search to _admin_search... is that for speed?
Yes.
Just want to make sure it doesn't leave us exposed to somehow breaking regular user searches.
You would be a better QA person than I!!!
There should only be five aspects where "is_staff" has EXPLICIT effect:
1. quota enforcement
2. default rate limit
3. whether pulling a single story by id comes with full text
4. whether expanded or randomize arguments for story_list are accepted
5. whether the "recent requests" API endpoint works
If one isn't willing to trust that (and a good QA person shouldn't),
then we should do ALL operations with both kinds of users (actually
there are even more kinds of users: ones in different permutations of
groups!), and verify correct behavior.
For having one's cake AND eating it, the tests could take THREE keys
from the environment (and create three API objects): one staff, one
vanilla user, and a third for executing queries than can be done
either way, which if you have patience, you can set to a vanilla user,
or if you want QUICK validation (as changed it runs in under 30
seconds) you can supply an admin key. The truely paranoid can run the
tests both ways!!
BTW there were previously no tests for correct admin/user behavior;
I added one for fetching a single story; did it both ways verifying
that the text was there or not.
Do we have much in the way of testing negative/error cases?
I'm more inclined to trust non-mocked, over the network tests of the
API than I am mocked tests of the endpoints. In my experience the
latter tend to be very brittle.
In any case, I'm not going to merge this until the web-search backend
with the api-params endpoint is in production.
I think we should consider releasing this as 6.0, and remove the
stories_by_source_week method at the same time, since the backend call
is gone (IMO this was done in the wrong order: the library call should
have been removed first), see
#126
|
* Restarted with version from main * Kept global API sessions from new version * Added check for MC_API_TEST_FAST (runs in about 20 sec vs 20 minutes) (by modifying contents of self._search vs using _admin_search) * Made test_story explicitly use _user_search * Kept test_story_admin (explicitly using _admin_search) * Kept addition of collections arg in stories_by_source_over_interval tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
draft using web-search search/api-params call to:
To avoid confusion (and adhere to comments) this version goes thru some gyrations to allow the user to patch URL and rate limit AFTER instantiating an API object, but not as many as my first version (which implemented caching). Users who create a new API object for each call they make will fetch the api-params on each request!
Open issues: