API V2: test that command is actually saved (#9827)

* API V2: test that command is actually saved

* Build details page: normalize/trim command paths (second attempt) (#9831)

Continuation of the work done in
#9815 that didn't work as we
expected.

We weren't saving the commands properly when hitting the API since
`serializers.SerializerMethodField` is a read-only field. This commit fixes this
problem by using a different Serializer depending if it's a GET request or not,
and if it's made by an Admin or not.

It overrides the method `BuildViewSet.get_serializer_class` to manage these
different cases and return the proper serializer.

Done on top of #9827 to be sure the
tests for saving commands are passing.

* Feedback from review addressed

- rename serializer to mention "ReadOnly" on it's class name
- add docstrings to these serializers

* Typo when accessing the action

* Optional builder hash on local instance

Old builds don't have the builder hash in the docroot, since that was implmented
recently.

Co-authored-by: Manuel Kaufmann <[email protected]>
Co-authored-by: Benjamin Balder Bach <[email protected]>

readthedocs/api/v2/serializers.py

@@ -1,6 +1,9 @@
 """Defines serializers for each of our models."""
 
+import re
+
 from allauth.socialaccount.models import SocialAccount
+from django.conf import settings
 from rest_framework import serializers
 
 from readthedocs.builds.models import Build, BuildCommandResult, Version
@@ -133,11 +136,63 @@ class Meta:
         exclude = []
 
 
+class BuildCommandReadOnlySerializer(BuildCommandSerializer):
+
+    """
+    Serializer used on GETs to trim the commands' path.
+
+    Remove unreadable paths from the command outputs when returning it from the API.
+    We could make this change at build level, but we want to avoid undoable issues from now
+    and hack a small solution to fix the immediate problem.
+
+    This converts:
+        $ /usr/src/app/checkouts/readthedocs.org/user_builds/
+            <container_hash>/<project_slug>/envs/<version_slug>/bin/python
+        $ /home/docs/checkouts/readthedocs.org/user_builds/
+            <project_slug>/envs/<version_slug>/bin/python
+    into
+        $ python
+    """
+
+    command = serializers.SerializerMethodField()
+
+    def get_command(self, obj):
+        project_slug = obj.build.version.project.slug
+        version_slug = obj.build.version.slug
+        docroot = settings.DOCROOT.rstrip("/")  # remove trailing '/'
+
+        # Remove Docker hash from DOCROOT when running it locally
+        # DOCROOT contains the Docker container hash (e.g. b7703d1b5854).
+        # We have to remove it from the DOCROOT it self since it changes each time
+        # we spin up a new Docker instance locally.
+        container_hash = "/"
+        if settings.RTD_DOCKER_COMPOSE:
+            docroot = re.sub("/[0-9a-z]+/?$", "", settings.DOCROOT, count=1)
+            container_hash = "/([0-9a-z]+/)?"
+
+        regex = f"{docroot}{container_hash}{project_slug}/envs/{version_slug}(/bin/)?"
+        command = re.sub(regex, "", obj.command, count=1)
+        return command
+
+
 class BuildSerializer(serializers.ModelSerializer):
 
-    """Build serializer for user display, doesn't display internal fields."""
+    """
+    Build serializer for user display.
 
-    commands = BuildCommandSerializer(many=True, read_only=True)
+    This is the default serializer for Build objects over read-only operations from regular users.
+    Take into account that:
+
+    - It doesn't display internal fields (builder, _config)
+    - It's read-only for multiple fields (commands, project_slug, etc)
+
+    Staff users should use either:
+
+    - BuildAdminSerializer for write operations (e.g. builders hitting the API),
+    - BuildAdminReadOnlySerializer for read-only actions (e.g. dashboard retrieving build details)
+    """
+
+    commands = BuildCommandReadOnlySerializer(many=True, read_only=True)
     project_slug = serializers.ReadOnlyField(source='project.slug')
     version_slug = serializers.ReadOnlyField(source='get_version_slug')
     docs_url = serializers.SerializerMethodField()
@@ -160,13 +215,32 @@ def get_docs_url(self, obj):
 
 class BuildAdminSerializer(BuildSerializer):
 
-    """Build serializer for display to admin users and build instances."""
+    """
+    Build serializer to update Build objects from build instances.
+
+    It allows write operations on `commands` and display fields (e.g. builder)
+    that are allowed for admin purposes only.
+    """
+
+    commands = BuildCommandSerializer(many=True, read_only=True)
 
     class Meta(BuildSerializer.Meta):
         # `_config` should be excluded to avoid conflicts with `config`
         exclude = ('_config',)
 
 
+class BuildAdminReadOnlySerializer(BuildAdminSerializer):
+
+    """
+    Build serializer to retrieve Build objects from the dashboard.
+
+    It uses `BuildCommandReadOnlySerializer` to automatically parse the command
+    and trim the useless path.
+    """
+
+    commands = BuildCommandReadOnlySerializer(many=True, read_only=True)
+
+
 class SearchIndexSerializer(serializers.Serializer):
     q = serializers.CharField(max_length=500)
     project = serializers.CharField(max_length=500, required=False)

readthedocs/api/v2/views/model_views.py

@@ -22,6 +22,7 @@
 
 from ..permissions import APIPermission, APIRestrictedPermission, IsOwner
 from ..serializers import (
+    BuildAdminReadOnlySerializer,
     BuildAdminSerializer,
     BuildCommandSerializer,
     BuildSerializer,
@@ -224,11 +225,25 @@ class VersionViewSet(DisableListEndpoint, UserSelectViewSet):
 class BuildViewSet(DisableListEndpoint, UserSelectViewSet):
     permission_classes = [APIRestrictedPermission]
     renderer_classes = (JSONRenderer, PlainTextBuildRenderer)
-    serializer_class = BuildSerializer
-    admin_serializer_class = BuildAdminSerializer
     model = Build
     filterset_fields = ('project__slug', 'commit')
 
+    def get_serializer_class(self):
+        """
+        Return the proper serializer for UI and Admin.
+
+        This ViewSet has a sligtly different pattern since we want to
+        pre-process the `command` field before returning it to the user, and we
+        also want to have a specific serializer for admins.
+        """
+        if self.request.user.is_staff:
+            # Logic copied from `UserSelectViewSet.get_serializer_class`
+            # and extended to choose serializer from self.action
+            if self.action not in ["list", "retrieve"]:
+                return BuildAdminSerializer  # Staff write-only
+            return BuildAdminReadOnlySerializer  # Staff read-only
+        return BuildSerializer  # Non-staff
+
     @decorators.action(
         detail=False,
         permission_classes=[permissions.IsAdminUser],

readthedocs/rtd_tests/tests/test_api.py

@@ -3,11 +3,13 @@
 import json
 from unittest import mock
 
+import dateutil
 from allauth.socialaccount.models import SocialAccount
 from django.contrib.auth.models import User
 from django.http import QueryDict
 from django.test import TestCase, override_settings
 from django.urls import reverse
+from django.utils import timezone
 from django_dynamic_fixture import get
 from rest_framework import status
 from rest_framework.test import APIClient
@@ -474,26 +476,36 @@ def test_make_build_commands(self):
         )
         self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
         build = resp.data
-        now = datetime.datetime.utcnow()
+        now = timezone.now()
+        start_time = now - datetime.timedelta(seconds=5)
+        end_time = now
         resp = client.post(
             '/api/v2/command/',
             {
-                'build': build['id'],
-                'command': 'echo test',
-                'description': 'foo',
-                'exit_code': 0,
-                'start_time': str(now - datetime.timedelta(seconds=5)),
-                'end_time': str(now),
+                "build": build["id"],
+                "command": "echo test",
+                "description": "foo",
+                "exit_code": 0,
+                "start_time": start_time,
+                "end_time": end_time,
             },
             format='json',
         )
         self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
         resp = client.get('/api/v2/build/%s/' % build['id'])
         self.assertEqual(resp.status_code, 200)
         build = resp.data
-        self.assertEqual(len(build['commands']), 1)
-        self.assertEqual(build['commands'][0]['run_time'], 5)
-        self.assertEqual(build['commands'][0]['description'], 'foo')
+        self.assertEqual(len(build["commands"]), 1)
+        self.assertEqual(build["commands"][0]["command"], "echo test")
+        self.assertEqual(build["commands"][0]["run_time"], 5)
+        self.assertEqual(build["commands"][0]["description"], "foo")
+        self.assertEqual(build["commands"][0]["exit_code"], 0)
+        self.assertEqual(
+            dateutil.parser.parse(build["commands"][0]["start_time"]), start_time
+        )
+        self.assertEqual(
+            dateutil.parser.parse(build["commands"][0]["end_time"]), end_time
+        )
 
     def test_get_raw_log_success(self):
         project = Project.objects.get(pk=1)