fix(objects): make lists work for filters in all objects #1420

nejch · 2021-04-27T18:10:21Z

Well, this was tedious 😁 Follow-up to #1413.

Long term I don't see this being a good idea to maintain manually. Maybe later we can just convert any list we get in the data for ListMixin. Either way - finding candidates was grep -r 'type: Array' lib/api in the gitlab repo, and weeding out non-GET params and endpoints not implemented by python-gitlab:

lib/api/resource_access_tokens.rb:          requires :scopes, type: Array[String], desc: "The permissions of the token"
lib/api/deploy_tokens.rb:        requires :scopes, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s),
lib/api/deploy_tokens.rb:        requires :scopes, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s),
lib/api/rubygem_packages.rb:            optional :gems, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma delimited gem names'
lib/api/releases.rb:          optional :links, type: Array do
lib/api/releases.rb:        optional :milestones, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The titles of the related milestones', default: []
lib/api/releases.rb:        optional :milestones,  type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The titles of the related milestones'
lib/api/settings.rb:      optional :asset_proxy_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Deprecated: Use :asset_proxy_allowlist instead. Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted.'
lib/api/settings.rb:      optional :asset_proxy_allowlist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically allowed.'
lib/api/settings.rb:      optional :disabled_oauth_sign_in_sources, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Disable certain OAuth sign-in sources'
lib/api/settings.rb:      optional :domain_denylist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com'
lib/api/settings.rb:      optional :domain_allowlist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'ONLY users with e-mail addresses that match these domain(s) will be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com'
lib/api/settings.rb:      optional :import_sources, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce,
lib/api/settings.rb:      optional :restricted_visibility_levels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Selected levels cannot be used by non-admin users for groups, projects or snippets. If the public level is restricted, user profiles are only visible to logged in users.'
lib/api/repositories.rb:        requires :refs, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce
lib/api/milestone_responses.rb:          optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IIDs of the milestones'
lib/api/container_registry_event.rb:        requires :events, type: Array
lib/api/commits.rb:        requires :actions, type: Array, desc: 'Actions to perform in commit' do
lib/api/helpers/merge_requests_helpers.rb:        optional :assignee_username, type: Array[String], check_assignees_count: true,
lib/api/helpers/merge_requests_helpers.rb:                 type: Array[String],
lib/api/helpers/projects_helpers.rb:        optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The list of tags for a project'
lib/api/helpers/snippets_helpers.rb:        optional :files, type: Array, desc: 'An array of files' do
lib/api/helpers/snippets_helpers.rb:        optional :files, type: Array, desc: 'An array of files to update' do
lib/api/members.rb:          optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of user ids to look up for membership'
lib/api/members.rb:          optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of user ids to look up for membership'
lib/api/projects.rb:        optional :topic, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of topics. Limit results to projects having all topics'
lib/api/projects.rb:        optional :skip_users, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Filter out users with the specified IDs'
lib/api/projects.rb:        optional :skip_groups, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of group ids to exclude from list'
lib/api/suggestions.rb:        requires :ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: "An array of suggestion ID's"
lib/api/ci/runner.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: %q(List of Runner's tags)
lib/api/ci/runners.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show'
lib/api/ci/runners.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show'
lib/api/ci/runners.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The list of tags for a runner'
lib/api/ci/runners.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show'
lib/api/ci/runners.rb:          optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show'
lib/api/issues.rb:        optional :labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/issues.rb:        optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IID array of issues'
lib/api/issues.rb:        optional :assignee_username, type: Array[String], check_assignees_count: true,
lib/api/issues.rb:        optional :labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/issues.rb:        optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IID array of issues'
lib/api/issues.rb:        optional :assignee_username, type: Array[String], check_assignees_count: true,
lib/api/issues.rb:        optional :assignee_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The array of user IDs to assign issue'
lib/api/issues.rb:        optional :labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/issues.rb:        optional :add_labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/issues.rb:        optional :remove_labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/groups.rb:        optional :skip_groups, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of group ids to exclude from list'
lib/api/users.rb:            optional :scopes, type: Array, desc: 'The array of scopes of the impersonation token'
lib/api/users.rb:            requires :scopes, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, values: ::Gitlab::Auth.all_available_scopes.map(&:to_s),
lib/api/feature_flags.rb:          optional :scopes, type: Array do
lib/api/feature_flags.rb:          optional :strategies, type: Array do
lib/api/feature_flags.rb:            optional :scopes, type: Array do
lib/api/feature_flags.rb:          optional :strategies, type: Array do
lib/api/feature_flags.rb:            optional :scopes, type: Array do
lib/api/merge_requests.rb:          optional :assignee_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Comma-separated list of assignee ids'
lib/api/merge_requests.rb:          optional :reviewer_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Comma-separated list of reviewer ids'
lib/api/merge_requests.rb:          optional :labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/merge_requests.rb:          optional :add_labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/merge_requests.rb:          optional :remove_labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names'
lib/api/merge_requests.rb:        optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IID array of merge requests'
lib/api/merge_requests.rb:        requires :commits, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, allow_blank: false, desc: 'List of context commits sha'
lib/api/merge_requests.rb:        requires :commits, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, allow_blank: false, desc: 'List of context commits sha'

Closes #1256
Closes #1419

codecov-commenter · 2021-04-27T18:27:26Z

Codecov Report

Merging #1420 (603a351) into master (a6b6cd4) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1420      +/-   ##
==========================================
+ Coverage   80.24%   80.53%   +0.29%     
==========================================
  Files          73       73              
  Lines        4064     4080      +16     
==========================================
+ Hits         3261     3286      +25     
+ Misses        803      794       -9

Flag	Coverage Δ
unit	`80.53% <100.00%> (+0.29%)`	⬆️

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

Impacted Files	Coverage Δ
gitlab/v4/objects/deploy_tokens.py	`100.00% <100.00%> (ø)`
gitlab/v4/objects/groups.py	`76.66% <100.00%> (+0.26%)`	⬆️
gitlab/v4/objects/issues.py	`80.88% <100.00%> (ø)`
gitlab/v4/objects/members.py	`82.85% <100.00%> (+1.03%)`	⬆️
gitlab/v4/objects/merge_requests.py	`65.45% <100.00%> (ø)`
gitlab/v4/objects/milestones.py	`70.90% <100.00%> (+1.09%)`	⬆️
gitlab/v4/objects/projects.py	`73.09% <100.00%> (ø)`
gitlab/v4/objects/runners.py	`98.14% <100.00%> (+0.18%)`	⬆️
gitlab/v4/objects/settings.py	`73.68% <100.00%> (+3.09%)`	⬆️
gitlab/v4/objects/users.py	`90.64% <100.00%> (+0.05%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6b6cd4...603a351. Read the comment docs.

JohnVillalovos

Looks good.

For the future it would be nice to have functional tests for some of these, but that is a MUCH bigger task.

Thanks!

JohnVillalovos · 2021-04-28T04:10:08Z

Long term I don't see this being a good idea to maintain manually. Maybe later we can just convert any list we get in the data for ListMixin. Either way - finding candidates was grep -r 'type: Array' lib/api in the gitlab repo, and weeding out non-GET params and endpoints not implemented by python-gitlab:

That sounds like a good idea. I think it should be possible to do that. If we have a list for the value then we can assume it needs to be changed. That could be to either make it into a comma-delimited string or split it up into multiple key[]=value1&key[]=value2

nejch · 2021-04-28T05:37:28Z

Looks good.

For the future it would be nice to have functional tests for some of these, but that is a MUCH bigger task.

Thanks!

Yes agreed :) I'll try to add more fixtures so that writing functional tests is a bit quicker for these things.

JohnVillalovos · 2021-05-01T17:55:30Z

@nejch I'm okay with merging this. Not sure if you also want @max-wittig to approve too. But I have no complaints if you want to merge it 🙂

nejch · 2021-05-06T19:16:19Z

Sure, assigning to you as you were pretty involved in the initial issue anyway :) I just pushed a small assert though that should prove we fixed at least the linked issue.

The test_groups() case is not pretty, but that's for historical reasons as it was already a headache to split some old tests that had a hundreds of steps interdependent across different sections 😁 hence the TODO there

JohnVillalovos · 2021-05-06T19:24:59Z

Still looks good to me. An additional test is a nice addition.

fix(objects): allow lists for filters for in all objects

603a351

nejch force-pushed the fix/missing-list-attributes branch from 1181e53 to 603a351 Compare April 27, 2021 18:25

nejch assigned max-wittig Apr 27, 2021

JohnVillalovos approved these changes Apr 28, 2021

View reviewed changes

test(functional): add test for skip_groups list filter

a014774

nejch assigned JohnVillalovos and unassigned max-wittig May 6, 2021

JohnVillalovos merged commit 45edae9 into master May 6, 2021

nejch mentioned this pull request May 15, 2021

fix: make scopes work with multiple scope-names #1360

Closed

nejch deleted the fix/missing-list-attributes branch May 30, 2021 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(objects): make lists work for filters in all objects #1420

fix(objects): make lists work for filters in all objects #1420

nejch commented Apr 27, 2021 •

edited

Loading

codecov-commenter commented Apr 27, 2021

JohnVillalovos left a comment

JohnVillalovos commented Apr 28, 2021

nejch commented Apr 28, 2021

JohnVillalovos commented May 1, 2021

nejch commented May 6, 2021

JohnVillalovos commented May 6, 2021

fix(objects): make lists work for filters in all objects #1420

fix(objects): make lists work for filters in all objects #1420

Conversation

nejch commented Apr 27, 2021 • edited Loading

codecov-commenter commented Apr 27, 2021

Codecov Report

JohnVillalovos left a comment

Choose a reason for hiding this comment

JohnVillalovos commented Apr 28, 2021

nejch commented Apr 28, 2021

JohnVillalovos commented May 1, 2021

nejch commented May 6, 2021

JohnVillalovos commented May 6, 2021

nejch commented Apr 27, 2021 •

edited

Loading