fix: use the [] after key names for array variables

JohnVillalovos · JohnVillalovos · commit 1591a9114ad1 · 2021-11-18T20:30:37.000-08:00
This is not a complete fix. We need to re-architect things so that for
the 'params' parameter passed into `requests.request()` we use a list
of tuples. The reason for this is then we can pass in items which
contain the same key but different values. For example we want to be
able to pass [('assigned_ids[]', 1), ('assigned_ids[]', 2)] which is
not something we can do with a dictionary.

1. Create a new CsvStringAttribute class. This is to indicate types
   which are sent to the GitLab server as comma-separated-strings
   (CSV) but we have been allowing users to use a list-of-strings.
   These values are NOT array values, so adding [] to the key name
   breaks them.

2. Rename ListAttribute to ArrayAttribute.

3. If a value is of type ArrayAttribute then append '[]' to the name
   of the value.
diff --git a/gitlab/mixins.py b/gitlab/mixins.py
@@ -231,7 +231,17 @@ def list(self, **kwargs: Any) -> Union[base.RESTObjectList, List[base.RESTObject
             for attr_name, type_cls in self._types.items():
                 if attr_name in data.keys():
                     type_obj = type_cls(data[attr_name])
-                    data[attr_name] = type_obj.get_for_api()
+                    if isinstance(type_obj, g_types.ArrayAttribute):
+                        # NOTE(jlvillal): Array values are supposed to be
+                        # passed in the form of 'key[]=value1&key[]=value2'
+                        # https://docs.gitlab.com/ee/api/#array
+                        # But at the moment we haven't determined an elegant
+                        # way to do that. So for now just add the '[]' to the
+                        # key we send in the request.
+                        del data[attr_name]
+                        data[f"{attr_name}[]"] = type_obj.get_for_api()
+                    else:
+                        data[attr_name] = type_obj.get_for_api()
 
         # Allow to overwrite the path, handy for custom listings
         path = data.pop("path", self.path)
@@ -313,6 +323,15 @@ def create(
                     if isinstance(type_obj, g_types.FileAttribute):
                         k = type_obj.get_file_name(attr_name)
                         files[attr_name] = (k, data.pop(attr_name))
+                    elif isinstance(type_obj, g_types.ArrayAttribute):
+                        # NOTE(jlvillal): Array values are supposed to be
+                        # passed in the form of 'key[]=value1&key[]=value2'
+                        # https://docs.gitlab.com/ee/api/#array
+                        # But at the moment we haven't determined an elegant
+                        # way to do that. So for now just add the '[]' to the
+                        # key we send in the request.
+                        del data[attr_name]
+                        data[f"{attr_name}[]"] = type_obj.get_for_api()
                     else:
                         data[attr_name] = type_obj.get_for_api()
 
@@ -409,6 +428,15 @@ def update(
                     if isinstance(type_obj, g_types.FileAttribute):
                         k = type_obj.get_file_name(attr_name)
                         files[attr_name] = (k, new_data.pop(attr_name))
+                    elif isinstance(type_obj, g_types.ArrayAttribute):
+                        # NOTE(jlvillal): Array values are supposed to be
+                        # passed in the form of 'key[]=value1&key[]=value2'
+                        # https://docs.gitlab.com/ee/api/#array
+                        # But at the moment we haven't determined an elegant
+                        # way to do that. So for now just add the '[]' to the
+                        # key we send in the request.
+                        del new_data[attr_name]
+                        new_data[f"{attr_name}[]"] = type_obj.get_for_api()
                     else:
                         new_data[attr_name] = type_obj.get_for_api()
 
diff --git a/gitlab/types.py b/gitlab/types.py
@@ -32,7 +32,31 @@ def get_for_api(self) -> Any:
         return self._value
 
 
-class ListAttribute(GitlabAttribute):
+class ArrayAttribute(GitlabAttribute):
+    """To support `array` types as documented in
+    https://docs.gitlab.com/ee/api/#array"""
+
+    def set_from_cli(self, cli_value: str) -> None:
+        if not cli_value.strip():
+            self._value = []
+        else:
+            self._value = [item.strip() for item in cli_value.split(",")]
+
+    def get_for_api(self) -> str:
+        # Do not comma-split single value passed as string
+        if isinstance(self._value, str):
+            return self._value
+
+        if TYPE_CHECKING:
+            assert isinstance(self._value, list)
+        return ",".join([str(x) for x in self._value])
+
+
+class CsvStringAttribute(GitlabAttribute):
+    """For values which are sent to the server as a Comma Separated Values
+    (CSV) string.  We allow them to be specified as a list and we convert it
+    into a CSV"""
+
     def set_from_cli(self, cli_value: str) -> None:
         if not cli_value.strip():
             self._value = []
diff --git a/gitlab/v4/objects/deploy_tokens.py b/gitlab/v4/objects/deploy_tokens.py
@@ -39,7 +39,7 @@ class GroupDeployTokenManager(ListMixin, CreateMixin, DeleteMixin, RESTManager):
             "username",
         ),
     )
-    _types = {"scopes": types.ListAttribute}
+    _types = {"scopes": types.CsvStringAttribute}
 
 
 class ProjectDeployToken(ObjectDeleteMixin, RESTObject):
@@ -60,4 +60,4 @@ class ProjectDeployTokenManager(ListMixin, CreateMixin, DeleteMixin, RESTManager
             "username",
         ),
     )
-    _types = {"scopes": types.ListAttribute}
+    _types = {"scopes": types.CsvStringAttribute}
diff --git a/gitlab/v4/objects/epics.py b/gitlab/v4/objects/epics.py
@@ -40,7 +40,7 @@ class GroupEpicManager(CRUDMixin, RESTManager):
     _update_attrs = RequiredOptional(
         optional=("title", "labels", "description", "start_date", "end_date"),
     )
-    _types = {"labels": types.ListAttribute}
+    _types = {"labels": types.CsvStringAttribute}
 
 
 class GroupEpicIssue(ObjectDeleteMixin, SaveMixin, RESTObject):
diff --git a/gitlab/v4/objects/groups.py b/gitlab/v4/objects/groups.py
@@ -290,7 +290,7 @@ class GroupManager(CRUDMixin, RESTManager):
             "shared_runners_setting",
         ),
     )
