From e3d27cce08aa0e86f86c05736bbfe196c55e84b0 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sat, 18 Apr 2026 12:54:36 -0700 Subject: [PATCH] fix(epics): use actual group_id for save/delete operations on nested epics When an epic belonging to a subgroup is retrieved through a parent group's epic listing, save() and delete() operations would fail because they used the parent group's path instead of the epic's actual group_id. Add an internal custom path hook for object save and delete operations, and let UpdateMixin and DeleteMixin accept an explicit `_custom_path` keyword. GroupEpic uses this hook to build mutation paths from the epic's server-provided group_id, so save() and delete() target the epic's owning group. Avoid using parent manager attrs as a fallback for the epic group path. Lazy epics do not have a server-provided group_id, so raise a clear error instead of silently using the parent group path. Add unit coverage for custom update/delete paths, GroupEpic save/delete path selection, and lazy epic path failures. Add a functional test for saving a subgroup epic discovered through the parent group listing. Assisted-by: OpenAI Codex (GPT-5) Closes: #3261 --- gitlab/mixins.py | 46 +++++++++++-- gitlab/v4/objects/epics.py | 23 +++++++ tests/functional/api/test_epics.py | 60 ++++++++++++++++ tests/unit/mixins/test_mixin_methods.py | 91 +++++++++++++++++++++++++ tests/unit/objects/test_epics.py | 76 +++++++++++++++++++++ 5 files changed, 291 insertions(+), 5 deletions(-) create mode 100644 tests/unit/objects/test_epics.py diff --git a/gitlab/mixins.py b/gitlab/mixins.py index 51de97876..1f438536a 100644 --- a/gitlab/mixins.py +++ b/gitlab/mixins.py @@ -292,6 +292,8 @@ def update( self, id: str | int | None = None, new_data: dict[str, Any] | None = None, + *, + _custom_path: str | None = None, **kwargs: Any, ) -> dict[str, Any]: """Update an object on the server. @@ -299,6 +301,7 @@ def update( Args: id: ID of the object to update (can be None if not required) new_data: the update data for the object + _custom_path: Optional custom path for special API endpoints **kwargs: Extra options to send to the server (e.g. sudo) Returns: @@ -310,7 +313,9 @@ def update( """ new_data = new_data or {} - if id is None: + if _custom_path is not None: + path = _custom_path + elif id is None: path = self.path else: path = f"{self.path}/{utils.EncodedId(id)}" @@ -357,18 +362,27 @@ def set(self, key: str, value: str, **kwargs: Any) -> base.TObjCls: class DeleteMixin(base.RESTManager[base.TObjCls]): @exc.on_http_error(exc.GitlabDeleteError) - def delete(self, id: str | int | None = None, **kwargs: Any) -> None: + def delete( + self, + id: str | int | None = None, + *, + _custom_path: str | None = None, + **kwargs: Any, + ) -> None: """Delete an object on the server. Args: id: ID of the object to delete + _custom_path: Optional custom path for special API endpoints **kwargs: Extra options to send to the server (e.g. sudo) Raises: GitlabAuthenticationError: If authentication is not correct GitlabDeleteError: If the server cannot perform the request """ - if id is None: + if _custom_path is not None: + path = _custom_path + elif id is None: path = self.path else: path = f"{self.path}/{utils.EncodedId(id)}" @@ -403,6 +417,12 @@ class SaveMixin(_RestObjectBase): _updated_attrs: dict[str, Any] manager: base.RESTManager[Any] + def _get_custom_path(self) -> str | None: + # NOTE(jlvillal): pylint will complain for the callers with an + # 'assignment-from-none' error, if we don't do this. + custom_path: str | None = None + return custom_path + def _get_updated_data(self) -> dict[str, Any]: updated_data = {} for attr in self.manager._update_attrs.required: @@ -437,7 +457,13 @@ def save(self, **kwargs: Any) -> dict[str, Any] | None: obj_id = self.encoded_id if TYPE_CHECKING: assert isinstance(self.manager, UpdateMixin) - server_data = self.manager.update(obj_id, updated_data, **kwargs) + custom_path = self._get_custom_path() + if custom_path is None: + server_data = self.manager.update(obj_id, updated_data, **kwargs) + else: + server_data = self.manager.update( + obj_id, updated_data, _custom_path=custom_path, **kwargs + ) self._update_attrs(server_data) return server_data @@ -452,6 +478,12 @@ class ObjectDeleteMixin(_RestObjectBase): _updated_attrs: dict[str, Any] manager: base.RESTManager[Any] + def _get_custom_path(self) -> str | None: + # NOTE(jlvillal): pylint will complain for the callers with an + # 'assignment-from-none' error, if we don't do this. + custom_path: str | None = None + return custom_path + def delete(self, **kwargs: Any) -> None: """Delete the object from the server. @@ -465,7 +497,11 @@ def delete(self, **kwargs: Any) -> None: if TYPE_CHECKING: assert isinstance(self.manager, DeleteMixin) assert self.encoded_id is not None - self.manager.delete(self.encoded_id, **kwargs) + custom_path = self._get_custom_path() + if custom_path is None: + self.manager.delete(self.encoded_id, **kwargs) + else: + self.manager.delete(self.encoded_id, _custom_path=custom_path, **kwargs) class UserAgentDetailMixin(_RestObjectBase): diff --git a/gitlab/v4/objects/epics.py b/gitlab/v4/objects/epics.py index 06400528f..064f12d3d 100644 --- a/gitlab/v4/objects/epics.py +++ b/gitlab/v4/objects/epics.py @@ -2,6 +2,7 @@ from typing import Any, TYPE_CHECKING +import gitlab.utils from gitlab import exceptions as exc from gitlab import types from gitlab.base import RESTObject @@ -29,6 +30,28 @@ class GroupEpic(ObjectDeleteMixin, SaveMixin, RESTObject): resourcelabelevents: GroupEpicResourceLabelEventManager notes: GroupEpicNoteManager + def _epic_path(self) -> str: + """Return the API path for this epic using its real group.""" + if self._lazy: + raise AttributeError( + "Cannot compute epic path for a lazy epic: attribute 'group_id' " + "is missing. Fetch the epic without lazy=True before saving or " + "deleting it." + ) + + try: + group_id = self._attrs["group_id"] + except KeyError as error: + raise AttributeError( + "Cannot compute epic path: attribute 'group_id' is missing." + ) from error + + encoded_group_id = gitlab.utils.EncodedId(group_id) + return f"/groups/{encoded_group_id}/epics/{self.encoded_id}" + + def _get_custom_path(self) -> str | None: + return self._epic_path() + class GroupEpicManager(CRUDMixin[GroupEpic]): _path = "/groups/{group_id}/epics" diff --git a/tests/functional/api/test_epics.py b/tests/functional/api/test_epics.py index b61e23776..22be22cbb 100644 --- a/tests/functional/api/test_epics.py +++ b/tests/functional/api/test_epics.py @@ -1,5 +1,14 @@ +import collections.abc +import dataclasses +import uuid + import pytest +import gitlab +import gitlab.v4.objects.epics +import gitlab.v4.objects.groups +from tests.functional import helpers + pytestmark = pytest.mark.gitlab_premium @@ -32,3 +41,54 @@ def test_epic_notes(epic): epic.notes.create({"body": "Test note"}) new_notes = epic.notes.list(get_all=True) assert len(new_notes) == (len(notes) + 1), f"{new_notes} {notes}" + + +@dataclasses.dataclass(frozen=True) +class NestedEpicInSubgroup: + subgroup: gitlab.v4.objects.groups.Group + nested_epic: gitlab.v4.objects.epics.GroupEpic + + +@pytest.fixture +def nested_epic_in_subgroup( + gl: gitlab.Gitlab, group: gitlab.v4.objects.groups.Group +) -> collections.abc.Generator[NestedEpicInSubgroup, None, None]: + subgroup_id = uuid.uuid4().hex + subgroup = gl.groups.create( + { + "name": f"subgroup-{subgroup_id}", + "path": f"sg-{subgroup_id}", + "parent_id": group.id, + } + ) + + nested_epic = subgroup.epics.create( + {"title": f"Nested epic {subgroup_id}", "description": "Nested epic"} + ) + + try: + yield NestedEpicInSubgroup(subgroup=subgroup, nested_epic=nested_epic) + finally: + helpers.safe_delete(nested_epic) + helpers.safe_delete(subgroup) + + +def test_epic_save_from_parent_group_updates_subgroup_epic( + group: gitlab.v4.objects.groups.Group, nested_epic_in_subgroup: NestedEpicInSubgroup +) -> None: + fetched_epics = group.epics.list(search=nested_epic_in_subgroup.nested_epic.title) + assert fetched_epics, "Expected to discover nested epic via parent group list" + + fetched_epic = fetched_epics[0] + assert ( + fetched_epic.id == nested_epic_in_subgroup.nested_epic.id + ), "Parent group listing did not include nested epic" + + new_label = f"nested-{uuid.uuid4().hex}" + fetched_epic.labels = [new_label] + fetched_epic.save() + + refreshed_epic = nested_epic_in_subgroup.subgroup.epics.get( + nested_epic_in_subgroup.nested_epic.iid + ) + assert new_label in refreshed_epic.labels diff --git a/tests/unit/mixins/test_mixin_methods.py b/tests/unit/mixins/test_mixin_methods.py index fb6ded881..ad280507f 100644 --- a/tests/unit/mixins/test_mixin_methods.py +++ b/tests/unit/mixins/test_mixin_methods.py @@ -12,6 +12,7 @@ GetMixin, GetWithoutIdMixin, ListMixin, + ObjectDeleteMixin, RefreshMixin, SaveMixin, SetMixin, @@ -421,6 +422,27 @@ class M(UpdateMixin, FakeManager): assert responses.assert_call_count(url, 1) is True +@responses.activate +def test_update_mixin_custom_path(gl): + class M(UpdateMixin, FakeManager): + pass + + url = "http://localhost/api/v4/others/42" + responses.add( + method=responses.PUT, + url=url, + json={"id": 42, "foo": "baz"}, + status=200, + match=[responses.matchers.query_param_matcher({})], + ) + + mgr = M(gl) + server_data = mgr.update(42, {"foo": "baz"}, _custom_path="/others/42") + assert isinstance(server_data, dict) + assert server_data["foo"] == "baz" + assert responses.assert_call_count(url, 1) is True + + @responses.activate def test_delete_mixin(gl): class M(DeleteMixin, FakeManager): @@ -440,6 +462,25 @@ class M(DeleteMixin, FakeManager): assert responses.assert_call_count(url, 1) is True +@responses.activate +def test_delete_mixin_custom_path(gl): + class M(DeleteMixin, FakeManager): + pass + + url = "http://localhost/api/v4/others/42" + responses.add( + method=responses.DELETE, + url=url, + json="", + status=200, + match=[responses.matchers.query_param_matcher({})], + ) + + mgr = M(gl) + mgr.delete(42, _custom_path="/others/42") + assert responses.assert_call_count(url, 1) is True + + @responses.activate def test_save_mixin(gl): class M(UpdateMixin, FakeManager): @@ -466,6 +507,32 @@ class TestClass(SaveMixin, base.RESTObject): assert responses.assert_call_count(url, 1) is True +@responses.activate +def test_save_mixin_custom_path(gl): + class M(UpdateMixin, FakeManager): + pass + + class TestClass(SaveMixin, base.RESTObject): + def _get_custom_path(self): + return "/others/42" + + url = "http://localhost/api/v4/others/42" + responses.add( + method=responses.PUT, + url=url, + json={"id": 42, "foo": "baz"}, + status=200, + match=[responses.matchers.query_param_matcher({})], + ) + + mgr = M(gl) + obj = TestClass(mgr, {"id": 42, "foo": "bar"}) + obj.foo = "baz" + obj.save() + assert obj._attrs["foo"] == "baz" + assert responses.assert_call_count(url, 1) is True + + @responses.activate def test_save_mixin_without_new_data(gl): class M(UpdateMixin, FakeManager): @@ -485,6 +552,30 @@ class TestClass(SaveMixin, base.RESTObject): assert responses.assert_call_count(url, 0) is True +@responses.activate +def test_object_delete_mixin_custom_path(gl): + class M(DeleteMixin, FakeManager): + pass + + class TestClass(ObjectDeleteMixin, base.RESTObject): + def _get_custom_path(self): + return "/others/42" + + url = "http://localhost/api/v4/others/42" + responses.add( + method=responses.DELETE, + url=url, + json="", + status=200, + match=[responses.matchers.query_param_matcher({})], + ) + + mgr = M(gl) + obj = TestClass(mgr, {"id": 42}) + obj.delete() + assert responses.assert_call_count(url, 1) is True + + @responses.activate def test_set_mixin(gl): class M(SetMixin, FakeManager): diff --git a/tests/unit/objects/test_epics.py b/tests/unit/objects/test_epics.py new file mode 100644 index 000000000..48330f903 --- /dev/null +++ b/tests/unit/objects/test_epics.py @@ -0,0 +1,76 @@ +from typing import Any + +import pytest +import responses + +import gitlab.base +import gitlab.v4.objects.epics +import gitlab.v4.objects.groups + + +def _build_epic( + manager: gitlab.v4.objects.epics.GroupEpicManager, + iid: int = 3, + group_id: int = 2, + title: str = "Epic", +) -> gitlab.v4.objects.epics.GroupEpic: + data: dict[str, int | str] = {"iid": iid, "group_id": group_id, "title": title} + return gitlab.v4.objects.epics.GroupEpic(manager, data) + + +def test_group_epic_save_uses_actual_group_path( + group: gitlab.v4.objects.groups.Group, +) -> None: + epic_manager = group.epics + epic = _build_epic(epic_manager, title="Original") + epic.title = "Updated" + + with responses.RequestsMock() as rsps: + rsps.add( + method=responses.PUT, + url="http://localhost/api/v4/groups/2/epics/3", + json={"iid": 3, "group_id": 2, "title": "Updated"}, + content_type="application/json", + status=200, + match=[responses.matchers.json_params_matcher({"title": "Updated"})], + ) + + epic.save() + + assert epic.title == "Updated" + + +def test_group_epic_delete_uses_actual_group_path( + group: gitlab.v4.objects.groups.Group, +) -> None: + epic_manager = group.epics + epic = _build_epic(epic_manager) + + with responses.RequestsMock() as rsps: + rsps.add( + method=responses.DELETE, + url="http://localhost/api/v4/groups/2/epics/3", + status=204, + ) + + epic.delete() + + assert len(epic._updated_attrs) == 0 + + +def test_group_epic_path_requires_group_id( + fake_manager: gitlab.base.RESTManager[Any], +) -> None: + epic = gitlab.v4.objects.epics.GroupEpic(manager=fake_manager, attrs={"iid": 5}) + + with pytest.raises(AttributeError): + epic._epic_path() + + +def test_group_epic_path_requires_real_group_id_for_lazy_epic( + group: gitlab.v4.objects.groups.Group, +) -> None: + epic = group.epics.get(3, lazy=True) + + with pytest.raises(AttributeError, match="lazy epic"): + epic._epic_path()