-
Notifications
You must be signed in to change notification settings - Fork 143
Add an overwrite boolean when modifying repo content #7551
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
Open
daviddavis
wants to merge
1
commit into
pulp:main
Choose a base branch
from
daviddavis:safe-update
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Added an `overwrite` boolean parameter to the repository content modify and content upload | ||
| endpoints. When set to `false`, the operation will return a 409 Conflict error if the content | ||
| being added would overwrite existing content based on `repo_key_fields`. Defaults to `true`. |
224 changes: 224 additions & 0 deletions
224
pulp_file/tests/functional/api/test_overwrite_content.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,224 @@ | ||
| """Tests for the repository content overwrite protection feature.""" | ||
|
|
||
| import uuid | ||
|
|
||
| import pytest | ||
|
|
||
| from pulpcore.client.pulp_file.exceptions import ApiException | ||
| from pulpcore.tests.functional.utils import PulpTaskError | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_modify_overwrite_false_rejects_overwrite( | ||
| file_bindings, | ||
| file_repo, | ||
| file_content_unit_with_name_factory, | ||
| monitor_task, | ||
| ): | ||
| """Adding content with a conflicting relative_path and overwrite=False returns a 409.""" | ||
| shared_path = str(uuid.uuid4()) | ||
|
|
||
| # Create two content units that share the same relative_path but have different artifacts | ||
| content_a = file_content_unit_with_name_factory(shared_path) | ||
| content_b = file_content_unit_with_name_factory(shared_path) | ||
| assert content_a.pulp_href != content_b.pulp_href | ||
|
|
||
| # Add the first content unit (overwrite is irrelevant for an empty repo) | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_a.pulp_href]}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # Attempt to add the second content unit with overwrite=False — expect HTTP 409 | ||
| with pytest.raises(ApiException) as exc_info: | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_b.pulp_href], "overwrite": False}, | ||
| ) | ||
| assert exc_info.value.status == 409 | ||
| assert "PLP0023" in exc_info.value.body | ||
|
|
||
| # Repo version should not have advanced | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_modify_overwrite_false_allows_non_conflicting( | ||
| file_bindings, | ||
| file_repo, | ||
| file_content_unit_with_name_factory, | ||
| monitor_task, | ||
| ): | ||
| """Adding content without a conflicting relative_path and overwrite=False succeeds.""" | ||
| content_a = file_content_unit_with_name_factory(str(uuid.uuid4())) | ||
| content_b = file_content_unit_with_name_factory(str(uuid.uuid4())) | ||
|
|
||
| # Add the first content unit | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_a.pulp_href], "overwrite": False}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # Add non-conflicting content with overwrite=False — should succeed | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_b.pulp_href], "overwrite": False}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/2/") | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_modify_default_allows_overwrite( | ||
| file_bindings, | ||
| file_repo, | ||
| file_content_unit_with_name_factory, | ||
| monitor_task, | ||
| ): | ||
| """Default modify (overwrite=True) still allows overwriting content.""" | ||
| shared_path = str(uuid.uuid4()) | ||
| content_a = file_content_unit_with_name_factory(shared_path) | ||
| content_b = file_content_unit_with_name_factory(shared_path) | ||
|
|
||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_a.pulp_href]}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # With default overwrite=True, adding conflicting content succeeds | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_b.pulp_href]}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/2/") | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_modify_overwrite_false_allows_replace_when_removing_conflict( | ||
| file_bindings, | ||
| file_repo, | ||
| file_content_unit_with_name_factory, | ||
| monitor_task, | ||
| ): | ||
| """Replacing content with overwrite=False succeeds when the conflicting unit is removed.""" | ||
| shared_path = str(uuid.uuid4()) | ||
| content_a = file_content_unit_with_name_factory(shared_path) | ||
| content_b = file_content_unit_with_name_factory(shared_path) | ||
|
|
||
| # Seed the repo with content_a | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| {"add_content_units": [content_a.pulp_href]}, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # Remove content_a and add content_b in the same call with overwrite=False — should succeed | ||
| monitor_task( | ||
| file_bindings.RepositoriesFileApi.modify( | ||
| file_repo.pulp_href, | ||
| { | ||
| "remove_content_units": [content_a.pulp_href], | ||
| "add_content_units": [content_b.pulp_href], | ||
| "overwrite": False, | ||
| }, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/2/") | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_content_upload_overwrite_false_rejects_overwrite( | ||
| file_bindings, | ||
| file_repo, | ||
| random_artifact_factory, | ||
| monitor_task, | ||
| ): | ||
| """Uploading content with overwrite=False and a conflicting relative_path is rejected.""" | ||
| shared_path = str(uuid.uuid4()) | ||
|
|
||
| # Upload first content unit into the repo | ||
| artifact_a = random_artifact_factory() | ||
| monitor_task( | ||
| file_bindings.ContentFilesApi.create( | ||
| relative_path=shared_path, | ||
| artifact=artifact_a.pulp_href, | ||
| repository=file_repo.pulp_href, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # Attempt to upload a second content unit with the same relative_path and overwrite=False | ||
| artifact_b = random_artifact_factory() | ||
| with pytest.raises(PulpTaskError) as exc_info: | ||
| monitor_task( | ||
| file_bindings.ContentFilesApi.create( | ||
| relative_path=shared_path, | ||
| artifact=artifact_b.pulp_href, | ||
| repository=file_repo.pulp_href, | ||
| overwrite=False, | ||
| ).task | ||
| ) | ||
| assert exc_info.value.task.error["description"] | ||
| assert "PLP0023" in exc_info.value.task.error["description"] | ||
|
|
||
| # Repo version should not have advanced | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
|
|
||
| @pytest.mark.parallel | ||
| def test_content_upload_overwrite_false_allows_non_conflicting( | ||
| file_bindings, | ||
| file_repo, | ||
| random_artifact_factory, | ||
| monitor_task, | ||
| ): | ||
| """Uploading content with overwrite=False and no conflict succeeds.""" | ||
| # Upload first content unit into the repo with overwrite=False | ||
| artifact_a = random_artifact_factory() | ||
| monitor_task( | ||
| file_bindings.ContentFilesApi.create( | ||
| relative_path=str(uuid.uuid4()), | ||
| artifact=artifact_a.pulp_href, | ||
| repository=file_repo.pulp_href, | ||
| overwrite=False, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/1/") | ||
|
|
||
| # Upload a second content unit with a different relative_path | ||
| artifact_b = random_artifact_factory() | ||
| monitor_task( | ||
| file_bindings.ContentFilesApi.create( | ||
| relative_path=str(uuid.uuid4()), | ||
| artifact=artifact_b.pulp_href, | ||
| repository=file_repo.pulp_href, | ||
| overwrite=False, | ||
| ).task | ||
| ) | ||
| repo = file_bindings.RepositoriesFileApi.read(file_repo.pulp_href) | ||
| assert repo.latest_version_href.endswith("/versions/2/") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ | |
| ) | ||
| from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS, PROTECTED_REPO_VERSION_MESSAGE | ||
| from pulpcore.download.factory import DownloaderFactory | ||
| from pulpcore.exceptions import ResourceImmutableError | ||
| from pulpcore.exceptions import ContentOverwriteError, ResourceImmutableError | ||
|
|
||
| from pulpcore.cache import Cache | ||
|
|
||
|
|
@@ -242,6 +242,73 @@ def finalize_new_version(self, new_version): | |
| """ | ||
| pass | ||
|
|
||
| def check_content_overwrite(self, version, add_content_pks, remove_content_pks=None): | ||
| """ | ||
| Check that content being added would not overwrite existing repository content. | ||
|
|
||
| Uses repo_key_fields to determine if any content to be added conflicts with content | ||
| already present in the version. Raises ContentOverwriteError (HTTP 409) if a conflict | ||
| is detected. | ||
|
|
||
| Plugin writers may override this method to customize overwrite-check behavior. | ||
|
|
||
| Args: | ||
| version (pulpcore.app.models.RepositoryVersion): The version to check against. | ||
| add_content_pks (list): List of primary keys for Content to be added. | ||
| remove_content_pks (list): Optional list of primary keys for Content being removed. | ||
| These are excluded from the conflict check. | ||
| """ | ||
| existing_content = version.content | ||
| if remove_content_pks: | ||
| existing_content = existing_content.exclude(pk__in=remove_content_pks) | ||
| if not existing_content.exists(): | ||
| return | ||
|
|
||
| add_content = Content.objects.filter(pk__in=add_content_pks) | ||
|
|
||
| for type_obj in self.CONTENT_TYPES: | ||
| repo_key_fields = type_obj.repo_key_fields | ||
| if not repo_key_fields: | ||
| continue | ||
|
|
||
| pulp_type = type_obj.get_pulp_type() | ||
| incoming_qs = type_obj.objects.filter(pk__in=add_content.filter(pulp_type=pulp_type)) | ||
| if not incoming_qs.exists(): | ||
| continue | ||
|
|
||
| for batch in batch_qs(incoming_qs.values(*repo_key_fields)): | ||
| find_dup_qs = Q() | ||
| for content_dict in batch: | ||
| find_dup_qs |= Q(**content_dict) | ||
|
|
||
| existing_dups = ( | ||
| type_obj.objects.filter(pk__in=existing_content) | ||
| .filter(find_dup_qs) | ||
| .values("pk", *repo_key_fields) | ||
| ) | ||
| if not existing_dups.exists(): | ||
| continue | ||
|
|
||
| existing_by_key = { | ||
| tuple(row[f] for f in repo_key_fields): row["pk"] for row in existing_dups | ||
| } | ||
| incoming_by_key = { | ||
| tuple(row[f] for f in repo_key_fields): row["pk"] | ||
| for row in incoming_qs.filter(find_dup_qs).values("pk", *repo_key_fields) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this query if we add |
||
| } | ||
| conflict_map = { | ||
| str(incoming_by_key[key]): str(existing_by_key[key]) | ||
| for key in incoming_by_key | ||
| if key in existing_by_key | ||
| } | ||
| if conflict_map: | ||
| _logger.info( | ||
| "Content overwrite conflict: type=%s, incoming->existing=%s", | ||
| pulp_type, | ||
| conflict_map, | ||
| ) | ||
| raise ContentOverwriteError(pulp_type, conflict_map) | ||
|
|
||
| def latest_version(self): | ||
| """ | ||
| Get the latest RepositoryVersion on a repository | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have this pattern in our repo-version utils file, but I wonder if it is more performant than a normal
type_obj.objects.filter(pk__in=add_content). Theoretically it should filter down the number of content to check to only those of the correct type, but does it actually increase the speed?