-    _types = {"avatar": types.ImageAttribute, "skip_groups": types.ListAttribute}
+    _types = {"avatar": types.ImageAttribute, "skip_groups": types.ArrayAttribute}
 
     def get(self, id: Union[str, int], lazy: bool = False, **kwargs: Any) -> Group:
         return cast(Group, super().get(id=id, lazy=lazy, **kwargs))
@@ -350,7 +350,7 @@ class GroupSubgroupManager(ListMixin, RESTManager):
         "with_custom_attributes",
         "min_access_level",
     )
-    _types = {"skip_groups": types.ListAttribute}
+    _types = {"skip_groups": types.ArrayAttribute}
 
 
 class GroupDescendantGroup(RESTObject):
diff --git a/gitlab/v4/objects/issues.py b/gitlab/v4/objects/issues.py
@@ -63,7 +63,7 @@ class IssueManager(RetrieveMixin, RESTManager):
         "updated_after",
         "updated_before",
     )
-    _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}
+    _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}
 
 
 class GroupIssue(RESTObject):
@@ -90,7 +90,7 @@ class GroupIssueManager(ListMixin, RESTManager):
         "updated_after",
         "updated_before",
     )
-    _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}
+    _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}
 
 
 class ProjectIssue(
@@ -220,7 +220,7 @@ class ProjectIssueManager(CRUDMixin, RESTManager):
             "discussion_locked",
         ),
     )
-    _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}
+    _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}
 
 
 class ProjectIssueLink(ObjectDeleteMixin, RESTObject):
diff --git a/gitlab/v4/objects/members.py b/gitlab/v4/objects/members.py
@@ -39,7 +39,7 @@ class GroupMemberManager(CRUDMixin, RESTManager):
     _update_attrs = RequiredOptional(
         required=("access_level",), optional=("expires_at",)
     )
