Skip to content

Commit 4702278

Browse files
JohnVillalovosJohnVillalovos
committed
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. This commit overrides save() and delete() methods in GroupEpic to use the epic's group_id attribute to construct the correct API path, ensuring operations work correctly regardless of how the epic was retrieved. Also add the ability to pass a custom path using `_pg_custom_path` to the `UpdateMixin.update()` and `SaveMixin.save()` methods. This allowed the override of the `update()` method to re-use the `SaveMixin.save()` method. Closes: #3261
1 parent 659c648 commit 4702278

5 files changed

Lines changed: 272 additions & 5 deletions

File tree

‎gitlab/mixins.py‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -292,13 +292,16 @@ def update(
292292
self,
293293
id: str | int | None = None,
294294
new_data: dict[str, Any] | None = None,
295+
*,
296+
_custom_path: str | None = None,
295297
**kwargs: Any,
296298
) -> dict[str, Any]:
297299
"""Update an object on the server.
298300
299301
Args:
300302
id: ID of the object to update (can be None if not required)
301303
new_data: the update data for the object
304+
_custom_path: Optional custom path for special API endpoints
302305
**kwargs: Extra options to send to the server (e.g. sudo)
303306
304307
Returns:
@@ -310,7 +313,9 @@ def update(
310313
"""
311314
new_data = new_data or {}
312315

313-
if id is None:
316+
if _custom_path is not None:
317+
path = _custom_path
318+
elif id is None:
314319
path = self.path
315320
else:
316321
path = f"{self.path}/{utils.EncodedId(id)}"
@@ -357,18 +362,27 @@ def set(self, key: str, value: str, **kwargs: Any) -> base.TObjCls:
357362

358363
class DeleteMixin(base.RESTManager[base.TObjCls]):
359364
@exc.on_http_error(exc.GitlabDeleteError)
360-
def delete(self, id: str | int | None = None, **kwargs: Any) -> None:
365+
def delete(
366+
self,
367+
id: str | int | None = None,
368+
*,
369+
_custom_path: str | None = None,
370+
**kwargs: Any,
371+
) -> None:
361372
"""Delete an object on the server.
362373
363374
Args:
364375
id: ID of the object to delete
376+
_custom_path: Optional custom path for special API endpoints
365377
**kwargs: Extra options to send to the server (e.g. sudo)
366378
367379
Raises:
368380
GitlabAuthenticationError: If authentication is not correct
369381
GitlabDeleteError: If the server cannot perform the request
370382
"""
371-
if id is None:
383+
if _custom_path is not None:
384+
path = _custom_path
385+
elif id is None:
372386
path = self.path
373387
else:
374388
path = f"{self.path}/{utils.EncodedId(id)}"
@@ -403,6 +417,12 @@ class SaveMixin(_RestObjectBase):
403417
_updated_attrs: dict[str, Any]
404418
manager: base.RESTManager[Any]
405419

420+
def _get_custom_path(self) -> str | None:
421+
# NOTE(jlvillal): pylint will complain for the callers with an
422+
# 'assignment-from-none' error, if we don't do this.
423+
custom_path: str | None = None
424+
return custom_path
425+
406426
def _get_updated_data(self) -> dict[str, Any]:
407427
updated_data = {}
408428
for attr in self.manager._update_attrs.required:
@@ -437,7 +457,13 @@ def save(self, **kwargs: Any) -> dict[str, Any] | None:
437457
obj_id = self.encoded_id
438458
if TYPE_CHECKING:
439459
assert isinstance(self.manager, UpdateMixin)
440-
server_data = self.manager.update(obj_id, updated_data, **kwargs)
460+
custom_path = self._get_custom_path()
461+
if custom_path is None:
462+
server_data = self.manager.update(obj_id, updated_data, **kwargs)
463+
else:
464+
server_data = self.manager.update(
465+
obj_id, updated_data, _custom_path=custom_path, **kwargs
466+
)
441467
self._update_attrs(server_data)
442468
return server_data
443469

@@ -452,6 +478,12 @@ class ObjectDeleteMixin(_RestObjectBase):
452478
_updated_attrs: dict[str, Any]
453479
manager: base.RESTManager[Any]
454480

