Skip to content

Commit

Permalink
Addons: accept project-slug and version-slug on endpoint (#10823)
Browse files Browse the repository at this point in the history 
* Addons: accept `project-slug` and `version-slug` on endpoint

`/_/addons/` API endpoint now accepts `project-slug` and `version-slug`.
This is useful to be able to cache these requests in our CDN.

It still supports sending `url=` when necessary for those requests that relies
on the full URL to generate specific data in the response.

The NGINX config is updated to inject `meta` tags with project/version slugs,
emulating what our CF worker does on production.

Closes #10536

* Add tests and update logic for API permissions
  • Loading branch information
@humitos
humitos authored Oct 24, 2023
1 parent 687f96e commit 00d188e
Show file tree
Hide file tree
Showing 4 changed files with 198 additions and 74 deletions.
5 changes: 3 additions & 2 deletions dockerfiles/nginx/proxito.conf.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,10 @@ server {
set $rtd_force_addons $upstream_http_x_rtd_force_addons;
add_header X-RTD-Force-Addons $rtd_force_addons always;

# Inject our own script dynamically
# Inject our own script dynamically and project/version slugs into the HTML to emulate what CF worker does
# TODO: find a way to make this work _without_ running `npm run dev` from the `addons` repository
sub_filter '</head>' '<script language="javascript" src="http://localhost:8000/readthedocs-addons.js"></script>\n</head>';
sub_filter '</head>' '<script async language="javascript" src="http://localhost:8000/readthedocs-addons.js"></script>\n<meta name="readthedocs-project-slug" content="$rtd_project" />\n<meta name="readthedocs-version-slug" content="latest" />\n</head>';
sub_filter_types text/html;
sub_filter_last_modified on;
sub_filter_once on;
}
Expand Down
11 changes: 11 additions & 0 deletions readthedocs/api/v2/permissions.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ class IsAuthorizedToViewVersion(permissions.BasePermission):
def has_permission(self, request, view):
project = view._get_project()
version = view._get_version()

# I had to add this condition here because I want to return a 400 when
# the `project-slug` or `version-slug` are not sent to the API
# endpoint. In those cases, we don't have a Project/Version and this
# function was failing.
#
# I think it's a valid use case when Project/Version is invalid to be
# able to return a good response from the API view.
if project is None or version is None:
return True

has_access = (
Version.objects.public(
user=request.user,
Expand Down
78 changes: 78 additions & 0 deletions readthedocs/proxito/tests/test_hosting.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,3 +498,81 @@ def test_flyout_subproject_urls(self):
},
]
assert r.json()["addons"]["flyout"]["translations"] == expected_translations

def test_send_project_not_version_slugs(self):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
"api-version": "0.1.0",
"client-version": "0.6.0",
"project-slug": self.project.slug,
},
secure=True,
headers={
"host": "project.dev.readthedocs.io",
},
)
assert r.status_code == 400
assert r.json() == {
"error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'"
}

def test_send_version_not_project_slugs(self):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
"api-version": "0.1.0",
"client-version": "0.6.0",
"version-slug": self.version.slug,
},
secure=True,
headers={
"host": "project.dev.readthedocs.io",
},
)
assert r.status_code == 400
assert r.json() == {
"error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'"
}

def test_send_project_version_slugs(self):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
"api-version": "0.1.0",
"client-version": "0.6.0",
"project-slug": self.project.slug,
"version-slug": self.version.slug,
},
secure=True,
headers={
"host": "project.dev.readthedocs.io",
},
)
assert r.status_code == 200
expected_response = self._get_response_dict("v0")
# Remove `addons.doc_diff` from the response because it's not present when `url=` is not sent
expected_response["addons"].pop("doc_diff")

assert self._normalize_datetime_fields(r.json()) == expected_response

def test_send_project_version_slugs_and_url(self):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
"api-version": "0.1.0",
"client-version": "0.6.0",
"url": "https://project.dev.readthedocs.io/en/latest/",
# When sending `url=`, slugs are ignored
"project-slug": "different-project-slug-than-url",
"version-slug": "different-version-slug-than-url",
},
secure=True,
headers={
"host": "project.dev.readthedocs.io",
},
)
assert r.status_code == 200
assert self._normalize_datetime_fields(r.json()) == self._get_response_dict(
"v0"
)
178 changes: 106 additions & 72 deletions readthedocs/proxito/views/hosting.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from readthedocs.core.resolver import resolver
from readthedocs.core.unresolver import UnresolverError, unresolver
from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.projects.models import Feature
from readthedocs.projects.models import Feature, Project