-    _types = {"user_ids": types.ListAttribute}
+    _types = {"user_ids": types.ArrayAttribute}
 
     def get(
         self, id: Union[str, int], lazy: bool = False, **kwargs: Any
@@ -95,7 +95,7 @@ class ProjectMemberManager(CRUDMixin, RESTManager):
     _update_attrs = RequiredOptional(
         required=("access_level",), optional=("expires_at",)
     )
-    _types = {"user_ids": types.ListAttribute}
+    _types = {"user_ids": types.ArrayAttribute}
 
     def get(
         self, id: Union[str, int], lazy: bool = False, **kwargs: Any
diff --git a/gitlab/v4/objects/merge_requests.py b/gitlab/v4/objects/merge_requests.py
@@ -95,10 +95,9 @@ class MergeRequestManager(ListMixin, RESTManager):
         "deployed_after",
     )
     _types = {
-        "approver_ids": types.ListAttribute,
-        "approved_by_ids": types.ListAttribute,
-        "in": types.ListAttribute,
-        "labels": types.ListAttribute,
+        "approver_ids": types.ArrayAttribute,
+        "approved_by_ids": types.ArrayAttribute,
+        "in": types.CsvStringAttribute,
     }
 
 
@@ -133,9 +132,8 @@ class GroupMergeRequestManager(ListMixin, RESTManager):
         "wip",
     )
     _types = {
-        "approver_ids": types.ListAttribute,
-        "approved_by_ids": types.ListAttribute,
-        "labels": types.ListAttribute,
+        "approver_ids": types.ArrayAttribute,
+        "approved_by_ids": types.ArrayAttribute,
     }
 
 
@@ -450,10 +448,9 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):
         "wip",
     )
     _types = {
-        "approver_ids": types.ListAttribute,
-        "approved_by_ids": types.ListAttribute,
-        "iids": types.ListAttribute,
-        "labels": types.ListAttribute,
+        "approver_ids": types.ArrayAttribute,
+        "approved_by_ids": types.ArrayAttribute,
+        "iids": types.ArrayAttribute,
     }
 
     def get(
diff --git a/gitlab/v4/objects/milestones.py b/gitlab/v4/objects/milestones.py
@@ -89,7 +89,7 @@ class GroupMilestoneManager(CRUDMixin, RESTManager):
         optional=("title", "description", "due_date", "start_date", "state_event"),
     )
     _list_filters = ("iids", "state", "search")
-    _types = {"iids": types.ListAttribute}
+    _types = {"iids": types.ArrayAttribute}
 
 
 class ProjectMilestone(PromoteMixin, SaveMixin, ObjectDeleteMixin, RESTObject):
@@ -164,4 +164,4 @@ class ProjectMilestoneManager(CRUDMixin, RESTManager):
         optional=("title", "description", "due_date", "start_date", "state_event"),
     )
     _list_filters = ("iids", "state", "search")
-    _types = {"iids": types.ListAttribute}
+    _types = {"iids": types.ArrayAttribute}
diff --git a/gitlab/v4/objects/projects.py b/gitlab/v4/objects/projects.py
@@ -771,7 +771,7 @@ class ProjectManager(CRUDMixin, RESTManager):
         "with_merge_requests_enabled",
         "with_programming_language",
     )
-    _types = {"avatar": types.ImageAttribute, "topic": types.ListAttribute}
+    _types = {"avatar": types.ImageAttribute, "topic": types.CsvStringAttribute}
 
     def get(self, id: Union[str, int], lazy: bool = False, **kwargs: Any) -> Project:
         return cast(Project, super().get(id=id, lazy=lazy, **kwargs))
diff --git a/gitlab/v4/objects/runners.py b/gitlab/v4/objects/runners.py
@@ -68,7 +68,7 @@ class RunnerManager(CRUDMixin, RESTManager):
         ),
     )
     _list_filters = ("scope", "tag_list")
-    _types = {"tag_list": types.ListAttribute}
+    _types = {"tag_list": types.CsvStringAttribute}
 
     @cli.register_custom_action("RunnerManager", tuple(), ("scope",))
     @exc.on_http_error(exc.GitlabListError)
