Checking for dupe content in finalize #599

daviddavis · 2020-03-19T16:21:33Z

fixes #6362
https://pulp.plan.io/issues/6362

I decided not to update remove_duplicates to handle checking for duplicate content because (a) it would violate the single responsibility principle as remove_duplicates removes existing content from the repo version if it collides with content being added.

Also (b) remove_duplicates is called at the beginning of finalize before processing happens while validation ought to happen at the end.

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599

bmbouter · 2020-03-19T16:40:41Z

pulpcore/plugin/repo_version_utils.py

@@ -82,3 +120,16 @@ def validate_version_paths(version):
        validate_file_paths(paths)
    except ValueError as e:
        raise ValueError(_("Cannot create repository version. {err}.").format(err=e))
+
+
+def validate_repo_version(version):


is the idea this would be configured to be called by plugins? or is this called for all plugins today?

Not sure I completely understand your question. This is just a helper method plugins can call during finalize if they want to. Here's an example from pulp_file: pulp/pulp_file#368.

It's sort of like a wrapper for any other validate_* methods in repo_version_utils.

Yup that answers it. You're saying plugin opt into this and this is a helpful wrapper. Sounds good to me.

bmbouter · 2020-03-19T16:49:25Z

pulpcore/app/models/repository.py

@@ -553,33 +553,6 @@ def add_content(self, content):
        if self.complete:
            raise ResourceImmutableError(self)

-        error_messages = []


To confirm my understanding of this code being removed. It wouldn't automatically remove the duplicate, it would just fail. Is that right?

Exactly. This is the crux of the problem. When duplicate content gets added to a repo version, it fails before giving the plugin writer the chance to deal with the problem during finalize.

pulpcore/plugin/repo_version_utils.py

bmbouter · 2020-03-19T16:57:25Z

Does this need any docs changes? I suspect there needs to be something.

fao89

I really liked this PR!
Thank you @daviddavis

fao89 · 2020-03-19T17:16:41Z

CHANGES/plugin_api/6362.feature

@@ -0,0 +1 @@
+Added two new repo validation methods (validate_repo_version and validate_duplicate_content).


Great idea!

fao89 · 2020-03-19T17:17:21Z

pulpcore/plugin/repo_version_utils.py

@@ -67,6 +67,44 @@ def remove_duplicates(repository_version):
                repository_version.remove_content(duplicates_qs)


+def validate_duplicate_content(version):


much better! it makes easy for plugin writers to customize

goosemania · 2020-03-19T18:21:07Z

@daviddavis , thanks! Looks good to me.

gmbnomis

While I like this approach a lot (in fact pulp_cookbook implements a method similar to validate_duplicate_content()), I don't fully agree to your statement:

it would violate the single responsibility principle as remove_duplicates removes existing content from the repo version if it collides with content being added.

My expectation for a function called remove_duplicates() is that it does exactly that. If remove_duplicates() can't remove all duplicates, I expect it to fail. Thus, the validation in the following code would never raise:

remove_duplicates(new_version)
validate_duplicate_content(version)

But, in fact, it will raise a ValueError if there are
a) duplicates in the added content
b) duplicates in the base/previous repo version's content (should never happen if proper validation is done)

This may be confusing to plugin writers.

Moreover, I think that the current docstring of remove_duplicates() does not emphasize that it only "removes existing content from the repo version if it collides with content being added" (as you point out correctly) and that there may still be duplicates after calling the function.

So, could we:

make the docstring very clear about this behavior?
and, maybe even: rename remove_duplicates to e.g. supersede_existing_duplicates ? (as said before, I am not good at naming things...)

daviddavis · 2020-03-20T11:47:41Z

@gmbnomis My comment about remove_duplicates only removing existing content is about the existing nature of the function and not what it ought to be doing based on its name. I agree that it's not clear and could be confusing to plugin writers. I went ahead and updated the docstring for remove_duplicates. I fully agree with that move.

As to renaming remove_duplicates, I'm a bit torn because it would be a lot of work but I do agree that its name is not entirely accurate. I could go either way. Any thoughts @goosemania @bmbouter @fabricio-aguiar ?

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulp_file#368

daviddavis · 2020-03-30T19:50:42Z

@gmbnomis I talked on IRC with @bmbouter and @fabricio-aguiar. We'd like to merge this and open a separate issue to consider renaming remove_duplicates since we agreed that its name is not accurate. Wdyt?