log = structlog.get_logger(__name__) # noqa

Expand All @@ -47,10 +47,19 @@ class BaseReadTheDocsConfigJson(CDNCacheTagsMixin, APIView):
Attributes:
url (required): absolute URL from where the request is performed
(e.g. ``window.location.href``)
api-version (required): API JSON structure version (e.g. ``0``, ``1``, ``2``).
project-slug (required): slug of the project.
Optional if "url" is sent.
version-slug (required): slug of the version.
Optional if "url" is sent.
url (optional): absolute URL from where the request is performed.
When sending "url" attribute, "project-slug" and "version-slug" are ignored.
(e.g. ``window.location.href``).
client-version (optional): JavaScript client version (e.g. ``0.6.0``).
"""

http_method_names = ["get"]
Expand All @@ -61,61 +70,73 @@ class BaseReadTheDocsConfigJson(CDNCacheTagsMixin, APIView):
@lru_cache(maxsize=1)
def _resolve_resources(self):
url = self.request.GET.get("url")
if not url:
# TODO: not sure what to return here when it fails on the `has_permission`
return None, None, None, None
project_slug = self.request.GET.get("project-slug")
version_slug = self.request.GET.get("version-slug")

project = None
version = None
build = None
filename = None

unresolved_domain = self.request.unresolved_domain

# Main project from the domain.
project = unresolved_domain.project

try:
unresolved_url = unresolver.unresolve_url(url)
# Project from the URL: if it's a subproject it will differ from
# the main project got from the domain.
project = unresolved_url.project
version = unresolved_url.version
filename = unresolved_url.filename
# This query should use a particular index:
# ``builds_build_version_id_state_date_success_12dfb214_idx``.
# Otherwise, if the index is not used, the query gets too slow.
build = version.builds.filter(
success=True,
state=BUILD_STATE_FINISHED,
).last()

except UnresolverError as exc:
# If an exception is raised and there is a ``project`` in the
# exception, it's a partial match. This could be because of an
# invalid URL path, but on a valid project domain. In this case, we
# continue with the ``project``, but without a ``version``.
# Otherwise, we return 404 NOT FOUND.
project = getattr(exc, "project", None)
if not project:
raise Http404() from exc

version = None
filename = None
build = None
if url:
try:
unresolved_url = unresolver.unresolve_url(url)
# Project from the URL: if it's a subproject it will differ from
# the main project got from the domain.
project = unresolved_url.project
version = unresolved_url.version
filename = unresolved_url.filename
# This query should use a particular index:
# ``builds_build_version_id_state_date_success_12dfb214_idx``.
# Otherwise, if the index is not used, the query gets too slow.
build = version.builds.filter(
success=True,
state=BUILD_STATE_FINISHED,
).last()

except UnresolverError as exc:
# If an exception is raised and there is a ``project`` in the
# exception, it's a partial match. This could be because of an
# invalid URL path, but on a valid project domain. In this case, we
# continue with the ``project``, but without a ``version``.
# Otherwise, we return 404 NOT FOUND.
project = getattr(exc, "project", None)
if not project:
raise Http404() from exc

else:
project = Project.objects.filter(slug=project_slug).first()
version = Version.objects.filter(slug=version_slug, project=project).first()
if version:
build = version.builds.last()

return project, version, build, filename

def _get_project(self):
project, version, build, filename = self._resolve_resources()
project, _, _, _ = self._resolve_resources()
return project

def _get_version(self):
project, version, build, filename = self._resolve_resources()
_, version, _, _ = self._resolve_resources()
return version

def get(self, request, format=None):
url = request.GET.get("url")
project_slug = request.GET.get("project-slug")
version_slug = request.GET.get("version-slug")
if not url:
return JsonResponse(
{"error": "'url' GET attribute is required"},
status=400,
)
if not project_slug or not version_slug:
return JsonResponse(
{
"error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'"
},
status=400,
)

addons_version = request.GET.get("api-version")
if not addons_version:
Expand Down Expand Up @@ -148,6 +169,7 @@ def get(self, request, format=None):
version,
build,
filename,
url,
user=request.user,
)
return JsonResponse(data, json_dumps_params={"indent": 4, "sort_keys": True})
Expand Down Expand Up @@ -199,6 +221,7 @@ def get(
version=None,
build=None,
filename=None,
url=None,
user=None,
):
"""
Expand All @@ -208,12 +231,12 @@ def get(
best JSON structure for that particular version.
"""
if addons_version.major == 0:
return self._v0(project, version, build, filename, user)
return self._v0(project, version, build, filename, url, user)

if addons_version.major == 1:
return self._v1(project, version, build, filename, user)
return self._v1(project, version, build, filename, url, user)

def _v0(self, project, version, build, filename, user):
def _v0(self, project, version, build, filename, url, user):
"""
Initial JSON data structure consumed by the JavaScript client.
Expand Down Expand Up @@ -308,27 +331,6 @@ def _v0(self, project, version, build, filename, user):
versions_active_built_not_hidden.values_list("slug", flat=True)
),
},
"doc_diff": {
"enabled": Feature.ADDONS_DOC_DIFF_DISABLED not in project_features,
# "http://test-builds-local.devthedocs.org/en/latest/index.html"
"base_url": resolver.resolve(
project=project,
# NOTE: we are using LATEST version to compare against to for now.
# Ideally, this should be configurable by the user.
version_slug=LATEST,
language=project.language,
filename=filename,
)
if filename
else None,
"root_selector": "[role=main]",
"inject_styles": True,
# NOTE: `base_host` and `base_page` are not required, since
# we are constructing the `base_url` in the backend instead
# of the frontend, as the doc-diff extension does.
"base_host": "",
"base_page": "",
},
"flyout": {
"enabled": Feature.ADDONS_FLYOUT_DISABLED not in project_features,
"translations": [
Expand All @@ -347,22 +349,22 @@ def _v0(self, project, version, build, filename, user):
"versions": [
{
# TODO: name this field "display_name"
"slug": version.slug,
"slug": version_.slug,
"url": resolver.resolve(
project=project,
version_slug=version.slug,
external=version.type == EXTERNAL,
version_slug=version_.slug,
external=version_.type == EXTERNAL,
),
}
for version in versions_active_built_not_hidden
for version_ in versions_active_built_not_hidden
],
"downloads": [
{
# TODO: name this field "display_name"
"name": name,
"url": url,
"url": url_,
}
for name, url in version_downloads
for name, url_ in version_downloads
],
# TODO: find a way to get this data in a reliably way.
# We don't have a simple way to map a URL to a file in the repository.
Expand Down Expand Up @@ -417,6 +419,38 @@ def _v0(self, project, version, build, filename, user):
},
}