@@ -130,7 +130,7 @@ class GroupRunnerManager(ListMixin, RESTManager):
     _from_parent_attrs = {"group_id": "id"}
     _create_attrs = RequiredOptional(required=("runner_id",))
     _list_filters = ("scope", "tag_list")
-    _types = {"tag_list": types.ListAttribute}
+    _types = {"tag_list": types.CsvStringAttribute}
 
 
 class ProjectRunner(ObjectDeleteMixin, RESTObject):
@@ -143,4 +143,4 @@ class ProjectRunnerManager(CreateMixin, DeleteMixin, ListMixin, RESTManager):
     _from_parent_attrs = {"project_id": "id"}
     _create_attrs = RequiredOptional(required=("runner_id",))
     _list_filters = ("scope", "tag_list")
-    _types = {"tag_list": types.ListAttribute}
+    _types = {"tag_list": types.CsvStringAttribute}
diff --git a/gitlab/v4/objects/settings.py b/gitlab/v4/objects/settings.py
@@ -80,12 +80,12 @@ class ApplicationSettingsManager(GetWithoutIdMixin, UpdateMixin, RESTManager):
         ),
     )
     _types = {
-        "asset_proxy_allowlist": types.ListAttribute,
-        "disabled_oauth_sign_in_sources": types.ListAttribute,
-        "domain_allowlist": types.ListAttribute,
-        "domain_denylist": types.ListAttribute,
-        "import_sources": types.ListAttribute,
-        "restricted_visibility_levels": types.ListAttribute,
+        "asset_proxy_allowlist": types.ArrayAttribute,
+        "disabled_oauth_sign_in_sources": types.ArrayAttribute,
+        "domain_allowlist": types.ArrayAttribute,
+        "domain_denylist": types.ArrayAttribute,
+        "import_sources": types.ArrayAttribute,
+        "restricted_visibility_levels": types.ArrayAttribute,
     }
 
     @exc.on_http_error(exc.GitlabUpdateError)
diff --git a/gitlab/v4/objects/users.py b/gitlab/v4/objects/users.py
@@ -359,7 +359,7 @@ class ProjectUserManager(ListMixin, RESTManager):
     _obj_cls = ProjectUser
     _from_parent_attrs = {"project_id": "id"}
     _list_filters = ("search", "skip_users")
-    _types = {"skip_users": types.ListAttribute}
+    _types = {"skip_users": types.ArrayAttribute}
 
 
 class UserEmail(ObjectDeleteMixin, RESTObject):
diff --git a/tests/unit/test_types.py b/tests/unit/test_types.py
@@ -30,42 +30,42 @@ def test_gitlab_attribute_get():
     assert o._value is None
 
 
-def test_list_attribute_input():
-    o = types.ListAttribute()
+def test_array_attribute_input():
+    o = types.ArrayAttribute()
     o.set_from_cli("foo,bar,baz")
     assert o.get() == ["foo", "bar", "baz"]
 
     o.set_from_cli("foo")
     assert o.get() == ["foo"]
 
 
-def test_list_attribute_empty_input():
-    o = types.ListAttribute()
+def test_array_attribute_empty_input():
+    o = types.ArrayAttribute()
     o.set_from_cli("")
     assert o.get() == []
 
     o.set_from_cli("  ")
     assert o.get() == []
 
 
-def test_list_attribute_get_for_api_from_cli():
-    o = types.ListAttribute()
+def test_array_attribute_get_for_api_from_cli():
+    o = types.ArrayAttribute()
     o.set_from_cli("foo,bar,baz")
     assert o.get_for_api() == "foo,bar,baz"
 
 
-def test_list_attribute_get_for_api_from_list():
-    o = types.ListAttribute(["foo", "bar", "baz"])
+def test_array_attribute_get_for_api_from_list():
+    o = types.ArrayAttribute(["foo", "bar", "baz"])
     assert o.get_for_api() == "foo,bar,baz"
 
 