pulpbot · 2020-03-30T19:57:12Z

Attached issue: https://pulp.plan.io/issues/6362

gmbnomis · 2020-03-30T20:44:26Z

@gmbnomis I talked on IRC with @bmbouter and @fabricio-aguiar. We'd like to merge this and open a separate issue to consider renaming remove_duplicates since we agreed that its name is not accurate. Wdyt?

@daviddavis yes, please go ahead. You have addressed the most important bit (updating the documentation). Of course, the name is still misleading, but if you are hesitant to change it because it creates issues with plugins/work for plugins, we can keep it.

daviddavis · 2020-03-30T20:55:12Z

@gmbnomis cool, sounds good. I opened https://pulp.plan.io/issues/6416.

bmbouter

This looks good to me as long as we are tracking the name fix in https://pulp.plan.io/issues/6416

Thanks @daviddavis, @gmbnomis and @fabricio-aguiar !

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599 (cherry picked from commit dc6acb9)

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request Mar 19, 2020

Moving duplicate content check to finalize

8431446

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599

daviddavis mentioned this pull request Mar 19, 2020

Moving duplicate content check to finalize pulp/pulp_file#368

Merged

daviddavis force-pushed the issue6362 branch 3 times, most recently from 7d12f35 to a1508f2 Compare March 19, 2020 16:36

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request Mar 19, 2020

Moving duplicate content check to finalize

3d7bcca

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599

bmbouter reviewed Mar 19, 2020

View reviewed changes

pulpcore/plugin/repo_version_utils.py Show resolved Hide resolved

fao89 approved these changes Mar 19, 2020

View reviewed changes

daviddavis force-pushed the issue6362 branch from a1508f2 to d4605b4 Compare March 19, 2020 17:33

gmbnomis reviewed Mar 19, 2020

View reviewed changes

daviddavis force-pushed the issue6362 branch from d4605b4 to db31633 Compare March 20, 2020 11:50

Checking for dupe content in finalize

db31633

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulp_file#368

pulpbot added the Needs Cherry Pick label Mar 30, 2020

daviddavis removed the Needs Cherry Pick label Mar 30, 2020

bmbouter approved these changes Mar 30, 2020

View reviewed changes

daviddavis merged commit 6d1fcbd into pulp:master Mar 30, 2020

daviddavis pushed a commit to pulp/pulp_file that referenced this pull request Mar 30, 2020

Moving duplicate content check to finalize

dc6acb9

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599

pulpbot pushed a commit to pulp/pulp_file that referenced this pull request Mar 31, 2020

Moving duplicate content check to finalize

7c032a1

fixes #6362 https://pulp.plan.io/issues/6362 Required PR: pulp/pulpcore#599 (cherry picked from commit dc6acb9)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checking for dupe content in finalize #599

Checking for dupe content in finalize #599

daviddavis commented Mar 19, 2020 •

edited

Loading

bmbouter Mar 19, 2020

daviddavis Mar 19, 2020

daviddavis Mar 19, 2020

bmbouter Mar 19, 2020

bmbouter Mar 19, 2020

daviddavis Mar 19, 2020

bmbouter commented Mar 19, 2020

fao89 left a comment

fao89 Mar 19, 2020

fao89 Mar 19, 2020

goosemania commented Mar 19, 2020

gmbnomis left a comment

daviddavis commented Mar 20, 2020

daviddavis commented Mar 30, 2020

pulpbot commented Mar 30, 2020

gmbnomis commented Mar 30, 2020

daviddavis commented Mar 30, 2020

bmbouter left a comment

		@@ -0,0 +1 @@
		Added two new repo validation methods (validate_repo_version and validate_duplicate_content).

		@@ -67,6 +67,44 @@ def remove_duplicates(repository_version):
		repository_version.remove_content(duplicates_qs)


		def validate_duplicate_content(version):

Checking for dupe content in finalize #599

Checking for dupe content in finalize #599

Conversation

daviddavis commented Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Mar 19, 2020

fao89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania commented Mar 19, 2020

gmbnomis left a comment

Choose a reason for hiding this comment

daviddavis commented Mar 20, 2020

daviddavis commented Mar 30, 2020

pulpbot commented Mar 30, 2020

gmbnomis commented Mar 30, 2020

daviddavis commented Mar 30, 2020

bmbouter left a comment

Choose a reason for hiding this comment

daviddavis commented Mar 19, 2020 •

edited

Loading