From 5190c625d63a8b2fafa75a8e98d2030a2bc1da91 Mon Sep 17 00:00:00 2001 From: Brian Bouterse Date: Thu, 9 May 2019 12:54:02 -0400 Subject: [PATCH] Split Distribution into two objects The Distribution contained options which should not be mixed. Specifically the repository and repository_version options go together and the publication goes by itself. This PR splits that object into two new objects RepositoryVersionDistribution and PublicationDistribution. The two models are not detail objects, and require the plugin writer to declare a Distribution detail object. It was already this way before this PR. This also adds a release note about the breaking changes that come with the switch to Master/Detail. Required PR: https://github.com/pulp/pulpcore-plugin/pull/97 Required PR: https://github.com/pulp/pulp_file/pull/219 https://pulp.plan.io/issues/4785 closes #4785 --- docs/release-notes/pulpcore/3.0.x.rst | 5 + pulpcore/app/migrations/0001_initial.py | 19 +-- pulpcore/app/models/__init__.py | 5 +- pulpcore/app/models/publication.py | 25 +++- pulpcore/app/serializers/__init__.py | 3 +- pulpcore/app/serializers/publication.py | 129 +++++++++--------- pulpcore/app/viewsets/__init__.py | 2 +- pulpcore/app/viewsets/publication.py | 35 ++--- pulpcore/content/handler.py | 39 +++--- .../api/using_plugin/test_crd_publications.py | 6 +- 10 files changed, 129 insertions(+), 139 deletions(-) diff --git a/docs/release-notes/pulpcore/3.0.x.rst b/docs/release-notes/pulpcore/3.0.x.rst index a91915919db..3f58eec4404 100644 --- a/docs/release-notes/pulpcore/3.0.x.rst +++ b/docs/release-notes/pulpcore/3.0.x.rst @@ -20,6 +20,11 @@ example from `pulp_file `_ see the URL change `here `_ as an example. See plugin docs compatible with 3.0.0rc2 for more details. +Distributions are now Master/Detail which causes the Distribution URL endpoint to change. To give an +example from `pulp_file `_ see the URL changes made +`in this PR `_ as an example. See plugin docs +compatible with 3.0.0rc2 for more details. + The semantics of :term:`Remote` attributes ``ssl_ca_certificate``, ``ssl_client_certificate``, and ``ssl_client_key`` changed even though the field names didn't. Now these assets are saved directly in the database instead of on the filesystem, and they are prevented from being read back out to diff --git a/pulpcore/app/migrations/0001_initial.py b/pulpcore/app/migrations/0001_initial.py index 6b48a73dc3b..8671b51fd54 100644 --- a/pulpcore/app/migrations/0001_initial.py +++ b/pulpcore/app/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 2.2.1 on 2019-05-08 18:30 +# Generated by Django 2.2.1 on 2019-05-09 16:49 from django.conf import settings import django.core.validators @@ -278,7 +278,7 @@ class Migration(migrations.Migration): ('version_removed', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='removed_memberships', to='core.RepositoryVersion')), ], options={ - 'unique_together': {('repository', 'content', 'version_added'), ('repository', 'content', 'version_removed')}, + 'unique_together': {('repository', 'content', 'version_removed'), ('repository', 'content', 'version_added')}, }, ), migrations.AddField( @@ -410,7 +410,7 @@ class Migration(migrations.Migration): ], options={ 'default_related_name': 'published_metadata', - 'unique_together': {('publication', 'relative_path'), ('publication', 'file')}, + 'unique_together': {('publication', 'file'), ('publication', 'relative_path')}, }, ), migrations.CreateModel( @@ -428,17 +428,4 @@ class Migration(migrations.Migration): 'unique_together': {('publication', 'content_artifact'), ('publication', 'relative_path')}, }, ), - migrations.CreateModel( - name='Distribution', - fields=[ - ('basedistribution_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, related_name='_distributions', serialize=False, to='core.BaseDistribution')), - ('publication', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='_distributions', to='core.Publication')), - ('repository', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='_distributions', to='core.Repository')), - ('repository_version', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='_distributions', to='core.RepositoryVersion')), - ], - options={ - 'default_related_name': '_distributions', - }, - bases=('core.basedistribution',), - ), ] diff --git a/pulpcore/app/models/__init__.py b/pulpcore/app/models/__init__.py index e43e3c966be..dd5cc24a9bf 100644 --- a/pulpcore/app/models/__init__.py +++ b/pulpcore/app/models/__init__.py @@ -6,10 +6,11 @@ from .publication import ( # noqa ContentGuard, BaseDistribution, - Distribution, Publication, + PublicationDistribution, PublishedArtifact, - PublishedMetadata + PublishedMetadata, + RepositoryVersionDistribution, ) from .repository import ( # noqa Exporter, diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py index b098eda2935..6f0e0f3d70a 100644 --- a/pulpcore/app/models/publication.py +++ b/pulpcore/app/models/publication.py @@ -232,23 +232,34 @@ class BaseDistribution(MasterModel): remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL) -class Distribution(BaseDistribution): +class PublicationDistribution(BaseDistribution): """ - A distribution defines how a publication is distributed by Pulp's content app. - - The use of repository, repository_version, or publication are mutually exclusive, and this - model's serializer will raise a ValidationError if they are used together. + Define how Pulp's content app will serve a :term:`Publication`. Relations: publication (models.ForeignKey): Publication to be served. + """ + + publication = models.ForeignKey(Publication, null=True, on_delete=models.SET_NULL) + + class Meta: + abstract = True + + +class RepositoryVersionDistribution(BaseDistribution): + """ + Define how Pulp's content app will serve a :term:`RepositoryVersion` or :term:`Repository`. + + The ``repository`` and ``repository_version`` fields cannot be used together. + + Relations: repository (models.ForeignKey): The latest RepositoryVersion for this Repository will be served. repository_version (models.ForeignKey): RepositoryVersion to be served. """ - publication = models.ForeignKey(Publication, null=True, on_delete=models.SET_NULL) repository = models.ForeignKey(Repository, null=True, on_delete=models.SET_NULL) repository_version = models.ForeignKey(RepositoryVersion, null=True, on_delete=models.SET_NULL) class Meta: - default_related_name = '_distributions' + abstract = True diff --git a/pulpcore/app/serializers/__init__.py b/pulpcore/app/serializers/__init__.py index 9c2ee51ee43..c3c51d95ea4 100644 --- a/pulpcore/app/serializers/__init__.py +++ b/pulpcore/app/serializers/__init__.py @@ -35,8 +35,9 @@ from .publication import ( # noqa BaseDistributionSerializer, ContentGuardSerializer, - DistributionSerializer, + PublicationDistributionSerializer, PublicationSerializer, + RepositoryVersionDistributionSerializer, ) from .repository import ( # noqa ExporterSerializer, diff --git a/pulpcore/app/serializers/publication.py b/pulpcore/app/serializers/publication.py index d67f3a6db93..5cc7a22a009 100644 --- a/pulpcore/app/serializers/publication.py +++ b/pulpcore/app/serializers/publication.py @@ -21,12 +21,6 @@ class PublicationSerializer(MasterModelSerializer): _href = DetailIdentityField() - _distributions = DetailRelatedField( - help_text=_('This publication is currently being served as' - 'defined by these distributions.'), - many=True, - read_only=True, - ) repository_version = NestedRelatedField( view_name='versions-detail', lookup_field='number', @@ -72,7 +66,6 @@ class Meta: model = models.Publication fields = MasterModelSerializer.Meta.fields + ( 'publisher', - '_distributions', 'repository_version', 'repository' ) @@ -100,6 +93,21 @@ class Meta: class BaseDistributionSerializer(MasterModelSerializer): _href = DetailIdentityField() + base_path = serializers.CharField( + help_text=_('The base (relative) path component of the published url. Avoid paths that \ + overlap with other distribution base paths (e.g. "foo" and "foo/bar")'), + validators=[validators.MaxLengthValidator( + models.BaseDistribution._meta.get_field('base_path').max_length, + message=_('`base_path` length must be less than {} characters').format( + models.BaseDistribution._meta.get_field('base_path').max_length + )), + UniqueValidator(queryset=models.BaseDistribution.objects.all()), + ] + ) + base_url = BaseURLField( + source='base_path', read_only=True, + help_text=_('The URL for accessing the publication as defined by this distribution.') + ) content_guard = DetailRelatedField( required=False, help_text=_('An optional content-guard.'), @@ -107,13 +115,13 @@ class BaseDistributionSerializer(MasterModelSerializer): allow_null=True ) name = serializers.CharField( - help_text=_('A unique distribution name. Ex, `rawhide` and `stable`.'), + help_text=_('A unique name. Ex, `rawhide` and `stable`.'), validators=[validators.MaxLengthValidator( - models.Distribution._meta.get_field('name').max_length, - message=_('Distribution name length must be less than {} characters').format( - models.Distribution._meta.get_field('name').max_length + models.BaseDistribution._meta.get_field('name').max_length, + message=_('`name` length must be less than {} characters').format( + models.BaseDistribution._meta.get_field('name').max_length )), - UniqueValidator(queryset=models.Distribution.objects.all())] + UniqueValidator(queryset=models.BaseDistribution.objects.all())] ) remote = DetailRelatedField( required=False, @@ -123,35 +131,58 @@ class BaseDistributionSerializer(MasterModelSerializer): ) class Meta: + abstract = True fields = ModelSerializer.Meta.fields + ( + 'base_path', + 'base_url', 'content_guard', 'name', 'remote', ) + def _validate_path_overlap(self, path): + # look for any base paths nested in path + search = path.split("/")[0] + q = Q(base_path=search) + for subdir in path.split("/")[1:]: + search = "/".join((search, subdir)) + q |= Q(base_path=search) -class DistributionSerializer(BaseDistributionSerializer): - base_path = serializers.CharField( - help_text=_('The base (relative) path component of the published url. Avoid paths that \ - overlap with other distribution base paths (e.g. "foo" and "foo/bar")'), - validators=[validators.MaxLengthValidator( - models.Distribution._meta.get_field('base_path').max_length, - message=_('Distribution base_path length must be less than {} characters').format( - models.Distribution._meta.get_field('base_path').max_length - )), - UniqueValidator(queryset=models.Distribution.objects.all()), - ] - ) - base_url = BaseURLField( - source='base_path', read_only=True, - help_text=_('The URL for accessing the publication as defined by this distribution.') - ) + # look for any base paths that nest path + q |= Q(base_path__startswith='{}/'.format(path)) + qs = models.BaseDistribution.objects.filter(q) + + if self.instance is not None: + qs = qs.exclude(pk=self.instance.pk) + + match = qs.first() + if match: + raise serializers.ValidationError(detail=_("Overlaps with existing distribution '" + "{}'").format(match.name)) + + return path + + def validate_base_path(self, path): + self._validate_relative_path(path) + return self._validate_path_overlap(path) + + +class PublicationDistributionSerializer(BaseDistributionSerializer): publication = DetailRelatedField( required=False, help_text=_('Publication to be served'), queryset=models.Publication.objects.exclude(complete=False), allow_null=True ) + + class Meta: + abstract = True + fields = BaseDistributionSerializer.Meta.fields + ( + 'publication', + ) + + +class RepositoryVersionDistributionSerializer(BaseDistributionSerializer): repository = RelatedField( required=False, help_text=_('The latest RepositoryVersion for this Repository will be served.'), @@ -170,52 +201,18 @@ class DistributionSerializer(BaseDistributionSerializer): ) class Meta: - model = models.Distribution + abstract = True fields = BaseDistributionSerializer.Meta.fields + ( - 'base_path', - 'base_url', - 'publication', 'repository', 'repository_version', ) - def _validate_path_overlap(self, path): - # look for any base paths nested in path - search = path.split("/")[0] - q = Q(base_path=search) - for subdir in path.split("/")[1:]: - search = "/".join((search, subdir)) - q |= Q(base_path=search) - - # look for any base paths that nest path - q |= Q(base_path__startswith='{}/'.format(path)) - qs = models.Distribution.objects.filter(q) - - if self.instance is not None: - qs = qs.exclude(pk=self.instance.pk) - - match = qs.first() - if match: - raise serializers.ValidationError(detail=_("Overlaps with existing distribution '" - "{}'").format(match.name)) - - return path - - def validate_base_path(self, path): - self._validate_relative_path(path) - return self._validate_path_overlap(path) - def validate(self, data): super().validate(data) - mutex_keys = ['publication', 'repository', 'repository_version'] - in_use_keys = [] - for mkey in mutex_keys: - if mkey in data: - in_use_keys.append(mkey) - - if len(in_use_keys) > 1: - msg = _("The attributes {keys} must be used exclusively.".format(keys=in_use_keys)) + if 'repository' in data and 'repository_version' in data: + msg = _("The attributes 'repository' and 'repository_version' must be used" + "exclusively.") raise serializers.ValidationError(msg) return data diff --git a/pulpcore/app/viewsets/__init__.py b/pulpcore/app/viewsets/__init__.py index 4970a0bb751..5b04621f827 100644 --- a/pulpcore/app/viewsets/__init__.py +++ b/pulpcore/app/viewsets/__init__.py @@ -16,9 +16,9 @@ RepoVersionHrefFilter ) from .publication import ( # noqa + BaseDistributionViewSet, ContentGuardFilter, ContentGuardViewSet, - DistributionViewSet, PublicationViewSet, ) from .repository import ( # noqa diff --git a/pulpcore/app/viewsets/publication.py b/pulpcore/app/viewsets/publication.py index 4ebda018ae5..5072504da46 100644 --- a/pulpcore/app/viewsets/publication.py +++ b/pulpcore/app/viewsets/publication.py @@ -3,13 +3,13 @@ from rest_framework.filters import OrderingFilter from pulpcore.app.models import ( + BaseDistribution, ContentGuard, - Distribution, Publication, ) from pulpcore.app.serializers import ( + BaseDistributionSerializer, ContentGuardSerializer, - DistributionSerializer, PublicationSerializer, ) from pulpcore.app.viewsets import ( @@ -65,42 +65,29 @@ class DistributionFilter(BaseFilterSet): base_path = filters.CharFilter() class Meta: - model = Distribution + model = BaseDistribution fields = { 'name': NAME_FILTER_OPTIONS, 'base_path': ['exact', 'contains', 'icontains', 'in'] } -class DistributionViewSet(NamedModelViewSet, - mixins.RetrieveModelMixin, - mixins.ListModelMixin, - AsyncCreateMixin, - AsyncRemoveMixin, - AsyncUpdateMixin): +class BaseDistributionViewSet(NamedModelViewSet, + mixins.RetrieveModelMixin, + mixins.ListModelMixin, + AsyncCreateMixin, + AsyncRemoveMixin, + AsyncUpdateMixin): """ Provides read and list methods and also provides asynchronous CUD methods to dispatch tasks with reservation that lock all Distributions preventing race conditions during base_path checking. """ endpoint_name = 'distributions' - queryset = Distribution.objects.all() - serializer_class = DistributionSerializer + queryset = BaseDistribution.objects.all() + serializer_class = BaseDistributionSerializer filterset_class = DistributionFilter def async_reserved_resources(self, instance): """Return resource that locks all Distributions.""" return "/api/v3/distributions/" - - @classmethod - def is_master_viewset(cls): - """ - Declare DistributionViewSet to be a master ViewSet. - - This Viewset is a master ViewSet despite that its associated Model isn't - a master model. This prevents registering the ViewSet with a router. - """ - if cls is DistributionViewSet: - return True - else: - return super().is_master_viewset() diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py index 47670cc4ecf..ca287b30b5a 100644 --- a/pulpcore/content/handler.py +++ b/pulpcore/content/handler.py @@ -11,7 +11,7 @@ from django.conf import settings from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist from django.db import IntegrityError, transaction -from pulpcore.app.models import Artifact, ContentArtifact, Distribution, Remote, RemoteArtifact +from pulpcore.app.models import Artifact, ContentArtifact, BaseDistribution, Remote, RemoteArtifact log = logging.getLogger(__name__) @@ -94,16 +94,16 @@ def _match_distribution(path): path (str): The path component of the URL. Returns: - Distribution: The matched distribution. + BaseDistribution: The matched distribution. Raises: PathNotResolved: when not matched. """ base_paths = Handler._base_paths(path) try: - return Distribution.objects.get(base_path__in=base_paths) + return BaseDistribution.objects.get(base_path__in=base_paths) except ObjectDoesNotExist: - log.debug(_('Distribution not matched for {path} using: {base_paths}').format( + log.debug(_('BaseDistribution not matched for {path} using: {base_paths}').format( path=path, base_paths=base_paths )) raise PathNotResolved(path) @@ -117,7 +117,8 @@ def _permit(request, distribution): Args: request (:class:`aiohttp.web.Request`): A request for a published file. - distribution (:class:`pulpcore.plugin.models.Distribution`): The matched distribution. + distribution (:class:`pulpcore.plugin.models.BaseDistribution`): The matched + distribution. Raises: :class:`aiohttp.web_exceptions.HTTPForbidden`: When not permitted. @@ -157,21 +158,19 @@ async def _match_and_stream(self, path, request): :class:`aiohttp.web.StreamResponse` or :class:`aiohttp.web.FileResponse`: The response streamed back to the client. """ - distro = Handler._match_distribution(path) + distro = Handler._match_distribution(path).cast() self._permit(request, distro) - if not distro.publication and not distro.repository \ - and not distro.repository_version and not distro.remote: - raise PathNotResolved(path) - rel_path = path.lstrip('/') rel_path = rel_path[len(distro.base_path):] rel_path = rel_path.lstrip('/') - if distro.publication: + publication = getattr(distro, 'publication', None) + + if publication: # published artifact try: - pa = distro.publication.published_artifact.get(relative_path=rel_path) + pa = publication.published_artifact.get(relative_path=rel_path) ca = pa.content_artifact except ObjectDoesNotExist: pass @@ -183,17 +182,17 @@ async def _match_and_stream(self, path, request): # published metadata try: - pm = distro.publication.published_metadata.get(relative_path=rel_path) + pm = publication.published_metadata.get(relative_path=rel_path) except ObjectDoesNotExist: pass else: return self._handle_file_response(pm.file) # pass-through - if distro.publication.pass_through: + if publication.pass_through: try: ca = ContentArtifact.objects.get( - content__in=distro.publication.repository_version.content, + content__in=publication.repository_version.content, relative_path=rel_path) except MultipleObjectsReturned: log.error( @@ -211,11 +210,13 @@ async def _match_and_stream(self, path, request): return self._handle_file_response(ca.artifact.file) else: return await self._stream_content_artifact(request, StreamResponse(), ca) - elif distro.repository or distro.repository_version: - if distro.repository: + + repo_version = getattr(distro, 'repository_version', None) + repository = getattr(distro, 'repository', None) + + if repository or repo_version: + if repository: repo_version = distro.repository.versions.get(number=distro.repository.last_version) - else: - repo_version = distro.repository_version try: ca = ContentArtifact.objects.get( diff --git a/pulpcore/tests/functional/api/using_plugin/test_crd_publications.py b/pulpcore/tests/functional/api/using_plugin/test_crd_publications.py index 44d0907600e..7b632b89630 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_crd_publications.py +++ b/pulpcore/tests/functional/api/using_plugin/test_crd_publications.py @@ -80,7 +80,7 @@ def test_02_read_publication_with_specific_fields(self): Permutate field list to ensure different combinations on result. """ - fields = ('_href', '_created', '_distributions', 'publisher') + fields = ('_href', '_created', '_file_distributions', 'publisher') for field_pair in permutations(fields, 2): # ex: field_pair = ('_href', '_created) with self.subTest(field_pair=field_pair): @@ -96,9 +96,9 @@ def test_02_read_publication_with_specific_fields(self): def test_02_read_publication_without_specific_fields(self): """Read a publication by its href excluding specific fields.""" # requests doesn't allow the use of != in parameters. - url = '{}?fields!=_distributions'.format(self.publication['_href']) + url = '{}?fields!=_file_distributions'.format(self.publication['_href']) publication = self.client.get(url) - self.assertNotIn('_distributions', publication.keys()) + self.assertNotIn('_file_distributions', publication.keys()) @skip_if(bool, 'publication', False) def test_02_read_publications(self):