-def test_list_attribute_get_for_api_from_int_list():
-    o = types.ListAttribute([1, 9, 7])
+def test_array_attribute_get_for_api_from_int_list():
+    o = types.ArrayAttribute([1, 9, 7])
     assert o.get_for_api() == "1,9,7"
 
 
-def test_list_attribute_does_not_split_string():
-    o = types.ListAttribute("foo")
+def test_array_attribute_does_not_split_string():
+    o = types.ArrayAttribute("foo")
     assert o.get_for_api() == "foo"
 
 

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ class GroupDeployTokenManager(ListMixin, CreateMixin, DeleteMixin, RESTManager):`
`39`	`39`	`"username",`
`40`	`40`	`),`
`41`	`41`	`)`
`42`		`- _types = {"scopes": types.ListAttribute}`
	`42`	`+ _types = {"scopes": types.CsvStringAttribute}`
`43`	`43`
`44`	`44`
`45`	`45`	`class ProjectDeployToken(ObjectDeleteMixin, RESTObject):`
`@@ -60,4 +60,4 @@ class ProjectDeployTokenManager(ListMixin, CreateMixin, DeleteMixin, RESTManager`
`60`	`60`	`"username",`
`61`	`61`	`),`
`62`	`62`	`)`
`63`		`- _types = {"scopes": types.ListAttribute}`
	`63`	`+ _types = {"scopes": types.CsvStringAttribute}`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ class GroupEpicManager(CRUDMixin, RESTManager):`
`40`	`40`	`_update_attrs = RequiredOptional(`
`41`	`41`	`optional=("title", "labels", "description", "start_date", "end_date"),`
`42`	`42`	`)`
`43`		`- _types = {"labels": types.ListAttribute}`
	`43`	`+ _types = {"labels": types.CsvStringAttribute}`
`44`	`44`
`45`	`45`
`46`	`46`	`class GroupEpicIssue(ObjectDeleteMixin, SaveMixin, RESTObject):`
Original file line number	Diff line number	Diff line change
`@@ -290,7 +290,7 @@ class GroupManager(CRUDMixin, RESTManager):`
`290`	`290`	`"shared_runners_setting",`
`291`	`291`	`),`
`292`	`292`	`)`
`293`		`- _types = {"avatar": types.ImageAttribute, "skip_groups": types.ListAttribute}`
	`293`	`+ _types = {"avatar": types.ImageAttribute, "skip_groups": types.ArrayAttribute}`
`294`	`294`
`295`	`295`	`def get(self, id: Union[str, int], lazy: bool = False, **kwargs: Any) -> Group:`
`296`	`296`	`return cast(Group, super().get(id=id, lazy=lazy, **kwargs))`
`@@ -350,7 +350,7 @@ class GroupSubgroupManager(ListMixin, RESTManager):`
`350`	`350`	`"with_custom_attributes",`
`351`	`351`	`"min_access_level",`
`352`	`352`	`)`
`353`		`- _types = {"skip_groups": types.ListAttribute}`
	`353`	`+ _types = {"skip_groups": types.ArrayAttribute}`
