From d612f34e9b4b8702fbad710db72e3e2a0d88f204 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Sat, 16 Jul 2022 10:55:05 +0200 Subject: [PATCH] Support Project Avatar removal in REST API This change set adds support to remove project avatars using the REST API, much like it's already possible for the [Topics API](https://docs.gitlab.com/ee/api/topics.html#remove-a-topic-avatar). To support this I've changed the `POST | PUT /projects` endpoint to be handled by workhorse. Can someone with more experience and the big picture verify this particular part of the change? Is this something which makes sense? Is it implemented correctly? If this is the case and all looks good with the Projects API here, I'll also implement similar changes to the other `Avatarable` endpoints, like Groups and Users. Changelog: added MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92604 --- doc/api/projects.md | 13 ++++ lib/api/helpers/projects_helpers.rb | 2 +- lib/api/projects.rb | 2 + spec/requests/api/projects_spec.rb | 89 +++++++++++++++++++++++---- workhorse/internal/upstream/routes.go | 32 +++++----- workhorse/upload_test.go | 4 ++ 6 files changed, 114 insertions(+), 28 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 32b26f3272649a..56fa36a44f81d0 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -2248,6 +2248,19 @@ Returned object: } ``` +## Remove a project avatar + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92604) in GitLab 15.4. + +To remove a project avatar, use a blank value for the `avatar` attribute. + +Example request: + +```shell +curl --request PUT --header "PRIVATE-TOKEN: " \ + --data "avatar=" "https://gitlab.example.com/api/v4/projects/5" +``` + ## Share project with group Allow to share project with group. diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb index 628182ad1ab0bb..2d0e8ae4bfd60f 100644 --- a/lib/api/helpers/projects_helpers.rb +++ b/lib/api/helpers/projects_helpers.rb @@ -58,7 +58,7 @@ module ProjectsHelpers optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Deprecated: Use :topics instead' optional :topics, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The list of topics for a project' # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab/issues/14960 - optional :avatar, type: File, desc: 'Avatar image for project' # rubocop:disable Scalability/FileUploads + optional :avatar, type: ::API::Validations::Types::WorkhorseFile, desc: 'Avatar image for project' optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line' optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests' optional :suggestion_commit_message, type: String, desc: 'The commit message used to apply merge request suggestions' diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f8dc3d45a5c8fa..8c58cc585d8a11 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -453,6 +453,8 @@ def add_import_params(params) filter_attributes_using_license!(attrs) verify_update_project_attrs!(user_project, attrs) + user_project.remove_avatar! if attrs.key?(:avatar) && attrs[:avatar].nil? + result = ::Projects::UpdateService.new(user_project, current_user, attrs).execute if result[:status] == :success diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index fcbd0cc784b43c..ac534b4e711748 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -48,6 +48,7 @@ RSpec.describe API::Projects do include ProjectForksHelper + include WorkhorseHelpers include StubRequests let_it_be(:user) { create(:user) } @@ -1349,7 +1350,12 @@ it 'uploads avatar for project a project' do project = attributes_for(:project, avatar: fixture_file_upload('spec/fixtures/banana_sample.gif', 'image/gif')) - post api('/projects', user), params: project + workhorse_form_with_file( + api('/projects', user), + method: :post, + file_key: :avatar, + params: project + ) project_id = json_response['id'] expect(json_response['avatar_url']).to eq("http://localhost/uploads/-/system/project/avatar/#{project_id}/banana_sample.gif") @@ -1925,8 +1931,6 @@ end describe "POST /projects/:id/uploads/authorize" do - include WorkhorseHelpers - let(:headers) { workhorse_internal_api_request_header.merge({ 'HTTP_GITLAB_WORKHORSE' => 1 }) } context 'with authorized user' do @@ -3584,18 +3588,77 @@ def failure_message(diff) end end - it 'updates avatar' do - project_param = { - avatar: fixture_file_upload('spec/fixtures/banana_sample.gif', - 'image/gif') - } + context 'with changes to the avatar' do + let_it_be(:avatar_file) { fixture_file_upload('spec/fixtures/banana_sample.gif', 'image/gif') } + let_it_be(:alternate_avatar_file) { fixture_file_upload('spec/fixtures/rails_sample.png', 'image/png') } + let_it_be(:project_with_avatar, reload: true) do + create(:project, + :private, + :repository, + name: 'project-with-avatar', + creator_id: user.id, + namespace: user.namespace, + avatar: avatar_file) + end - put api("/projects/#{project3.id}", user), params: project_param + it 'uploads avatar to project without an avatar' do + workhorse_form_with_file( + api("/projects/#{project3.id}", user), + method: :put, + file_key: :avatar, + params: { avatar: avatar_file } + ) - expect(response).to have_gitlab_http_status(:ok) - expect(json_response['avatar_url']).to eq('http://localhost/uploads/'\ - '-/system/project/avatar/'\ - "#{project3.id}/banana_sample.gif") + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['avatar_url']).to eq('http://localhost/uploads/'\ + '-/system/project/avatar/'\ + "#{project3.id}/banana_sample.gif") + end + end + + it 'uploads and changes avatar to project with an avatar' do + workhorse_form_with_file( + api("/projects/#{project_with_avatar.id}", user), + method: :put, + file_key: :avatar, + params: { avatar: alternate_avatar_file } + ) + + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['avatar_url']).to eq('http://localhost/uploads/'\ + '-/system/project/avatar/'\ + "#{project_with_avatar.id}/rails_sample.png") + end + end + + it 'uploads and changes avatar to project among other changes' do + workhorse_form_with_file( + api("/projects/#{project_with_avatar.id}", user), + method: :put, + file_key: :avatar, + params: { description: 'changed description', avatar: avatar_file } + ) + + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['description']).to eq('changed description') + expect(json_response['avatar_url']).to eq('http://localhost/uploads/'\ + '-/system/project/avatar/'\ + "#{project_with_avatar.id}/banana_sample.gif") + end + end + + it 'removes avatar from project with an avatar' do + put api("/projects/#{project_with_avatar.id}", user), params: { avatar: '' } + + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['avatar_url']).to be_nil + expect(project_with_avatar.reload.avatar_url).to be_nil + end + end end it 'updates auto_devops_deploy_strategy' do diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index c889f87ed96c31..c56089904eb330 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -51,7 +51,7 @@ const ( gitProjectPattern = `^/.+\.git/` geoGitProjectPattern = `^/[^-].+\.git/` // Prevent matching routes like /-/push_from_secondary projectPattern = `^/([^/]+/){1,}[^/]+/` - apiProjectPattern = apiPattern + `v4/projects/[^/]+/` // API: Projects can be encoded via group%2Fsubgroup%2Fproject + apiProjectPattern = apiPattern + `v4/projects/[^/]+` // API: Projects can be encoded via group%2Fsubgroup%2Fproject apiTopicPattern = apiPattern + `v4/topics` snippetUploadPattern = `^/uploads/personal_snippet` userUploadPattern = `^/uploads/user` @@ -269,37 +269,37 @@ func configureRoutes(u *upstream) { // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56731. // Maven Artifact Repository - u.route("PUT", apiProjectPattern+`packages/maven/`, requestBodyUploader), + u.route("PUT", apiProjectPattern+`/packages/maven/`, requestBodyUploader), // Conan Artifact Repository u.route("PUT", apiPattern+`v4/packages/conan/`, requestBodyUploader), - u.route("PUT", apiProjectPattern+`packages/conan/`, requestBodyUploader), + u.route("PUT", apiProjectPattern+`/packages/conan/`, requestBodyUploader), // Generic Packages Repository - u.route("PUT", apiProjectPattern+`packages/generic/`, requestBodyUploader), + u.route("PUT", apiProjectPattern+`/packages/generic/`, requestBodyUploader), // NuGet Artifact Repository - u.route("PUT", apiProjectPattern+`packages/nuget/`, mimeMultipartUploader), + u.route("PUT", apiProjectPattern+`/packages/nuget/`, mimeMultipartUploader), // PyPI Artifact Repository - u.route("POST", apiProjectPattern+`packages/pypi`, mimeMultipartUploader), + u.route("POST", apiProjectPattern+`/packages/pypi`, mimeMultipartUploader), // Debian Artifact Repository - u.route("PUT", apiProjectPattern+`packages/debian/`, requestBodyUploader), + u.route("PUT", apiProjectPattern+`/packages/debian/`, requestBodyUploader), // Gem Artifact Repository - u.route("POST", apiProjectPattern+`packages/rubygems/`, requestBodyUploader), + u.route("POST", apiProjectPattern+`/packages/rubygems/`, requestBodyUploader), // Terraform Module Package Repository - u.route("PUT", apiProjectPattern+`packages/terraform/modules/`, requestBodyUploader), + u.route("PUT", apiProjectPattern+`/packages/terraform/modules/`, requestBodyUploader), // Helm Artifact Repository - u.route("POST", apiProjectPattern+`packages/helm/api/[^/]+/charts\z`, mimeMultipartUploader), + u.route("POST", apiProjectPattern+`/packages/helm/api/[^/]+/charts\z`, mimeMultipartUploader), // We are porting API to disk acceleration // we need to declare each routes until we have fixed all the routes on the rails codebase. // Overall status can be seen at https://gitlab.com/groups/gitlab-org/-/epics/1802#current-status - u.route("POST", apiProjectPattern+`wikis/attachments\z`, tempfileMultipartProxy), + u.route("POST", apiProjectPattern+`/wikis/attachments\z`, tempfileMultipartProxy), u.route("POST", apiPattern+`graphql\z`, tempfileMultipartProxy), u.route("POST", apiTopicPattern, tempfileMultipartProxy), u.route("PUT", apiTopicPattern, tempfileMultipartProxy), @@ -312,16 +312,20 @@ func configureRoutes(u *upstream) { u.route("POST", importPattern+`gitlab_group`, mimeMultipartUploader), // Issuable Metric image upload - u.route("POST", apiProjectPattern+`issues/[0-9]+/metric_images\z`, mimeMultipartUploader), + u.route("POST", apiProjectPattern+`/issues/[0-9]+/metric_images\z`, mimeMultipartUploader), // Alert Metric image upload - u.route("POST", apiProjectPattern+`alert_management_alerts/[0-9]+/metric_images\z`, mimeMultipartUploader), + u.route("POST", apiProjectPattern+`/alert_management_alerts/[0-9]+/metric_images\z`, mimeMultipartUploader), // Requirements Import via UI upload acceleration u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, mimeMultipartUploader), // Uploads via API - u.route("POST", apiProjectPattern+`uploads\z`, mimeMultipartUploader), + u.route("POST", apiProjectPattern+`/uploads\z`, mimeMultipartUploader), + + // Project Avatar + u.route("POST", apiPattern+`v4/projects\z`, tempfileMultipartProxy), + u.route("PUT", apiProjectPattern+`\z`, tempfileMultipartProxy), // Explicitly proxy API requests u.route("", apiPattern, proxy), diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index 9947059770fbf8..970af3ca5ce461 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -122,6 +122,10 @@ func TestAcceleratedUpload(t *testing.T) { {"POST", `/example`, false}, {"POST", `/uploads/personal_snippet`, true}, {"POST", `/uploads/user`, true}, + {"POST", `/api/v4/projects`, false}, + {"PUT", `/api/v4/projects/group%2Fproject`, false}, + {"PUT", `/api/v4/projects/group%2Fsubgroup%2Fproject`, false}, + {"PUT", `/api/v4/projects/39`, false}, {"POST", `/api/v4/projects/1/uploads`, true}, {"POST", `/api/v4/projects/group%2Fproject/uploads`, true}, {"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/uploads`, true}, -- GitLab