Codecov Report

Merging #1699 (1af44ce) into main (194ee01) will decrease coverage by 0.00%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main    #1699      +/-   ##
==========================================
- Coverage   95.54%   95.53%   -0.01%     
==========================================
  Files          81       81              
  Lines        5296     5309      +13     
==========================================
+ Hits         5060     5072      +12     
- Misses        236      237       +1     

Flags with carried forward coverage won't be shown. Click here to find out more.

So in my testing this does not break anything. It also allows approver_ids and approved_by_ids to work for ONE specified user (before it just failed) in listing merge requests. But it does not work for two or more, as it just returns the match for the first one.

I'm not a fan of the silent failure as that is likely worse than current situation where it dies.

@nejch This is ready for review now. Interested to hear your feedback. Thanks.

I finally had some more time to look at this in more detail! I have some thoughts to maybe simplify this.

We only need to do encoding when sending data as query parameters, not as a payload. So I think the encoding part is relevant for GET requests, as we can use json-encoded data as provided otherwise. I just tested this with topics on project creation, which is an array:
project = gl.projects.create({"name":"test-repo", "visibility": "private", "topics": ["gitlab", "python"]})

GitLab's docs suggest this too:

GET requests usually send a query string, while PUT or POST requests usually send the payload body

So https://docs.gitlab.com/ee/api/#encoding-api-parameters-of-array-and-hash-types is a bit misleading here because they don't use application/json as they do in the example above that section, and it should be possible for us to do that instead as long as we ensure it is sent as JSON and not as query params. See also #780 (comment) where an array is sent successfully via the API when created as a list. (Of course we still need to later have some basic support hash/array of hashes in order to parse them from the CLI).

And I'm thinking that complex data structures only make sense for POST/PUT requests, while arrays are still useful for GET for list filters, for example. So we only need to encode arrays properly for query params, IMO. This would really simplify things as all we need to do is transform e.g. {"scopes": ["api", "read_user"} into {"scopes[]": ["api", "read_user"}, because requests already does a urlencode there:
https://github.com/psf/requests/blob/8bce583b9547c7b82d44c8e97f37cf9a16cbe758/requests/models.py#L112

E.g.:

>>> from urllib.parse import urlencode
>>> urlencode({"scopes": ["api", "read_user"]}, doseq=True)
'scopes=api&scopes=read_user'
>>> urlencode({"scopes[]": ["api", "read_user"]}, doseq=True)
'scopes%5B%5D=api&scopes%5B%5D=read_user'

I feel like we could avoid some of the conversions happening here if we do it like that. WDYT @JohnVillalovos?

Should we try to revive this one @JohnVillalovos? Seems like an important fix. Probably fixes a few more issues down the line, will need to check those.

Should we try to revive this one @JohnVillalovos? Seems like an important fix. Probably fixes a few more issues down the line, will need to check those.

Yeah, probably. Let me see if I get some time this weekend. Thanks for reminding me!

@nejch It is ready for review again. Thanks for your patience.

I have to take that back - seems like this also transforms the key for POST/PUT payloads which shouldn't happen. This maybe needs a test with create() to ensure we don't use key[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in the data portion is a different thing we need to figure out.

I have to take that back - seems like this also transforms the key for POST/PUT payloads which shouldn't happen. This maybe needs a test with create() to ensure we don't use key[] in payloads.

I think we need to transform "parameters'. Whether it is a GET/POST/PUT. Now handling values in the data portion is a different thing we need to figure out.

Yes, that's right :) sorry for the confusion. payload = no transform, query params = transform.

@JohnVillalovos I think this doesn't work quite right yet. Can you try this test in this patch?

diff --git a/tests/unit/mixins/test_mixin_methods.py b/tests/unit/mixins/test_mixin_methods.py
index 69cdb78..e8870b9 100644
--- a/tests/unit/mixins/test_mixin_methods.py
+++ b/tests/unit/mixins/test_mixin_methods.py
@@ -314,6 +314,26 @@ def test_create_mixin_custom_path(gl):
     assert responses.assert_call_count(url, 1) is True
 
 
+
+@responses.activate
+def test_create_mixin_with_attributes(gl):
+    class M(CreateMixin, FakeManager):
+        _types = {"my_array": gl_types.ArrayAttribute}
+
+    url = "http://localhost/api/v4/tests"
+    responses.add(
+        method=responses.POST,
+        headers={},
+        url=url,
+        json={},
+        status=200,
+        match=[responses.matchers.json_params_matcher({"my_array": ["1", "2", "3"]})],
+    )
+
+    mgr = M(gl)
+    mgr.create({"my_array": [1, 2, 3]})
+
+
 def test_update_mixin_missing_attrs(gl):
     class M(UpdateMixin, FakeManager):
         _update_attrs = gl_types.RequiredOptional(

The reason we might want this is, for example scopes as an array in create mixins for #780.

@JohnVillalovos I think this doesn't work quite right yet. Can you try this test in this patch?

Thanks @nejch I totally missed that the data was not being sent as params in the POST/PUT methods 😔

Updated the code to only modify the data for the list() method.