diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index 45adf473225..5cdca1d3fe0 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -1,6 +1,7 @@ """Utility functions that are used by both views and celery tasks.""" import itertools +import json import re import structlog @@ -18,6 +19,7 @@ from readthedocs.builds.models import RegexAutomationRule from readthedocs.builds.models import Version from readthedocs.core.utils.db import delete_in_batches +from readthedocs.storage import build_commands_storage log = structlog.get_logger(__name__) @@ -285,6 +287,44 @@ def normalize_build_command(command, project_slug, version_slug): return command +def get_build_commands_from_storage(build): + """ + Return build commands from storage for ``cold_storage`` builds. + + Returns ``None`` when commands can't be loaded from storage and callers + should keep using serializer output. + """ + if not settings.RTD_SAVE_BUILD_COMMANDS_TO_STORAGE: + return None + + if not build.cold_storage: + return None + + storage_path = "{date}/{id}.json".format( + date=str(build.date.date()), + id=build.id, + ) + if not build_commands_storage.exists(storage_path): + return None + + try: + json_resp = build_commands_storage.open(storage_path).read() + commands = json.loads(json_resp) + for buildcommand in commands: + buildcommand["command"] = normalize_build_command( + buildcommand["command"], + build.project.slug, + build.get_version_slug(), + ) + return commands + except Exception: + log.exception( + "Failed to read build data from storage.", + path=storage_path, + ) + return None + + class RemoteOrganizationPagination(PageNumberPagination): page_size = 25 diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index b0ee7dff1cf..9a26d52b530 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -1,6 +1,5 @@ """Endpoints for listing Projects, Versions, Builds, etc.""" -import json from dataclasses import asdict import structlog @@ -28,7 +27,7 @@ from readthedocs.api.v2.permissions import HasBuildAPIKey from readthedocs.api.v2.permissions import IsOwner from readthedocs.api.v2.permissions import ReadOnlyPermission -from readthedocs.api.v2.utils import normalize_build_command +from readthedocs.api.v2.utils import get_build_commands_from_storage from readthedocs.aws.security_token_service import AWSTemporaryCredentialsError from readthedocs.aws.security_token_service import get_s3_build_media_scoped_credentials from readthedocs.aws.security_token_service import get_s3_build_tools_scoped_credentials @@ -43,7 +42,6 @@ from readthedocs.oauth.services import registry from readthedocs.projects.models import Domain from readthedocs.projects.models import Project -from readthedocs.storage import build_commands_storage from ..serializers import BuildAdminReadOnlySerializer from ..serializers import BuildAdminSerializer @@ -336,35 +334,13 @@ def retrieve(self, *args, **kwargs): This uses files from storage to get the JSON, and replaces the ``commands`` part of the response data. """ - if not settings.RTD_SAVE_BUILD_COMMANDS_TO_STORAGE: - return super().retrieve(*args, **kwargs) - instance = self.get_object() serializer = self.get_serializer(instance) data = serializer.data - if instance.cold_storage: - storage_path = "{date}/{id}.json".format( - date=str(instance.date.date()), - id=instance.id, - ) - if build_commands_storage.exists(storage_path): - try: - json_resp = build_commands_storage.open(storage_path).read() - data["commands"] = json.loads(json_resp) - - # Normalize commands in the same way than when returning - # them using the serializer - for buildcommand in data["commands"]: - buildcommand["command"] = normalize_build_command( - buildcommand["command"], - instance.project.slug, - instance.get_version_slug(), - ) - except Exception: - log.exception( - "Failed to read build data from storage.", - path=storage_path, - ) + commands = get_build_commands_from_storage(instance) + if commands is not None: + data["commands"] = commands + return Response(data) @decorators.action( diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 1bc9fce73fb..024e410f26a 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -12,9 +12,11 @@ from taggit.serializers import TaggitSerializer from taggit.serializers import TagListSerializerField +from readthedocs.api.v2.utils import normalize_build_command from readthedocs.builds.constants import LATEST from readthedocs.builds.constants import STABLE from readthedocs.builds.models import Build +from readthedocs.builds.models import BuildCommandResult from readthedocs.builds.models import Version from readthedocs.core.permissions import AdminPermission from readthedocs.core.resolver import Resolver @@ -143,6 +145,32 @@ def get_version(self, obj): return None +class BuildCommandSerializer(serializers.ModelSerializer): + run_time = serializers.ReadOnlyField() + command = serializers.SerializerMethodField() + + class Meta: + model = BuildCommandResult + fields = [ + "id", + "build", + "command", + "description", + "output", + "exit_code", + "start_time", + "end_time", + "run_time", + ] + + def get_command(self, obj): + return normalize_build_command( + obj.command, + obj.build.project.slug, + obj.build.get_version_slug(), + ) + + class BuildConfigSerializer(FlexFieldsSerializerMixin, serializers.Serializer): """ Render ``Build.config`` property without modifying it. @@ -177,6 +205,10 @@ class BuildSerializer(FlexFieldsModelSerializer): state = BuildStateSerializer(source="*") _links = BuildLinksSerializer(source="*") urls = BuildURLsSerializer(source="*") + builder = serializers.CharField(read_only=True) + docs_url = serializers.SerializerMethodField() + commit_url = serializers.SerializerMethodField() + commands = BuildCommandSerializer(many=True, read_only=True) class Meta: model = Build @@ -191,6 +223,10 @@ class Meta: "success", "error", "commit", + "builder", + "docs_url", + "commit_url", + "commands", "_links", "urls", ] @@ -212,6 +248,18 @@ def get_success(self, obj): return None + def get_docs_url(self, obj): + if obj.version: + return obj.version.get_absolute_url() + + return None + + def get_commit_url(self, obj): + if obj.commit: + return obj.get_commit_url() + + return None + class NotificationMessageSerializer(serializers.Serializer): id = serializers.SlugField() diff --git a/readthedocs/api/v3/tests/mixins.py b/readthedocs/api/v3/tests/mixins.py index 0b0d2293c92..6f866128198 100644 --- a/readthedocs/api/v3/tests/mixins.py +++ b/readthedocs/api/v3/tests/mixins.py @@ -12,8 +12,10 @@ from rest_framework.authtoken.models import Token from rest_framework.test import APIClient -from readthedocs.builds.constants import LATEST, TAG -from readthedocs.builds.models import Build, Version +from readthedocs.builds.constants import LATEST +from readthedocs.builds.constants import TAG +from readthedocs.builds.models import Build +from readthedocs.builds.models import Version from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING from readthedocs.doc_builder.exceptions import BuildCancelled from readthedocs.notifications.models import Notification @@ -104,13 +106,14 @@ def setUp(self): state="finished", error="", success=True, - _config={"property": "test value"}, version=self.version, project=self.project, builder="builder01", commit="a1b2c3", length=60, ) + self.build.config = {"property": "test value"} + self.build.save() self.other = fixture.get(User, projects=[]) self.others_token = fixture.get(Token, key="other", user=self.other) @@ -134,13 +137,14 @@ def setUp(self): state="finished", error="", success=True, - _config={"property": "test value"}, version=self.others_version, project=self.others_project, builder="builder01", commit="a1b2c3", length=60, ) + self.others_build.config = {"property": "test value"} + self.others_build.save() # Make all non-html true so responses are complete self.project.versions.update( @@ -245,7 +249,11 @@ def _get_response_dict(self, view_name, filepath=None): filename = Path(filepath).absolute().parent / "responses" / f"{view_name}.json" return json.load(open(filename)) - def assertDictEqual(self, d1, d2): + # Keep unittest.TestCase signature compatibility. + # assertEqual() dispatches dict comparisons to assertDictEqual and passes + # `msg` as a keyword argument: + # https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertEqual + def assertDictEqual(self, d1, d2, msg=None): """ Show the differences between the dicts in a human readable way. @@ -258,4 +266,10 @@ def assertDictEqual(self, d1, d2): message = datadiff.diff(d1, d2) except ImportError: pass + + if msg and message: + message = f"{message}\n{msg}" + elif msg: + message = msg + return super().assertDictEqual(d1, d2, message) diff --git a/readthedocs/api/v3/tests/responses/projects-builds-detail.json b/readthedocs/api/v3/tests/responses/projects-builds-detail.json index 996bcef6e61..83d59ee73cf 100644 --- a/readthedocs/api/v3/tests/responses/projects-builds-detail.json +++ b/readthedocs/api/v3/tests/responses/projects-builds-detail.json @@ -5,6 +5,10 @@ "error": "", "finished": "2019-04-29T10:01:00Z", "id": 1, + "builder": "builder01", + "docs_url": "http://project.readthedocs.io/en/v1.0/", + "commit_url": "https://github.com/rtfd/project/commit/a1b2c3", + "commands": [], "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/", "project": "https://readthedocs.org/api/v3/projects/project/", diff --git a/readthedocs/api/v3/tests/responses/projects-builds-list.json b/readthedocs/api/v3/tests/responses/projects-builds-list.json index df60e9912fe..7652e06a4d2 100644 --- a/readthedocs/api/v3/tests/responses/projects-builds-list.json +++ b/readthedocs/api/v3/tests/responses/projects-builds-list.json @@ -10,6 +10,10 @@ "error": "", "finished": "2019-04-29T10:01:00Z", "id": 1, + "builder": "builder01", + "docs_url": "http://project.readthedocs.io/en/v1.0/", + "commit_url": "https://github.com/rtfd/project/commit/a1b2c3", + "commands": [], "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/1/", "project": "https://readthedocs.org/api/v3/projects/project/", diff --git a/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json b/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json index fbda2e3a255..65c09f6dd61 100644 --- a/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json +++ b/readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json @@ -6,6 +6,10 @@ "error": "", "finished": null, "id": 3, + "builder": null, + "docs_url": "http://project.readthedocs.io/en/v1.0/", + "commit_url": null, + "commands": [], "_links": { "_self": "https://readthedocs.org/api/v3/projects/project/builds/3/", "notifications": "https://readthedocs.org/api/v3/projects/project/builds/3/notifications/", diff --git a/readthedocs/api/v3/tests/test_builds.py b/readthedocs/api/v3/tests/test_builds.py index 37d451133fb..b21c42f7d5d 100644 --- a/readthedocs/api/v3/tests/test_builds.py +++ b/readthedocs/api/v3/tests/test_builds.py @@ -4,7 +4,8 @@ from django.urls import reverse from readthedocs.builds.constants import EXTERNAL -from readthedocs.projects.constants import PRIVATE, PUBLIC +from readthedocs.projects.constants import PRIVATE +from readthedocs.projects.constants import PUBLIC from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS from readthedocs.subscriptions.products import RTDProductFeature @@ -14,9 +15,7 @@ @override_settings( RTD_ALLOW_ORGANIZATIONS=False, ALLOW_PRIVATE_REPOS=False, - RTD_DEFAULT_FEATURES=dict( - [RTDProductFeature(TYPE_CONCURRENT_BUILDS, value=4).to_item()] - ), + RTD_DEFAULT_FEATURES=dict([RTDProductFeature(TYPE_CONCURRENT_BUILDS, value=4).to_item()]), ) @mock.patch("readthedocs.projects.tasks.builds.update_docs_task", mock.MagicMock()) class BuildsEndpointTests(APIEndpointMixin): @@ -239,6 +238,136 @@ def test_projects_builds_detail(self): self.assertEqual(response.status_code, 200) self.assertDictEqual(response.json(), expected_response) + def test_projects_builds_detail_expand_config(self): + url = reverse( + "projects-builds-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "build_pk": self.build.pk, + }, + ) + response = self.client.get(f"{url}?expand=config") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json()["config"], {"property": "test value"}) + + def test_projects_builds_list_expand_config(self): + url = reverse( + "projects-builds-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ) + response = self.client.get(f"{url}?expand=config") + self.assertEqual(response.status_code, 200) + self.assertEqual( + response.json()["results"][0]["config"], + {"property": "test value"}, + ) + + @override_settings(RTD_SAVE_BUILD_COMMANDS_TO_STORAGE=True) + @mock.patch("readthedocs.api.v3.views.get_build_commands_from_storage") + def test_projects_builds_list_does_not_read_commands_from_cold_storage( + self, + get_build_commands_from_storage, + ): + self.build.cold_storage = True + self.build.save() + get_build_commands_from_storage.return_value = [ + { + "id": 10, + "build": 1, + "command": "storage command", + "description": "Build docs", + "output": "Storage output", + "exit_code": 0, + "start_time": None, + "end_time": None, + "run_time": 0, + }, + ] + url = reverse( + "projects-builds-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + commands = response.json()["results"][0]["commands"] + self.assertEqual(commands, []) + get_build_commands_from_storage.assert_not_called() + + def test_projects_builds_detail_includes_build_metadata_and_commands(self): + self.build.commands.create( + command="python -m sphinx", + description="Build docs", + output="Done", + exit_code=0, + ) + url = reverse( + "projects-builds-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "build_pk": self.build.pk, + }, + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["docs_url"], "http://project.readthedocs.io/en/v1.0/") + self.assertEqual( + data["commit_url"], + "https://github.com/rtfd/project/commit/a1b2c3", + ) + self.assertEqual(data["builder"], "builder01") + self.assertEqual(len(data["commands"]), 1) + self.assertEqual(data["commands"][0]["build"], self.build.pk) + self.assertEqual(data["commands"][0]["command"], "python -m sphinx") + + @override_settings(RTD_SAVE_BUILD_COMMANDS_TO_STORAGE=True) + @mock.patch("readthedocs.api.v3.views.get_build_commands_from_storage") + def test_projects_builds_detail_reads_commands_from_cold_storage( + self, + get_build_commands_from_storage, + ): + self.build.commands.create( + command="db command", + description="Build docs", + output="DB output", + exit_code=0, + ) + self.build.cold_storage = True + self.build.save() + + get_build_commands_from_storage.return_value = [ + { + "id": 10, + "build": 1, + "command": "storage command", + "description": "Build docs", + "output": "Storage output", + "exit_code": 0, + "start_time": None, + "end_time": None, + "run_time": 0, + }, + ] + + url = reverse( + "projects-builds-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "build_pk": self.build.pk, + }, + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + commands = response.json()["commands"] + self.assertEqual(len(commands), 1) + self.assertEqual(commands[0]["command"], "storage command") + self.assertEqual(commands[0]["output"], "Storage output") + get_build_commands_from_storage.assert_called_once_with(self.build) + def test_projects_builds_detail_other_user(self): url = reverse( "projects-builds-detail", @@ -334,10 +463,14 @@ def test_external_version_projects_versions_builds_list_post(self): response_json["build"]["created"] = "2019-04-29T14:00:00Z" expected = self._get_response_dict("projects-versions-builds-list_POST") expected["build"]["commit"] = "d4e5f6" + expected["build"]["docs_url"] = ( + "http://project--v1.0.external-builds.readthedocs.io/en/v1.0/" + ) + expected["build"]["commit_url"] = "https://github.com/rtfd/project/pull/v1.0/commits/d4e5f6" expected["version"]["type"] = "external" - expected["version"]["urls"][ - "documentation" - ] = "http://project--v1.0.external-builds.readthedocs.io/en/v1.0/" + expected["version"]["urls"]["documentation"] = ( + "http://project--v1.0.external-builds.readthedocs.io/en/v1.0/" + ) expected["version"]["urls"]["vcs"] = "https://github.com/rtfd/project/pull/v1.0" self.assertDictEqual(response_json, expected) @@ -349,9 +482,7 @@ def test_projects_builds_notifications_list_anonymous_user(self): "parent_lookup_build__id": self.build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-list" - ) + expected_response = self._get_response_dict("projects-builds-notifications-list") self.client.logout() @@ -392,9 +523,7 @@ def test_projects_builds_notifications_list(self): "parent_lookup_build__id": self.build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-list" - ) + expected_response = self._get_response_dict("projects-builds-notifications-list") self.client.logout() @@ -440,9 +569,7 @@ def test_projects_builds_notifications_list_other_user(self): "parent_lookup_build__id": self.build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-list" - ) + expected_response = self._get_response_dict("projects-builds-notifications-list") self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.others_token.key}") @@ -526,9 +653,7 @@ def test_projects_builds_notifitications_detail_anonymous_user(self): "notification_pk": self.notification_build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-detail" - ) + expected_response = self._get_response_dict("projects-builds-notifications-detail") self.client.logout() # Project and version are public. @@ -569,9 +694,7 @@ def test_projects_builds_notifitications_detail(self): "notification_pk": self.notification_build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-detail" - ) + expected_response = self._get_response_dict("projects-builds-notifications-detail") self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") @@ -617,9 +740,7 @@ def test_projects_builds_notifitications_detail_other_user(self): "notification_pk": self.notification_build.pk, }, ) - expected_response = self._get_response_dict( - "projects-builds-notifications-detail" - ) + expected_response = self._get_response_dict("projects-builds-notifications-detail") self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.others_token.key}") diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index 2c73717f088..1ff1ce30883 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -28,6 +28,7 @@ from rest_framework_extensions.mixins import NestedViewSetMixin from readthedocs.api.v2.permissions import ReadOnlyPermission +from readthedocs.api.v2.utils import get_build_commands_from_storage from readthedocs.builds.constants import EXTERNAL from readthedocs.builds.models import Build from readthedocs.builds.models import Version @@ -400,6 +401,35 @@ class BuildsViewSet( permit_list_expands = [ "config", ] + permit_detail_expands = [ + "config", + ] + + def get_queryset(self): + return ( + super() + .get_queryset() + .select_related( + "project", + "version", + ) + .prefetch_related( + "commands", + ) + ) + + def retrieve(self, request, *args, **kwargs): + # Keep API behavior parity with v2: hydrate commands from cold storage + # on detail responses only (list returns serializer output as-is). + instance = self.get_object() + serializer = self.get_serializer(instance) + data = serializer.data + + commands = get_build_commands_from_storage(instance) + if commands is not None: + data["commands"] = commands + + return Response(data) class BuildsCreateViewSet(BuildsViewSet, CreateModelMixin): diff --git a/readthedocs/proxito/views/hosting.py b/readthedocs/proxito/views/hosting.py index 563894afb27..a1711d331c6 100644 --- a/readthedocs/proxito/views/hosting.py +++ b/readthedocs/proxito/views/hosting.py @@ -278,6 +278,11 @@ class VersionAddonsSerializer(RemoveFieldsMixin, VersionSerializer): class BuildAddonsSerializer(RemoveFieldsMixin, BuildSerializer): FIELDS_TO_REMOVE = [ "_links", + # Keep proxito payload small and avoid expensive lookups. + "builder", + "docs_url", + "commit_url", + "commands", ] diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 83eff726317..e840a7fd784 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -775,6 +775,75 @@ def test_build_without_version(self): assert r.data["version"] is None assert r.data["commands"][0]["command"] == command + @mock.patch("readthedocs.api.v2.views.model_views.get_build_commands_from_storage") + def test_build_detail_reads_commands_from_storage(self, get_build_commands_from_storage): + build = get( + Build, + project=self.project, + version=self.version, + state=BUILD_STATE_FINISHED, + exit_code=0, + ) + get_build_commands_from_storage.return_value = [ + { + "id": 10, + "build": build.pk, + "command": "storage command", + "description": "Build docs", + "output": "Storage output", + "exit_code": 0, + "start_time": None, + "end_time": None, + "run_time": 0, + }, + ] + client = APIClient() + client.force_authenticate(user=self.user) + r = client.get(reverse("build-detail", args=(build.pk,))) + assert r.status_code == 200 + assert r.data["commands"][0]["command"] == "storage command" + assert r.data["commands"][0]["output"] == "Storage output" + get_build_commands_from_storage.assert_called_once_with(build) + + @override_settings(RTD_SAVE_BUILD_COMMANDS_TO_STORAGE=True) + @mock.patch("readthedocs.api.v2.views.model_views.get_build_commands_from_storage") + def test_build_list_does_not_read_commands_from_cold_storage( + self, + get_build_commands_from_storage, + ): + build = get( + Build, + project=self.project, + version=self.version, + state=BUILD_STATE_FINISHED, + exit_code=0, + cold_storage=True, + ) + get_build_commands_from_storage.return_value = [ + { + "id": 10, + "build": build.pk, + "command": "storage command", + "description": "Build docs", + "output": "Storage output", + "exit_code": 0, + "start_time": None, + "end_time": None, + "run_time": 0, + }, + ] + client = APIClient() + client.force_authenticate(user=self.user) + r = client.get( + reverse("build-list"), + {"project__slug": self.project.slug}, + ) + assert r.status_code == 200 + assert len(r.data["results"]) == 1 + assert r.data["results"][0]["id"] == build.pk + assert r.data["results"][0]["commands"] == [] + get_build_commands_from_storage.assert_not_called() + class APITests(TestCase): fixtures = ["eric.json", "test_data.json"]