From 7cfdb26e2b6575da9e4b0f29a9923c0351b31474 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 26 Jul 2023 13:40:16 +0200 Subject: [PATCH 1/5] Build: skip duplicated commands Sometimes it happens the web servers are congested and the builder retries the API call to save the command in the database. However, one of the first attempts finally ended up working resulting in the command being saved twice. This small chunk of code checks for the existence of the command before saving it into the database and logs a warning and return 204 if it already exists, instead of re-saving it again. It's not a perfect solution, but it could help under similar circumstances. Related https://github.com/readthedocs/readthedocs.org/issues/10567 --- readthedocs/api/v2/views/model_views.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 529b498e37f..5687fe6758a 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -336,6 +336,11 @@ def perform_create(self, serializer): build_api_key = self.request.build_api_key if not build_api_key.project.builds.filter(pk=build_pk).exists(): raise PermissionDenied() + + if BuildCommandResult.objects.filter(**serializer.validated_data).exists(): + log.warning("Build command is duplicated. Skipping...") + return Response(status=status.HTTP_204_NO_CONTENT) + return super().perform_create(serializer) def get_queryset_for_api_key(self, api_key): From b5bf65ab8481211bff152811920065cff882a9bd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 27 Jul 2023 19:28:03 +0200 Subject: [PATCH 2/5] Test case for duplicated BuildCommandResult --- readthedocs/api/v2/views/model_views.py | 7 +++-- readthedocs/rtd_tests/tests/test_api.py | 42 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 5687fe6758a..6acfcd7050b 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -337,9 +337,12 @@ def perform_create(self, serializer): if not build_api_key.project.builds.filter(pk=build_pk).exists(): raise PermissionDenied() - if BuildCommandResult.objects.filter(**serializer.validated_data).exists(): + if BuildCommandResult.objects.filter( + build=serializer.validated_data["build"], + command=serializer.validated_data["command"], + ).exists(): log.warning("Build command is duplicated. Skipping...") - return Response(status=status.HTTP_204_NO_CONTENT) + return return super().perform_create(serializer) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index f0da2e55767..f14e2c9ad00 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -909,6 +909,48 @@ def test_build_read_and_write_endpoints_for_build_api_token(self): resp = client.patch(f"/api/v2/build/{build.pk}/") self.assertEqual(resp.status_code, 404) + def test_build_commands_duplicated_command(self): + """Sending the same request twice should only create one BuildCommandResult.""" + project = get( + Project, + language="en", + ) + version = project.versions.first() + build = Build.objects.create(project=project, version=version) + + self.assertEqual(BuildCommandResult.objects.count(), 0) + + client = APIClient() + _, build_api_key = BuildAPIKey.objects.create_key(project) + client.credentials(HTTP_AUTHORIZATION=f"Token {build_api_key}") + + now = timezone.now() + start_time = now - datetime.timedelta(seconds=5) + end_time = now + + data = { + "build": build.pk, + "command": "git status", + "description": "Git status", + "exit_code": 0, + "start_time": start_time, + "end_time": end_time, + } + + response = client.post( + "/api/v2/command/", + data, + format="json", + ) + self.assertEqual(response.status_code, 201) + response = client.post( + "/api/v2/command/", + data, + format="json", + ) + self.assertEqual(response.status_code, 201) + self.assertEqual(BuildCommandResult.objects.count(), 1) + def test_build_commands_read_only_endpoints_for_normal_user(self): user_normal = get(User, is_staff=False) user_admin = get(User, is_staff=True) From 3457a5a8323fb8c916eb0f4856329375877b9e58 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 31 Jul 2023 12:28:51 +0200 Subject: [PATCH 3/5] Add `start_date` to the filter --- readthedocs/api/v2/views/model_views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 6acfcd7050b..8b1830811b4 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -339,6 +339,7 @@ def perform_create(self, serializer): if BuildCommandResult.objects.filter( build=serializer.validated_data["build"], + start_date=serializer.validated_data["start_date"], command=serializer.validated_data["command"], ).exists(): log.warning("Build command is duplicated. Skipping...") From 3965d147533884b90c44e3d7465a3ad79f08afaa Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 31 Jul 2023 13:39:15 +0200 Subject: [PATCH 4/5] Typo --- 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 8b1830811b4..310b2de8e38 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -339,7 +339,7 @@ def perform_create(self, serializer): if BuildCommandResult.objects.filter( build=serializer.validated_data["build"], - start_date=serializer.validated_data["start_date"], + start_time=serializer.validated_data["start_time"], command=serializer.validated_data["command"], ).exists(): log.warning("Build command is duplicated. Skipping...") From 82adc09b027e1473c20643bb8d23aa6ca875f742 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 1 Aug 2023 10:38:54 +0200 Subject: [PATCH 5/5] Update readthedocs/api/v2/views/model_views.py Co-authored-by: Santos Gallegos --- readthedocs/api/v2/views/model_views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 310b2de8e38..c274b801df4 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -340,7 +340,6 @@ def perform_create(self, serializer): if BuildCommandResult.objects.filter( build=serializer.validated_data["build"], start_time=serializer.validated_data["start_time"], - command=serializer.validated_data["command"], ).exists(): log.warning("Build command is duplicated. Skipping...") return