# DocDiff depends on `url=` GET attribute.
# This attribute allows us to know the exact filename where the request was made.
# If we don't know the filename, we cannot return the data required by DocDiff to work.
# In that case, we just don't include the `doc_diff` object in the response.
if url:
data["addons"].update(
{
"doc_diff": {
"enabled": Feature.ADDONS_DOC_DIFF_DISABLED
not in project_features,
# "http://test-builds-local.devthedocs.org/en/latest/index.html"
"base_url": resolver.resolve(
project=project,
# NOTE: we are using LATEST version to compare against to for now.
# Ideally, this should be configurable by the user.
version_slug=LATEST,
language=project.language,
filename=filename,
)
if filename
else None,
"root_selector": "[role=main]",
"inject_styles": True,
# NOTE: `base_host` and `base_page` are not required, since
# we are constructing the `base_url` in the backend instead
# of the frontend, as the doc-diff extension does.
"base_host": "",
"base_page": "",
},
}
)

# Update this data with the one generated at build time by the doctool
if version and version.build_data:
data.update(version.build_data)
Expand Down Expand Up @@ -450,7 +484,7 @@ def _v0(self, project, version, build, filename, user):

return data

def _v1(self, project, version, build, filename, user):
def _v1(self, project, version, build, filename, url, user):
return {
"api_version": "1",
"comment": "Undefined yet. Use v0 for now",
Expand Down

0 comments on commit 00d188e

Please sign in to comment.