From 8c793f8add45c71d832bba00e1c59c430cd4063a Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Thu, 24 Jan 2019 17:08:59 -0500 Subject: [PATCH] Add custom serializers for "_artifact" vs "_artifacts" Remove boilerplate from the plugins by defining new, more specialized serializers for the single-artifact-per-content case, the multiple-artifacts-per-content case, and the no-artifacts-per-content case. Required PR: https://github.com/pulp/pulp_file/pull/159 Required PR: https://github.com/PulpQE/pulp-smash/pull/1164 Required PR: https://github.com/pulp/pulpcore-plugin/pull/38 re: #4366 https://pulp.plan.io/issues/4366 --- pulpcore/app/serializers/__init__.py | 7 +++- pulpcore/app/serializers/content.py | 25 +++++++++++++- pulpcore/app/serializers/fields.py | 33 +++++++++++++++++++ pulpcore/app/viewsets/content.py | 8 ++--- .../api/using_plugin/test_orphans.py | 2 +- .../api/using_plugin/test_repo_versions.py | 8 ++--- pulpcore/tests/unit/content/test_handler.py | 4 +-- 7 files changed, 74 insertions(+), 13 deletions(-) diff --git a/pulpcore/app/serializers/__init__.py b/pulpcore/app/serializers/__init__.py index 40bb60c6e8..b81c43930e 100644 --- a/pulpcore/app/serializers/__init__.py +++ b/pulpcore/app/serializers/__init__.py @@ -14,7 +14,12 @@ AsyncOperationResponseSerializer ) from .fields import BaseURLField, ContentRelatedField, LatestVersionField # noqa -from .content import ArtifactSerializer, ContentSerializer # noqa +from .content import ( # noqa + ArtifactSerializer, + NoArtifactContentSerializer, + SingleArtifactContentSerializer, + MultipleArtifactContentSerializer, +) from .progress import ProgressReportSerializer # noqa from .publication import ( # noqa ContentGuardSerializer, diff --git a/pulpcore/app/serializers/content.py b/pulpcore/app/serializers/content.py index 6ef1e9422b..8613f4a1c9 100644 --- a/pulpcore/app/serializers/content.py +++ b/pulpcore/app/serializers/content.py @@ -11,9 +11,32 @@ UNIQUE_ALGORITHMS = ['sha256', 'sha384', 'sha512'] -class ContentSerializer(base.MasterModelSerializer): +class BaseContentSerializer(base.MasterModelSerializer): _href = base.DetailIdentityField() + class Meta: + model = models.Content + fields = base.MasterModelSerializer.Meta.fields + + +class NoArtifactContentSerializer(BaseContentSerializer): + + class Meta: + model = models.Content + fields = base.MasterModelSerializer.Meta.fields + + +class SingleArtifactContentSerializer(BaseContentSerializer): + _artifact = fields.SingleContentArtifactField( + help_text=_("Artifact file representing the physical content"), + ) + + class Meta: + model = models.Content + fields = base.MasterModelSerializer.Meta.fields + ('_artifact',) + + +class MultipleArtifactContentSerializer(BaseContentSerializer): _artifacts = fields.ContentArtifactsField( help_text=_("A dict mapping relative paths inside the Content to the corresponding" "Artifact URLs. E.g.: {'relative/path': " diff --git a/pulpcore/app/serializers/fields.py b/pulpcore/app/serializers/fields.py index c7608b07ea..7072937994 100644 --- a/pulpcore/app/serializers/fields.py +++ b/pulpcore/app/serializers/fields.py @@ -17,6 +17,39 @@ class ContentRelatedField(DetailRelatedField): queryset = models.Content.objects.all() +class SingleContentArtifactField(RelatedField): + """ + A serializer field for the '_artifacts' ManyToManyField on the Content model (single-artifact). + """ + lookup_field = 'pk' + view_name = 'artifacts-detail' + queryset = models.Artifact.objects.all() + allow_null = True + + def get_attribute(self, instance): + """ + Returns the field from the instance that should be serialized using this serializer field. + + This serializer looks up the list of artifacts and returns only one, if any exist. If more + than one exist, it throws and exception because this serializer is being used in an + improper context. + + Args: + instance (:class:`pulpcore.app.models.Content`): An instance of Content being + serialized. + + Returns: + A single Artifact model related to the instance of Content. + """ + try: + return instance._artifacts.get() + except models.Artifact.DoesNotExist: + return None + except models.Artifact.MultipleObjectsReturned: + raise ValueError(_("SingleContentArtifactField should not be used in a context where " + "multiple artifacts for one content is possible.")) + + class ContentArtifactsField(serializers.DictField): """ A serializer field for the '_artifacts' ManyToManyField on the Content model. diff --git a/pulpcore/app/viewsets/content.py b/pulpcore/app/viewsets/content.py index 9f4b7d3f8f..02f417568a 100644 --- a/pulpcore/app/viewsets/content.py +++ b/pulpcore/app/viewsets/content.py @@ -6,7 +6,7 @@ from rest_framework.response import Response from pulpcore.app.models import Artifact, Content, ContentArtifact -from pulpcore.app.serializers import ArtifactSerializer, ContentSerializer +from pulpcore.app.serializers import ArtifactSerializer, MultipleArtifactContentSerializer from pulpcore.app.viewsets.base import BaseFilterSet, NamedModelViewSet from .custom_filters import ( @@ -80,7 +80,6 @@ class ContentFilter(BaseFilterSet): class Meta: model = Content fields = { - '_type': ['exact', 'in'], 'repository_version': ['exact'], 'repository_version_added': ['exact'], 'repository_version_removed': ['exact'], @@ -92,9 +91,10 @@ class ContentViewSet(NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin): endpoint_name = 'content' - queryset = Content.objects.all() - serializer_class = ContentSerializer filterset_class = ContentFilter + # These are just placeholders, the plugin writer would replace them with the actual + queryset = Content.objects.all() + serializer_class = MultipleArtifactContentSerializer @transaction.atomic def create(self, request): diff --git a/pulpcore/tests/functional/api/using_plugin/test_orphans.py b/pulpcore/tests/functional/api/using_plugin/test_orphans.py index 4563f4dda5..d0f19152bd 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_orphans.py +++ b/pulpcore/tests/functional/api/using_plugin/test_orphans.py @@ -79,7 +79,7 @@ def test_clean_orphan_content_unit(self): ) # Verify that the artifact is present on disk. - artifact_path = self.api_client.get(content['artifact'])['file'] + artifact_path = self.api_client.get(content['_artifact'])['file'] cmd = ('ls', artifact_path) self.cli_client.run(cmd, sudo=True) diff --git a/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py b/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py index 9f83eda801..0133f0855f 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py +++ b/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py @@ -324,7 +324,7 @@ def test_delete_first_version(self): get_content(self.repo, self.repo_version_hrefs[0]) for repo_version_href in self.repo_version_hrefs[1:]: artifact_paths = get_artifact_paths(self.repo, repo_version_href) - self.assertIn(self.content[0]['artifact'], artifact_paths) + self.assertIn(self.content[0]['_artifact'], artifact_paths) def test_delete_last_version(self): """Delete the last repository version. @@ -346,8 +346,8 @@ def test_delete_last_version(self): self.repo = self.client.get(self.repo['_href']) artifact_paths = get_artifact_paths(self.repo) - self.assertNotIn(self.content[-2]['artifact'], artifact_paths) - self.assertIn(self.content[-1]['artifact'], artifact_paths) + self.assertNotIn(self.content[-2]['_artifact'], artifact_paths) + self.assertIn(self.content[-1]['_artifact'], artifact_paths) def test_delete_middle_version(self): """Delete a middle version.""" @@ -359,7 +359,7 @@ def test_delete_middle_version(self): for repo_version_href in self.repo_version_hrefs[index + 1:]: artifact_paths = get_artifact_paths(self.repo, repo_version_href) - self.assertIn(self.content[index]['artifact'], artifact_paths) + self.assertIn(self.content[index]['_artifact'], artifact_paths) def test_delete_publication(self): """Delete a publication. diff --git a/pulpcore/tests/unit/content/test_handler.py b/pulpcore/tests/unit/content/test_handler.py index 6c9265dccd..d63efb227a 100644 --- a/pulpcore/tests/unit/content/test_handler.py +++ b/pulpcore/tests/unit/content/test_handler.py @@ -13,9 +13,9 @@ class HandlerSaveContentTestCase(TestCase): def setUp(self): self.f1 = FileContent.objects.create(relative_path='f1', digest='1') - self.f1.artifact = None + ContentArtifact.objects.create(artifact=None, content=self.f1, relative_path='f1') self.f2 = FileContent.objects.create(relative_path='f2', digest='1') - self.f2.artifact = None + ContentArtifact.objects.create(artifact=None, content=self.f2, relative_path='f2') def download_result_mock(self, path): dr = Mock()