APIv3: proxy these URLs to be served from El Proxito /_/api/v3/
#11831
Conversation
humitos
commented
Dec 9, 2024
humitos
commented
- Related: API: consider using APIv3 standard endpoints addons#356
- Related: API: use APIv3 endpoint for resources addons#468
|
|
||
| """Mixin to remove conflicting fields from serializers.""" | ||
|
|
||
| FIELDS_TO_REMOVE = ("_links",) |
There was a problem hiding this comment.
Adding links back may cause a snowball of dependencies, requiring to have all endpoints exposed in the proxied API. I wonder if another alternative can be to have a different and more restricted API under the addons umbrella instead of trying to move the whole API.
There was a problem hiding this comment.
I want to avoid having two different API responses one from the dashboard and one from proxito if possible. That will increase maintenance work and also will add confusion. Let's try how it works with this initial implementation that just proxies the APIv3 but read-only using the exact same response for now and see how it goes.
|
I think this PR is going in the direction we want and it's almost ready to replace the addons API endpoint. The only thing I'm not being able to do correctly is to handle the |
|
I did an initial implementation for the required sorting at DB level using a solution I found o a blog post: |
|
@stsewd I think your review here is probably most useful. I looked at the code and it seems reasonable, but not 100% sure about the robustness of the read-only checking, but didn't have an obvious suggestion to improve it. |
| @@ -110,7 +125,7 @@ class APIv3Settings: | |||
| LimitOffsetPagination.default_limit = 10 | |||
|
|
|||
| renderer_classes = (AlphabeticalSortedJSONRenderer, BrowsableAPIRenderer) | |||
| throttle_classes = (UserRateThrottle, AnonRateThrottle) | |||
| # throttle_classes = (UserRateThrottle, AnonRateThrottle) | |||
There was a problem hiding this comment.
I temporary commented this because I was blocked immediately since we have the throttle very low in our settings.
readthedocs.org/readthedocs/settings/base.py
Lines 902 to 905 in e70cf31
We should increase these values to be able to use APIv3 in the way we want with addons.
| if self.request.path.startswith(f"/{settings.DOC_PATH_PREFIX}"): | ||
| permission_classes = [ReadOnlyPermission] |
There was a problem hiding this comment.
This should probably be a kind of override at the view class level or a setting that detects if this is running under proxito instead of relying on the path.
There was a problem hiding this comment.
also, this is only being done for the projects viewsets, it needs to be done for the whole API.
| api_proxied_urls = [ | ||
| path("embed/", ProxiedEmbedAPI.as_view(), name="embed_api_v3"), | ||
| path("search/", ProxiedSearchAPI.as_view(), name="search_api_v3"), | ||
| ] | ||
|
|
||
| urlpatterns = api_proxied_urls | ||
| urlpatterns += router.urls |
There was a problem hiding this comment.
We are proxying the whole API here instead of what addons needs, this has several implications:
- Duplicate endpoints
- Attack surface is greater. Preventing XSS vulnerabilities in doc pages is not under our total control (as it depends on the user more). We won't be exposing write operations, but the attack surface is greater now (but you can argue that the most important information is docs content, project, and version, which is already possible to get, but it's harder to list all projects for example).
- Permissions need to be adapted, especially for .com, as addons needs to work with password and token access, we can't assume we always have a user.
- Caching as well will need to handle per-view depending on what data gets staled.
- I think we did lots of optimizations in the addons response, which we may be loosing when migrating everything to API v3.
I'd suggest we explicitly bring what we need and adapt it to work under proxito (as we have done with search and embed APIs) instead of trying to bring the whole API v3 at once. For example, we don't need to list all projects from docs pages, or redirects.
|
I'm re-considering the work on this PR based on the benefits vs. the work required. There is a lot of work we need to do here:
All this work will have impact on the caching rules mainly. Currently, the median response time for the addons request is 150ms and ~40% of its time is Python code (probably generating/dumping the serializers). The DB queries has been optimized over time and I think there isn't too much else we can do there -- they are fast enough. I checked the times for the same DB queries on each of the APIv3 endpoints and they are similar, so we won't be earning too much. Another thing to consider is that when hitting the addons API, we are currently sharing the resolver object as much as possible which reduces the number of DB queries. This won't be the case if we use multiple API endpoints. However, some of them should be cached (in theory), tho. I'm arriving at the conclusion that what we have works for now and it may not worth the effort/work right now considering the benefits. I think if we find that we exceed by far those 150ms in the addons response and we can prove there is going to be improvements using single cached endpoints, we can reconsider it in the future. Taking a look at CF stats, in the last 30 days, only 13% of requests for I'd propose to de-prioritize this work and use this time to something with more impact, at least for now. We can revisit this work in the future and re-evaluate based on cost vs. benefits again. @stsewd @ericholscher What do you think? |
