From a40a329430f2764df5eb4676865ce45d3d94ac6c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 21 Dec 2022 11:55:37 +0100 Subject: [PATCH 1/6] Build details page: normalize/trim command paths (second attempt) Continuation of the work done in https://github.com/readthedocs/readthedocs.org/pull/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 https://github.com/readthedocs/readthedocs.org/pull/9827 to be sure the tests for saving commands are passing. --- readthedocs/api/v2/serializers.py | 49 ++++++++++++++++++++++++- readthedocs/api/v2/views/model_views.py | 19 +++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index dd1a864028d..dc2f7c04bc6 100644 --- a/readthedocs/api/v2/serializers.py +++ b/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,49 @@ class Meta: exclude = [] +class BuildCommandUISerializer(BuildCommandSerializer): + """ + Serializer used on GETs to trimm 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/ + //envs//bin/python + $ /home/docs/checkouts/readthedocs.org/user_builds/ + /envs//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.""" - commands = BuildCommandSerializer(many=True, read_only=True) + commands = BuildCommandUISerializer(many=True, read_only=True) project_slug = serializers.ReadOnlyField(source='project.slug') version_slug = serializers.ReadOnlyField(source='get_version_slug') docs_url = serializers.SerializerMethodField() @@ -162,11 +203,17 @@ class BuildAdminSerializer(BuildSerializer): """Build serializer for display to admin users and build instances.""" + commands = BuildCommandSerializer(many=True, read_only=True) + class Meta(BuildSerializer.Meta): # `_config` should be excluded to avoid conflicts with `config` exclude = ('_config',) +class BuildAdminUISerializer(BuildAdminSerializer): + commands = BuildCommandUISerializer(many=True, read_only=True) + + class SearchIndexSerializer(serializers.Serializer): q = serializers.CharField(max_length=500) project = serializers.CharField(max_length=500, required=False) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index b8b3eb21cfa..b954f6b014f 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -23,6 +23,7 @@ from ..permissions import APIPermission, APIRestrictedPermission, IsOwner from ..serializers import ( BuildAdminSerializer, + BuildAdminUISerializer, BuildCommandSerializer, BuildSerializer, DomainSerializer, @@ -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 check for GET method + if self.request.method == "GET": + return BuildAdminUISerializer + return BuildAdminSerializer + return BuildSerializer + @decorators.action( detail=False, permission_classes=[permissions.IsAdminUser], From 06122e3fd138512327d307c88a1b71d6d03f8087 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 22 Dec 2022 12:36:09 +0100 Subject: [PATCH 2/6] Feedback from review addressed - rename serializer to mention "ReadOnly" on it's class name - add docstrings to these serializers --- readthedocs/api/v2/serializers.py | 35 ++++++++++++++++++++----- readthedocs/api/v2/views/model_views.py | 10 +++---- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index dc2f7c04bc6..de7faa4f22a 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -136,7 +136,7 @@ class Meta: exclude = [] -class BuildCommandUISerializer(BuildCommandSerializer): +class BuildCommandReadOnlySerializer(BuildCommandSerializer): """ Serializer used on GETs to trimm the commands' path. @@ -176,9 +176,20 @@ def get_command(self, obj): class BuildSerializer(serializers.ModelSerializer): - """Build serializer for user display, doesn't display internal fields.""" + """ + Build serializer for user display. + + 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) - commands = BuildCommandUISerializer(many=True, read_only=True) + Staff users should use either BuildAdminSerializer for write operations (e.g. builders hitting the API), + or 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() @@ -201,7 +212,12 @@ 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) @@ -210,8 +226,15 @@ class Meta(BuildSerializer.Meta): exclude = ('_config',) -class BuildAdminUISerializer(BuildAdminSerializer): - commands = BuildCommandUISerializer(many=True, read_only=True) +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): diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index b954f6b014f..7957052eed0 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -22,8 +22,8 @@ from ..permissions import APIPermission, APIRestrictedPermission, IsOwner from ..serializers import ( + BuildAdminReadOnlySerializer, BuildAdminSerializer, - BuildAdminUISerializer, BuildCommandSerializer, BuildSerializer, DomainSerializer, @@ -239,10 +239,10 @@ def get_serializer_class(self): if self.request.user.is_staff: # Logic copied from `UserSelectViewSet.get_serializer_class` # and extended to check for GET method - if self.request.method == "GET": - return BuildAdminUISerializer - return BuildAdminSerializer - return BuildSerializer + if self.request.action in ["list", "retrieve"]: + return BuildAdminReadOnlySerializer # Staff read-onlyl + return BuildAdminSerializer # Staff write-only + return BuildSerializer # Non-staff @decorators.action( detail=False, From f97d349d42f6fd622267734c2c7a4270fa5af737 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 22 Dec 2022 12:44:43 +0100 Subject: [PATCH 3/6] Typo when accessing the action --- readthedocs/api/v2/views/model_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 7957052eed0..41a6a23ffbc 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -239,7 +239,7 @@ def get_serializer_class(self): if self.request.user.is_staff: # Logic copied from `UserSelectViewSet.get_serializer_class` # and extended to check for GET method - if self.request.action in ["list", "retrieve"]: + if self.action in ["list", "retrieve"]: return BuildAdminReadOnlySerializer # Staff read-onlyl return BuildAdminSerializer # Staff write-only return BuildSerializer # Non-staff From 1948d665faa6263f4d3b98e61bb93e3bde1ba2e9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 22 Dec 2022 12:45:00 +0100 Subject: [PATCH 4/6] Optional builder hash on local instance Old builds don't have the builder hash in the docroot, since that was implmented recently. --- readthedocs/api/v2/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index de7faa4f22a..dfeb084d315 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -167,7 +167,7 @@ def get_command(self, obj): container_hash = "/" if settings.RTD_DOCKER_COMPOSE: docroot = re.sub("/[0-9a-z]+/?$", "", settings.DOCROOT, count=1) - container_hash = "/[0-9a-z]+/" + container_hash = "/([0-9a-z]+/)?" regex = f"{docroot}{container_hash}{project_slug}/envs/{version_slug}(/bin/)?" command = re.sub(regex, "", obj.command, count=1) From 53a876111139aabfe3752f1c961ac4eae439f699 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 22 Dec 2022 13:10:30 +0100 Subject: [PATCH 5/6] Lint --- readthedocs/api/v2/serializers.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index dfeb084d315..70bdf02358d 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -137,6 +137,7 @@ class Meta: class BuildCommandReadOnlySerializer(BuildCommandSerializer): + """ Serializer used on GETs to trimm the commands' path. @@ -185,8 +186,10 @@ class BuildSerializer(serializers.ModelSerializer): - 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), - or BuildAdminReadOnlySerializer for read-only actions (e.g. dashboard retrieving build details) + 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) @@ -227,6 +230,7 @@ class Meta(BuildSerializer.Meta): class BuildAdminReadOnlySerializer(BuildAdminSerializer): + """ Build serializer to retrieve Build objects from the dashboard. From b231cc0a1e59a31dbe0db296383de047d3df6a8d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 22 Dec 2022 14:12:18 +0100 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Benjamin Balder Bach --- readthedocs/api/v2/serializers.py | 2 +- readthedocs/api/v2/views/model_views.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 70bdf02358d..37c58e06c6d 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -139,7 +139,7 @@ class Meta: class BuildCommandReadOnlySerializer(BuildCommandSerializer): """ - Serializer used on GETs to trimm the commands' path. + 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 diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 41a6a23ffbc..1faf7dc2066 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -238,10 +238,10 @@ def get_serializer_class(self): """ if self.request.user.is_staff: # Logic copied from `UserSelectViewSet.get_serializer_class` - # and extended to check for GET method - if self.action in ["list", "retrieve"]: - return BuildAdminReadOnlySerializer # Staff read-onlyl - return BuildAdminSerializer # Staff write-only + # 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(