Skip to content

Commit cb08077

Browse files
fregataaclaudelablup-octodog
authored
fix(BA-5311): Fix UpdateRoleInput null handling for description field (#10354)
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: octodog <mu001@lablup.com>
1 parent f4a11e9 commit cb08077

6 files changed

Lines changed: 307 additions & 21 deletions

File tree

‎changes/10354.fix.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix UpdateRoleInput to correctly handle explicit null values for the description field using TriState.from_graphql()

‎docs/manager/graphql-reference/supergraph.graphql‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13233,9 +13233,9 @@ input UpdateRoleInput
1323313233
@join__type(graph: STRAWBERRY)
1323413234
{
1323513235
id: UUID!
13236-
name: String = null
13237-
description: String = null
13238-
status: RoleStatus = null
13236+
name: String
13237+
description: String
13238+
status: RoleStatus
1323913239
}
1324013240

1324113241
"""Added in 25.19.0. Input for updating route traffic status."""

‎docs/manager/graphql-reference/v2-schema.graphql‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8131,9 +8131,9 @@ type UpdateResourceGroupPayload {
81318131
"""Added in 26.3.0. Input for updating a role"""
81328132
input UpdateRoleInput {
81338133
id: UUID!
8134-
name: String = null
8135-
description: String = null
8136-
status: RoleStatus = null
8134+
name: String
8135+
description: String
8136+
status: RoleStatus
81378137
}
81388138

81398139
"""Added in 25.19.0. Input for updating route traffic status."""

‎src/ai/backend/manager/api/gql/rbac/types/role.py‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from typing import TYPE_CHECKING, Annotated, Any, Self, override
1010

1111
import strawberry
12-
from strawberry import ID, Info
12+
from strawberry import ID, UNSET, Info
1313
from strawberry.relay import Connection, Edge, Node, NodeID
1414

1515
from ai.backend.common.data.permission.types import (
@@ -564,23 +564,15 @@ def to_creator(self) -> Creator[RoleRow]:
564564
@strawberry.input(description="Added in 26.3.0. Input for updating a role")
565565
class UpdateRoleInput:
566566
id: uuid.UUID
567-
name: str | None = None
568-
description: str | None = None
569-
status: RoleStatusGQL | None = None
567+
name: str | None = UNSET
568+
description: str | None = UNSET
569+
status: RoleStatusGQL | None = UNSET
570570

571571
def to_updater(self) -> Updater[RoleRow]:
572572
spec = RoleUpdaterSpec(
573-
name=OptionalState.update(self.name) if self.name is not None else OptionalState.nop(),
574-
description=(
575-
TriState.update(self.description)
576-
if self.description is not None
577-
else TriState.nop()
578-
),
579-
status=(
580-
OptionalState.update(self.status.to_internal())
581-
if self.status is not None
582-
else OptionalState.nop()
583-
),
573+
name=OptionalState.from_graphql(self.name),
574+
description=TriState.from_graphql(self.description),
575+
status=OptionalState.from_graphql(self.status).map(lambda s: s.to_internal()),
584576
)
585577
return Updater(spec=spec, pk_value=self.id)
586578

‎tests/unit/manager/api/gql/rbac/test_role_input.py‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
"""Unit tests for UpdateRoleInput.to_updater()."""
2+
3+
from __future__ import annotations
4+
5+
import uuid
6+
7+
from ai.backend.common.data.permission.types import RoleStatus
8+
from ai.backend.manager.api.gql.rbac.types.role import RoleStatusGQL, UpdateRoleInput
9+
from ai.backend.manager.repositories.permission_controller.updaters import RoleUpdaterSpec
10+
from ai.backend.manager.types import _TriStateEnum
11+
12+
13+
class TestUpdateRoleInputToUpdater:
14+
"""Tests for UpdateRoleInput.to_updater() method."""
15+
16+
def test_description_with_none_creates_nullify_tristate(self) -> None:
17+
"""UpdateRoleInput with description=None → TriState.nullify() in updater spec."""
18+
role_input = UpdateRoleInput(
19+
id=uuid.uuid4(),
20+
description=None,
21+
)
22+
23+
updater = role_input.to_updater()
24+
spec = updater.spec
25+
assert isinstance(spec, RoleUpdaterSpec)
26+
27+
assert spec.description._state == _TriStateEnum.NULLIFY
28+
29+
def test_description_with_unset_creates_nop_tristate(self) -> None:
30+
"""UpdateRoleInput with description=UNSET → TriState.nop() in updater spec."""
31+
role_input = UpdateRoleInput(
32+
id=uuid.uuid4(),
33+
# description is omitted (UNSET by default)
34+
)
35+
36+
updater = role_input.to_updater()
37+
spec = updater.spec
38+
assert isinstance(spec, RoleUpdaterSpec)
39+
40+
assert spec.description._state == _TriStateEnum.NOP
41+
42+
def test_description_with_string_creates_update_tristate(self) -> None:
43+
"""UpdateRoleInput with description="text" → TriState.update("text") in updater spec."""
44+
role_input = UpdateRoleInput(
45+
id=uuid.uuid4(),
46+
description="some text",
47+
)
48+
49+
updater = role_input.to_updater()
50+
spec = updater.spec
51+
assert isinstance(spec, RoleUpdaterSpec)
52+
53+
assert spec.description._state == _TriStateEnum.UPDATE
54+
assert spec.description.value() == "some text"
55+
56+
def test_name_with_unset_creates_nop_optionalstate(self) -> None:
57+
"""UpdateRoleInput with name=UNSET → OptionalState.nop() in updater spec."""
58+
role_input = UpdateRoleInput(
59+
id=uuid.uuid4(),
60+
# name is omitted (UNSET by default)
61+
)
62+
63+
updater = role_input.to_updater()
64+
spec = updater.spec
65+
assert isinstance(spec, RoleUpdaterSpec)
66+
67+
assert spec.name._state == _TriStateEnum.NOP
68+
69+
def test_name_with_string_creates_update_optionalstate(self) -> None:
70+
"""UpdateRoleInput with name="new name" → OptionalState.update("new name") in updater spec."""
71+
role_input = UpdateRoleInput(
72+
id=uuid.uuid4(),
73+
name="new name",
74+
)
75+
76+
updater = role_input.to_updater()
77+
spec = updater.spec
78+
assert isinstance(spec, RoleUpdaterSpec)
79+
80+
assert spec.name._state == _TriStateEnum.UPDATE
81+
assert spec.name.value() == "new name"
82+
83+
def test_status_with_unset_creates_nop_optionalstate(self) -> None:
84+
"""UpdateRoleInput with status=UNSET → OptionalState.nop() in updater spec."""
85+
role_input = UpdateRoleInput(
86+
id=uuid.uuid4(),
87+
# status is omitted (UNSET by default)
88+
)
89+
90+
updater = role_input.to_updater()
91+
spec = updater.spec
92+
assert isinstance(spec, RoleUpdaterSpec)
93+
94+
assert spec.status._state == _TriStateEnum.NOP
95+
96+
def test_status_with_active_creates_update_optionalstate(self) -> None:
97+
"""UpdateRoleInput with status=RoleStatusGQL.ACTIVE → OptionalState.update(RoleStatus.ACTIVE)."""
98+
role_input = UpdateRoleInput(
99+
id=uuid.uuid4(),
100+
status=RoleStatusGQL.ACTIVE,
101+
)
102+
103+
updater = role_input.to_updater()
104+
spec = updater.spec
105+
assert isinstance(spec, RoleUpdaterSpec)
106+
107+
assert spec.status._state == _TriStateEnum.UPDATE
108+
assert spec.status.value() == RoleStatus.ACTIVE
109+
110+
def test_status_with_inactive_creates_update_optionalstate(self) -> None:
111+
"""UpdateRoleInput with status=RoleStatusGQL.INACTIVE → OptionalState.update(RoleStatus.INACTIVE)."""
112+
role_input = UpdateRoleInput(
113+
id=uuid.uuid4(),
114+
status=RoleStatusGQL.INACTIVE,
115+
)
116+
117+
updater = role_input.to_updater()
118+
spec = updater.spec
119+
assert isinstance(spec, RoleUpdaterSpec)
120+
121+
assert spec.status._state == _TriStateEnum.UPDATE
122+
assert spec.status.value() == RoleStatus.INACTIVE
123+
124+
def test_multiple_fields_with_mixed_states(self) -> None:
125+
"""Test multiple fields with different states."""
126+
role_id = uuid.uuid4()
127+
role_input = UpdateRoleInput(
128+
id=role_id,
129+
name="updated role",
130+
description=None,
131+
status=RoleStatusGQL.ACTIVE,
132+
)
133+
134+
updater = role_input.to_updater()
135+
spec = updater.spec
136+
assert isinstance(spec, RoleUpdaterSpec)
137+
138+
assert updater.pk_value == role_id
139+
assert spec.name._state == _TriStateEnum.UPDATE
140+
assert spec.name.value() == "updated role"
141+
assert spec.description._state == _TriStateEnum.NULLIFY
142+
assert spec.status._state == _TriStateEnum.UPDATE
143+
assert spec.status.value() == RoleStatus.ACTIVE
144+
145+
def test_all_fields_unset_creates_all_nop(self) -> None:
146+
"""When all fields are UNSET, all spec fields should be nop."""
147+
role_input = UpdateRoleInput(
148+
id=uuid.uuid4(),
149+
# all fields omitted (UNSET by default)
150+
)
151+
152+
updater = role_input.to_updater()
153+
spec = updater.spec
154+
assert isinstance(spec, RoleUpdaterSpec)
155+
156+
assert spec.name._state == _TriStateEnum.NOP
157+
assert spec.description._state == _TriStateEnum.NOP
158+
assert spec.status._state == _TriStateEnum.NOP
159+
160+
def test_updater_pk_value_matches_input_id(self) -> None:
161+
"""Updater.pk_value should match the input id."""
162+
role_id = uuid.uuid4()
163+
role_input = UpdateRoleInput(id=role_id)
164+
165+
updater = role_input.to_updater()
166+
167+
assert updater.pk_value == role_id

‎tests/unit/manager/repositories/permission_controller/test_role_updaters.py‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
"""Unit tests for RoleUpdaterSpec."""
2+
3+
from __future__ import annotations
4+
5+
from ai.backend.manager.data.permission.status import RoleStatus
6+
from ai.backend.manager.data.permission.types import RoleSource
7+
from ai.backend.manager.models.rbac_models.role import RoleRow
8+
from ai.backend.manager.repositories.permission_controller.updaters import RoleUpdaterSpec
9+
from ai.backend.manager.types import OptionalState, TriState
10+
11+
12+
class TestRoleUpdaterSpecBuildValues:
13+
"""Tests for RoleUpdaterSpec.build_values() method."""
14+
15+
def test_build_values_with_all_nop_returns_empty_dict(self) -> None:
16+
"""When all fields are nop, build_values should return empty dict."""
17+
spec = RoleUpdaterSpec()
18+
19+
result = spec.build_values()
20+
21+
assert result == {}
22+
23+
def test_build_values_with_description_nullify(self) -> None:
24+
"""When description is nullify, build_values should include description: None."""
25+
spec = RoleUpdaterSpec(
26+
description=TriState.nullify(),
27+
)
28+
29+
result = spec.build_values()
30+
31+
assert result == {"description": None}
32+
33+
def test_build_values_with_description_nop(self) -> None:
34+
"""When description is nop, build_values should not include description key."""
35+
spec = RoleUpdaterSpec(
36+
description=TriState.nop(),
37+
)
38+
39+
result = spec.build_values()
40+
41+
assert "description" not in result
42+
43+
def test_build_values_with_description_update(self) -> None:
44+
"""When description is update, build_values should include the description value."""
45+
spec = RoleUpdaterSpec(
46+
description=TriState.update("Updated description"),
47+
)
48+
49+
result = spec.build_values()
50+
51+
assert result == {"description": "Updated description"}
52+
53+
def test_build_values_with_name_update(self) -> None:
54+
"""Test name field is included when set."""
55+
spec = RoleUpdaterSpec(
56+
name=OptionalState.update("new_role_name"),
57+
)
58+
59+
result = spec.build_values()
60+
61+
assert result == {"name": "new_role_name"}
62+
63+
def test_build_values_with_status_update(self) -> None:
64+
"""Test status field is included when set."""
65+
spec = RoleUpdaterSpec(
66+
status=OptionalState.update(RoleStatus.ACTIVE),
67+
)
68+
69+
result = spec.build_values()
70+
71+
assert result == {"status": RoleStatus.ACTIVE}
72+
73+
def test_build_values_with_source_update(self) -> None:
74+
"""Test source field is included when set."""
75+
spec = RoleUpdaterSpec(
76+
source=OptionalState.update(RoleSource.CUSTOM),
77+
)
78+
79+
result = spec.build_values()
80+
81+
assert result == {"source": RoleSource.CUSTOM}
82+
83+
def test_build_values_with_multiple_fields(self) -> None:
84+
"""Test multiple fields are correctly included."""
85+
spec = RoleUpdaterSpec(
86+
name=OptionalState.update("updated_role"),
87+
source=OptionalState.update(RoleSource.CUSTOM),
88+
status=OptionalState.update(RoleStatus.INACTIVE),
89+
description=TriState.update("Updated role description"),
90+
)
91+
92+
result = spec.build_values()
93+
94+
assert result == {
95+
"name": "updated_role",
96+
"source": RoleSource.CUSTOM,
97+
"status": RoleStatus.INACTIVE,
98+
"description": "Updated role description",
99+
}
100+
101+
def test_build_values_with_mixed_states(self) -> None:
102+
"""Test with a mix of nop, update, and nullify states."""
103+
spec = RoleUpdaterSpec(
104+
name=OptionalState.update("role_name"),
105+
source=OptionalState.nop(), # Should not be included
106+
status=OptionalState.update(RoleStatus.ACTIVE),
107+
description=TriState.nullify(), # Should be None
108+
)
109+
110+
result = spec.build_values()
111+
112+
assert result == {
113+
"name": "role_name",
114+
"status": RoleStatus.ACTIVE,
115+
"description": None,
116+
}
117+
118+
119+
class TestRoleUpdaterSpecRowClass:
120+
"""Tests for RoleUpdaterSpec.row_class property."""
121+
122+
def test_row_class_returns_role_row(self) -> None:
123+
"""row_class should return RoleRow."""
124+
spec = RoleUpdaterSpec()
125+
126+
assert spec.row_class is RoleRow

0 commit comments

Comments
 (0)