481+
def _get_custom_path(self) -> str | None:
482+
# NOTE(jlvillal): pylint will complain for the callers with an
483+
# 'assignment-from-none' error, if we don't do this.
484+
custom_path: str | None = None
485+
return custom_path
486+
455487
def delete(self, **kwargs: Any) -> None:
456488
"""Delete the object from the server.
457489
@@ -465,7 +497,11 @@ def delete(self, **kwargs: Any) -> None:
465497
if TYPE_CHECKING:
466498
assert isinstance(self.manager, DeleteMixin)
467499
assert self.encoded_id is not None
468-
self.manager.delete(self.encoded_id, **kwargs)
500+
custom_path = self._get_custom_path()
501+
if custom_path is None:
502+
self.manager.delete(self.encoded_id, **kwargs)
503+
else:
504+
self.manager.delete(self.encoded_id, _custom_path=custom_path, **kwargs)
469505

470506

471507
class UserAgentDetailMixin(_RestObjectBase):

‎gitlab/v4/objects/epics.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from typing import Any, TYPE_CHECKING
44

5+
import gitlab.utils
56
from gitlab import exceptions as exc
67
from gitlab import types
78
from gitlab.base import RESTObject
@@ -29,6 +30,18 @@ class GroupEpic(ObjectDeleteMixin, SaveMixin, RESTObject):
2930
resourcelabelevents: GroupEpicResourceLabelEventManager
3031
notes: GroupEpicNoteManager
3132

33+
def _epic_path(self) -> str:
34+
"""Return the API path for this epic using its real group."""
35+
if not hasattr(self, "group_id") or self.group_id is None:
36+
raise AttributeError(
37+
"Cannot compute epic path: attribute 'group_id' is missing."
38+
)
39+
encoded_group_id = gitlab.utils.EncodedId(self.group_id)
40+
return f"/groups/{encoded_group_id}/epics/{self.encoded_id}"
41+
42+
def _get_custom_path(self) -> str | None:
43+
return self._epic_path()
44+
3245

3346
class GroupEpicManager(CRUDMixin[GroupEpic]):
3447
_path = "/groups/{group_id}/epics"

‎tests/functional/api/test_epics.py‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
1+
import collections.abc
2+
import dataclasses
3+
import uuid
4+
15
import pytest
26

7+
import gitlab
8+
import gitlab.v4.objects.epics
9+
import gitlab.v4.objects.groups
10+
from tests.functional import helpers
11+
312
pytestmark = pytest.mark.gitlab_premium
413

514

@@ -32,3 +41,54 @@ def test_epic_notes(epic):
3241
epic.notes.create({"body": "Test note"})
3342
new_notes = epic.notes.list(get_all=True)
3443
assert len(new_notes) == (len(notes) + 1), f"{new_notes} {notes}"
44+
45+
46+
@dataclasses.dataclass(frozen=True)
47+
class NestedEpicInSubgroup:
48+
subgroup: gitlab.v4.objects.groups.Group
49+
nested_epic: gitlab.v4.objects.epics.GroupEpic
50+
51+
52+
@pytest.fixture
53+
def nested_epic_in_subgroup(
54+
gl: gitlab.Gitlab, group: gitlab.v4.objects.groups.Group
55+
) -> collections.abc.Generator[NestedEpicInSubgroup, None, None]:
56+
subgroup_id = uuid.uuid4().hex
57+
subgroup = gl.groups.create(
58+
{
59+
"name": f"subgroup-{subgroup_id}",
60+
"path": f"sg-{subgroup_id}",
61+
"parent_id": group.id,
62+
}
63+
)
64+
65+
nested_epic = subgroup.epics.create(
66+
{"title": f"Nested epic {subgroup_id}", "description": "Nested epic"}
67+
)
68+
69+
try:
70+
yield NestedEpicInSubgroup(subgroup=subgroup, nested_epic=nested_epic)
71+
finally:
72+
helpers.safe_delete(nested_epic)
73+
helpers.safe_delete(subgroup)
74+
75+
76+
def test_epic_save_from_parent_group_updates_subgroup_epic(
77+
group: gitlab.v4.objects.groups.Group, nested_epic_in_subgroup: NestedEpicInSubgroup
78+
) -> None:
79+
fetched_epics = group.epics.list(search=nested_epic_in_subgroup.nested_epic.title)
80+
assert fetched_epics, "Expected to discover nested epic via parent group list"
81+
82+
fetched_epic = fetched_epics[0]
83+
assert (
84+
fetched_epic.id == nested_epic_in_subgroup.nested_epic.id
85+
), "Parent group listing did not include nested epic"
86+
87+
new_label = f"nested-{uuid.uuid4().hex}"
88+
fetched_epic.labels = [new_label]
89+
fetched_epic.save()
90+
91+
refreshed_epic = nested_epic_in_subgroup.subgroup.epics.get(
92+
nested_epic_in_subgroup.nested_epic.iid
93+
)
94+
assert new_label in refreshed_epic.labels

‎tests/unit/mixins/test_mixin_methods.py‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
GetMixin,
1313
GetWithoutIdMixin,
1414
ListMixin,
15+
ObjectDeleteMixin,
1516
RefreshMixin,
1617
SaveMixin,
1718
SetMixin,
@@ -421,6 +422,27 @@ class M(UpdateMixin, FakeManager):
421422
assert responses.assert_call_count(url, 1) is True
422423

423424

425+
@responses.activate
426+
def test_update_mixin_custom_path(gl):
427+
class M(UpdateMixin, FakeManager):
428+
pass
429+
430+
url = "http://localhost/api/v4/others/42"
431+
responses.add(
432+
method=responses.PUT,
433+
url=url,
434+
json={"id": 42, "foo": "baz"},
435+
status=200,
436+
match=[responses.matchers.query_param_matcher({})],
437+
)
438+
439+
mgr = M(gl)
440+
server_data = mgr.update(42, {"foo": "baz"}, _custom_path="/others/42")
441+
assert isinstance(server_data, dict)
442+
assert server_data["foo"] == "baz"
443+
assert responses.assert_call_count(url, 1) is True
444+
445+
424446
@responses.activate
425447
def test_delete_mixin(gl):
426448
class M(DeleteMixin, FakeManager):
@@ -440,6 +462,25 @@ class M(DeleteMixin, FakeManager):
440462
assert responses.assert_call_count(url, 1) is True
441463

442464

465+
@responses.activate
466+
def test_delete_mixin_custom_path(gl):
467+
class M(DeleteMixin, FakeManager):
468+
pass
469+
470+
url = "http://localhost/api/v4/others/42"
471+
responses.add(
472+
method=responses.DELETE,
473+
url=url,
474+
json="",
475+
status=200,
476+
match=[responses.matchers.query_param_matcher({})],
477+
)
478+
479+
mgr = M(gl)
480+
mgr.delete(42, _custom_path="/others/42")
481+
assert responses.assert_call_count(url, 1) is True
482+
483+
443484
@responses.activate
444485
def test_save_mixin(gl):
445486
class M(UpdateMixin, FakeManager):
@@ -466,6 +507,32 @@ class TestClass(SaveMixin, base.RESTObject):
466507
assert responses.assert_call_count(url, 1) is True
467508

468509

510+
@responses.activate
511+
def test_save_mixin_custom_path(gl):
512+
class M(UpdateMixin, FakeManager):
513+
pass
514+
515+
class TestClass(SaveMixin, base.RESTObject):
516+
def _get_custom_path(self):
517+
return "/others/42"
518+
519+
url = "http://localhost/api/v4/others/42"
520+
responses.add(
521+
method=responses.PUT,
522+
url=url,
523+
json={"id": 42, "foo": "baz"},
524+
status=200,
525+
match=[responses.matchers.query_param_matcher({})],
526+
)
527+
528+
mgr = M(gl)
529+
obj = TestClass(mgr, {"id": 42, "foo": "bar"})
530+
obj.foo = "baz"
531+
obj.save()
532+
assert obj._attrs["foo"] == "baz"
533+
assert responses.assert_call_count(url, 1) is True
534+
535+
469536
@responses.activate
470537
def test_save_mixin_without_new_data(gl):
471538
class M(UpdateMixin, FakeManager):
@@ -485,6 +552,30 @@ class TestClass(SaveMixin, base.RESTObject):
485552
assert responses.assert_call_count(url, 0) is True
486553

487554

555+
@responses.activate
556+
def test_object_delete_mixin_custom_path(gl):
557+
class M(DeleteMixin, FakeManager):
558+
pass
559+
560+
class TestClass(ObjectDeleteMixin, base.RESTObject):
561+
def _get_custom_path(self):
562+
return "/others/42"
563+
564+
url = "http://localhost/api/v4/others/42"
565+
responses.add(
566+
method=responses.DELETE,
567+
url=url,
568+
json="",
569+
status=200,
570+
match=[responses.matchers.query_param_matcher({})],
571+
)
572+
573+
mgr = M(gl)
574+
obj = TestClass(mgr, {"id": 42})
575+
obj.delete()
576+
assert responses.assert_call_count(url, 1) is True
577+
578+
488579
@responses.activate
489580
def test_set_mixin(gl):
490581
class M(SetMixin, FakeManager):

0 commit comments

Comments
 (0)