Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List added/removed content according to a given RepositoryVersion #460

Merged
merged 1 commit into from
Jan 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/plugin_api/5706.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added an optional parameter base_version to RepositoryVersion add() and removed() methods.
48 changes: 37 additions & 11 deletions pulpcore/app/models/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def new_version(self, base_version=None):
# now add any content that's in the base_version but not in version
version.add_content(base_version.content.exclude(pk__in=version.content))

if Task.current:
if Task.current():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now I know why I had to work around this in my unit tests. Thank you!

resource = CreatedResource(content_object=version)
resource.save()
return version
Expand Down Expand Up @@ -396,6 +396,17 @@ class Meta:
get_latest_by = 'number'
ordering = ('number',)

def _relationships(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only hangup I have with this PR. _relationships sounds generic. Would there be a better name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about _find_content? @gmbnomis and @daviddavis what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's not really Content though. It's RepositoryContent. What about _repository_content?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's not really Content though. It's RepositoryContent. What about _repository_content?

This sounds good to me.

"""
Returns a set of repository_content for a repository version

Returns:
django.db.models.QuerySet: The repository_content that is contained within this version.
"""
return RepositoryContent.objects.filter(
repository=self.repository, version_added__number__lte=self.number
).exclude(version_removed__number__lte=self.number)

@property
def content(self):
"""
Expand All @@ -415,12 +426,7 @@ def content(self):
>>> ...
>>>
"""
relationships = RepositoryContent.objects.filter(
repository=self.repository, version_added__number__lte=self.number
).exclude(
version_removed__number__lte=self.number
)
return Content.objects.filter(version_memberships__in=relationships)
return Content.objects.filter(version_memberships__in=self._relationships())

def _content_old(self):
"""
Expand Down Expand Up @@ -449,19 +455,39 @@ def artifacts(self):
"""
Artifact.objects.filter(content__pk__in=self.content)

def added(self):
def added(self, base_version=None):
Copy link
Contributor

@gmbnomis gmbnomis Dec 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks complicated and may take a long time to run if there are many versions between the base_version and the current version. I was thinking about:


    def _relationships(self):
        return RepositoryContent.objects.filter(
            repository=self.repository, version_added__number__lte=self.number
        ).exclude(version_removed__number__lte=self.number)

    @property
    def content(self):
        return Content.objects.filter(version_memberships__in=self._relationships())


    def added(self, base_version=None):
        if not base_version:
            return Content.objects.filter(version_memberships__version_added=self)

        return Content.objects.filter(
            version_memberships__in=self._relationships()
        ).exclude(
            version_memberships__in=base_version._relationships()
        )

    def removed(self, base_version=None):
        if not base_version:
            return Content.objects.filter(version_memberships__version_removed=self)

        return Content.objects.filter(
            version_memberships__in=base_version._relationships()
        ).exclude(
            version_memberships__in=self._relationships()
        )

I hadn't the time to look into the cost of the queries in your implementation and in the one from above.

The queries above may be costly (we need a join with the repository version table to get the version numbers to do the range query on version numbers). However, I don't know how this relates to your queries and we do .content queries all the time anyway...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely your queries are better, thank you!

"""
Args:
base_version (pulpcore.app.models.RepositoryVersion): an optional repository version

Returns:
QuerySet: The Content objects that were added by this version.
"""
return Content.objects.filter(version_memberships__version_added=self)
if not base_version:
return Content.objects.filter(version_memberships__version_added=self)

return Content.objects.filter(
version_memberships__in=self._relationships()
).exclude(
version_memberships__in=base_version._relationships()
)

def removed(self):
def removed(self, base_version=None):
"""
Args:
base_version (pulpcore.app.models.RepositoryVersion): an optional repository version

Returns:
QuerySet: The Content objects that were removed by this version.
"""
return Content.objects.filter(version_memberships__version_removed=self)
if not base_version:
return Content.objects.filter(version_memberships__version_removed=self)

return Content.objects.filter(
version_memberships__in=base_version._relationships()
).exclude(
version_memberships__in=self._relationships()
)

def contains(self, content):
"""
Expand Down
65 changes: 65 additions & 0 deletions pulpcore/tests/unit/models/test_repository.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
from itertools import compress

from django.test import TestCase
from pulpcore.plugin.models import Content, Repository, RepositoryVersion


class RepositoryVersionTestCase(TestCase):

def setUp(self):
self.repository = Repository.objects.create()
self.repository.CONTENT_TYPES = [Content]
self.repository.save()

contents = []
for _ in range(0, 4):
contents.append(Content(pulp_type="core.content"))

Content.objects.bulk_create(contents)
self.pks = [c.pk for c in contents]

def test_add_and_remove_content(self):
contents = Content.objects.filter(pk__in=self.pks)
with self.repository.new_version() as version1:
version1.add_content(contents) # v1 == all contents

to_remove = contents[0:2]
with self.repository.new_version() as version2:
version2.remove_content(to_remove) # v2 == removed 2 first contents

to_add = Content.objects.filter(pk=contents[0].pk)
with self.repository.new_version() as version3:
version3.add_content(to_add) # v3 == added first content

version0 = RepositoryVersion.objects.filter(number=0, repository=self.repository).first()

self.assertEqual(version0.added().count(), 0)
self.assertEqual(version1.added().count(), 4)
self.assertEqual(version2.added().count(), 0)
self.assertEqual(version3.added().count(), 1)

self.assertEqual(version0.removed().count(), 0)
self.assertEqual(version1.removed().count(), 0)
self.assertEqual(version2.removed().count(), 2)
self.assertEqual(version3.removed().count(), 0)

self.assertEqual(version3.added(version0).count(), 3)
self.assertEqual(version3.removed(version0).count(), 0)

added_pks_0 = version3.added(version0).values_list('pk', flat=True)
removed_pks_0 = version3.removed(version0).values_list('pk', flat=True)

self.assertCountEqual(added_pks_0, compress(self.pks, [1, 0, 1, 1]), added_pks_0)
self.assertCountEqual(removed_pks_0, compress(self.pks, [0, 0, 0, 0]), removed_pks_0)

added_pks_1 = version3.added(version1).values_list('pk', flat=True)
removed_pks_1 = version3.removed(version1).values_list('pk', flat=True)

self.assertCountEqual(added_pks_1, compress(self.pks, [0, 0, 0, 0]), added_pks_1)
self.assertCountEqual(removed_pks_1, compress(self.pks, [0, 1, 0, 0]), removed_pks_1)

added_pks_2 = version3.added(version2).values_list('pk', flat=True)
removed_pks_2 = version3.removed(version2).values_list('pk', flat=True)

self.assertCountEqual(added_pks_2, compress(self.pks, [1, 0, 0, 0]), added_pks_2)
self.assertCountEqual(removed_pks_2, compress(self.pks, [0, 0, 0, 0]), removed_pks_2)