`354`	`354`
`355`	`355`
`356`	`356`	`class GroupDescendantGroup(RESTObject):`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ class IssueManager(RetrieveMixin, RESTManager):`
`63`	`63`	`"updated_after",`
`64`	`64`	`"updated_before",`
`65`	`65`	`)`
`66`		`- _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}`
	`66`	`+ _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}`
`67`	`67`
`68`	`68`
`69`	`69`	`class GroupIssue(RESTObject):`
`@@ -90,7 +90,7 @@ class GroupIssueManager(ListMixin, RESTManager):`
`90`	`90`	`"updated_after",`
`91`	`91`	`"updated_before",`
`92`	`92`	`)`
`93`		`- _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}`
	`93`	`+ _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}`
`94`	`94`
`95`	`95`
`96`	`96`	`class ProjectIssue(`
`@@ -220,7 +220,7 @@ class ProjectIssueManager(CRUDMixin, RESTManager):`
`220`	`220`	`"discussion_locked",`
`221`	`221`	`),`
`222`	`222`	`)`
`223`		`- _types = {"iids": types.ListAttribute, "labels": types.ListAttribute}`
	`223`	`+ _types = {"iids": types.ArrayAttribute, "labels": types.CsvStringAttribute}`
`224`	`224`
`225`	`225`
`226`	`226`	`class ProjectIssueLink(ObjectDeleteMixin, RESTObject):`
Original file line number	Diff line number	Diff line change
`@@ -95,10 +95,9 @@ class MergeRequestManager(ListMixin, RESTManager):`
`95`	`95`	`"deployed_after",`
`96`	`96`	`)`
`97`	`97`	`_types = {`
`98`		`- "approver_ids": types.ListAttribute,`
`99`		`- "approved_by_ids": types.ListAttribute,`
`100`		`- "in": types.ListAttribute,`
`101`		`- "labels": types.ListAttribute,`
	`98`	`+ "approver_ids": types.ArrayAttribute,`
	`99`	`+ "approved_by_ids": types.ArrayAttribute,`
	`100`	`+ "in": types.CsvStringAttribute,`
`102`	`101`	`}`
`103`	`102`
`104`	`103`
`@@ -133,9 +132,8 @@ class GroupMergeRequestManager(ListMixin, RESTManager):`
`133`	`132`	`"wip",`
`134`	`133`	`)`
`135`	`134`	`_types = {`
`136`		`- "approver_ids": types.ListAttribute,`
`137`		`- "approved_by_ids": types.ListAttribute,`
`138`		`- "labels": types.ListAttribute,`
	`135`	`+ "approver_ids": types.ArrayAttribute,`
	`136`	`+ "approved_by_ids": types.ArrayAttribute,`
`139`	`137`	`}`
`140`	`138`
`141`	`139`
`@@ -450,10 +448,9 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):`
`450`	`448`	`"wip",`
`451`	`449`	`)`
`452`	`450`	`_types = {`
`453`		`- "approver_ids": types.ListAttribute,`
`454`		`- "approved_by_ids": types.ListAttribute,`
`455`		`- "iids": types.ListAttribute,`
`456`		`- "labels": types.ListAttribute,`
	`451`	`+ "approver_ids": types.ArrayAttribute,`
	`452`	`+ "approved_by_ids": types.ArrayAttribute,`
	`453`	`+ "iids": types.ArrayAttribute,`
`457`	`454`	`}`
`458`	`455`
`459`	`456`	`def get(`
Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ class GroupMilestoneManager(CRUDMixin, RESTManager):`
`89`	`89`	`optional=("title", "description", "due_date", "start_date", "state_event"),`
`90`	`90`	`)`
`91`	`91`	`_list_filters = ("iids", "state", "search")`
`92`		`- _types = {"iids": types.ListAttribute}`
	`92`	`+ _types = {"iids": types.ArrayAttribute}`
`93`	`93`
`94`	`94`
`95`	`95`	`class ProjectMilestone(PromoteMixin, SaveMixin, ObjectDeleteMixin, RESTObject):`
`@@ -164,4 +164,4 @@ class ProjectMilestoneManager(CRUDMixin, RESTManager):`
`164`	`164`	`optional=("title", "description", "due_date", "start_date", "state_event"),`
`165`	`165`	`)`
`166`	`166`	`_list_filters = ("iids", "state", "search")`
`167`		`- _types = {"iids": types.ListAttribute}`
	`167`	`+ _types = {"iids": types.ArrayAttribute}`
Original file line number	Diff line number	Diff line change
`@@ -771,7 +771,7 @@ class ProjectManager(CRUDMixin, RESTManager):`
`771`	`771`	`"with_merge_requests_enabled",`
`772`	`772`	`"with_programming_language",`
`773`	`773`	`)`
`774`		`- _types = {"avatar": types.ImageAttribute, "topic": types.ListAttribute}`
	`774`	`+ _types = {"avatar": types.ImageAttribute, "topic": types.CsvStringAttribute}`
`775`	`775`
`776`	`776`	`def get(self, id: Union[str, int], lazy: bool = False, **kwargs: Any) -> Project:`
`777`	`777`	`return cast(Project, super().get(id=id, lazy=lazy, **kwargs))`