Merge branch 'api-remove-snippets-expires-at' into 'master'

Remove deprecated 'expires_at' from project snippets API

Closes #14156

See merge request !8723

changelogs/unreleased/api-remove-snippets-expires-at.yml

+---
+title: 'API: Remove deprecated ''expires_at'' from project snippets'
+merge_request: 8723
+author: Robert Schilling

doc/api/project_snippets.md

@@ -51,7 +51,6 @@ Parameters:
     "state": "active",
     "created_at": "2012-05-23T08:00:58Z"
   },
-  "expires_at": null,
   "updated_at": "2012-06-28T10:52:04Z",
   "created_at": "2012-06-28T10:52:04Z",
   "web_url": "http://example.com/example/example/snippets/1"

doc/api/v3_to_v4.md

@@ -10,4 +10,4 @@ changes are in V4:
 - `iid` filter has been removed from `projects/:id/issues`
 - `projects/:id/merge_requests?iid[]=x&iid[]=y` array filter has been renamed to `iids`
 - Endpoints under `projects/merge_request/:id` have been removed (use: `projects/merge_requests/:id`)
-
+- Project snippets do not return deprecated field `expires_at`

lib/api/api.rb

@@ -8,6 +8,7 @@ module API
       mount ::API::V3::Issues
       mount ::API::V3::MergeRequests
       mount ::API::V3::Projects
+      mount ::API::V3::ProjectSnippets
     end
 
     before { allow_access_with_scope :api }

lib/api/entities.rb

@@ -213,9 +213,6 @@ module API
       expose :author, using: Entities::UserBasic
       expose :updated_at, :created_at
 
-      # TODO (rspeicher): Deprecated; remove in 9.0
-      expose(:expires_at) { |snippet| nil }
-
       expose :web_url do |snippet, options|
         Gitlab::UrlBuilder.build(snippet)
       end

lib/api/v3/entities.rb

+module API
+  module V3
+    module Entities
+      class ProjectSnippet < Grape::Entity
+        expose :id, :title, :file_name
+        expose :author, using: ::API::Entities::UserBasic
+        expose :updated_at, :created_at
+        expose(:expires_at) { |snippet| nil }
+
+        expose :web_url do |snippet, options|
+          Gitlab::UrlBuilder.build(snippet)
+        end
+      end
+    end
+  end
+end

lib/api/v3/issues.rb

@@ -50,7 +50,7 @@ module API
 
       resource :issues do
         desc "Get currently authenticated user's issues" do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           optional :state, type: String, values: %w[opened closed all], default: 'all',
@@ -60,7 +60,7 @@ module API
         get do
           issues = find_issues(scope: 'authored')
 
-          present paginate(issues), with: Entities::Issue, current_user: current_user
+          present paginate(issues), with: ::API::Entities::Issue, current_user: current_user
         end
       end
 
@@ -69,7 +69,7 @@ module API
       end
       resource :groups do
         desc 'Get a list of group issues' do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           optional :state, type: String, values: %w[opened closed all], default: 'opened',
@@ -81,7 +81,7 @@ module API
 
           issues = find_issues(group_id: group.id, state: params[:state] || 'opened', match_all_labels: true)
 
-          present paginate(issues), with: Entities::Issue, current_user: current_user
+          present paginate(issues), with: ::API::Entities::Issue, current_user: current_user
         end
       end
 
@@ -93,7 +93,7 @@ module API
 
         desc 'Get a list of project issues' do
           detail 'iid filter is deprecated have been removed on V4'
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           optional :state, type: String, values: %w[opened closed all], default: 'all',
@@ -106,22 +106,22 @@ module API
 
           issues = find_issues(project_id: project.id)
 
-          present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project
+          present paginate(issues), with: ::API::Entities::Issue, current_user: current_user, project: user_project
         end
 
         desc 'Get a single project issue' do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           requires :issue_id, type: Integer, desc: 'The ID of a project issue'
         end
         get ":id/issues/:issue_id" do
           issue = find_project_issue(params[:issue_id])
-          present issue, with: Entities::Issue, current_user: current_user, project: user_project
+          present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
         end
 
         desc 'Create a new project issue' do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           requires :title, type: String, desc: 'The title of an issue'
@@ -153,14 +153,14 @@ module API
           end
 
           if issue.valid?
-            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+            present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
           else
             render_validation_error!(issue)
           end
         end
 
         desc 'Update an existing issue' do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           requires :issue_id, type: Integer, desc: 'The ID of a project issue'
@@ -186,14 +186,14 @@ module API
                                               declared_params(include_missing: false)).execute(issue)
 
           if issue.valid?
-            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+            present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
           else
             render_validation_error!(issue)
           end
         end
 
         desc 'Move an existing issue' do
-          success Entities::Issue
+          success ::API::Entities::Issue
         end
         params do
           requires :issue_id, type: Integer, desc: 'The ID of a project issue'
@@ -208,7 +208,7 @@ module API
 
           begin
             issue = ::Issues::MoveService.new(user_project, current_user).execute(issue, new_project)
-            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+            present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
           rescue ::Issues::MoveService::MoveError => error
             render_api_error!(error.message, 400)
           end

lib/api/v3/merge_requests.rb

@@ -39,7 +39,7 @@ module API
 
         desc 'List merge requests' do
           detail 'iid filter is deprecated have been removed on V4'
-          success Entities::MergeRequest
+          success ::API::Entities::MergeRequest
         end
         params do
           optional :state, type: String, values: %w[opened closed merged all], default: 'all',
@@ -66,11 +66,11 @@ module API
             end
 
           merge_requests = merge_requests.reorder(params[:order_by] => params[:sort])
-          present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user, project: user_project
+          present paginate(merge_requests), with: ::API::Entities::MergeRequest, current_user: current_user, project: user_project
         end
 
         desc 'Create a merge request' do
-          success Entities::MergeRequest
+          success ::API::Entities::MergeRequest
         end
         params do
           requires :title, type: String, desc: 'The title of the merge request'
@@ -89,7 +89,7 @@ module API
           merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
           if merge_request.valid?
-            present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
+            present merge_request, with: ::API::Entities::MergeRequest, current_user: current_user, project: user_project
           else
             handle_merge_request_errors! merge_request.errors
           end
@@ -114,34 +114,34 @@ module API
             if status == :deprecated
               detail DEPRECATION_MESSAGE
             end
-            success Entities::MergeRequest
+            success ::API::Entities::MergeRequest
           end
           get path do
             merge_request = find_merge_request_with_access(params[:merge_request_id])
 
-            present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
+            present merge_request, with: ::API::Entities::MergeRequest, current_user: current_user, project: user_project
           end
 
           desc 'Get the commits of a merge request' do
-            success Entities::RepoCommit
+            success ::API::Entities::RepoCommit
           end
           get "#{path}/commits" do
             merge_request = find_merge_request_with_access(params[:merge_request_id])
 
-            present merge_request.commits, with: Entities::RepoCommit
+            present merge_request.commits, with: ::API::Entities::RepoCommit
           end
 
           desc 'Show the merge request changes' do
-            success Entities::MergeRequestChanges
+            success ::API::Entities::MergeRequestChanges
           end
           get "#{path}/changes" do
             merge_request = find_merge_request_with_access(params[:merge_request_id])
 
-            present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
+            present merge_request, with: ::API::Entities::MergeRequestChanges, current_user: current_user
           end
 
           desc 'Update a merge request' do
-            success Entities::MergeRequest
+            success ::API::Entities::MergeRequest
           end
           params do
             optional :title, type: String, allow_blank: false, desc: 'The title of the merge request'
@@ -162,14 +162,14 @@ module API
             merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
 
             if merge_request.valid?
-              present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
+              present merge_request, with: ::API::Entities::MergeRequest, current_user: current_user, project: user_project
             else
               handle_merge_request_errors! merge_request.errors
             end
           end
 
           desc 'Merge a merge request' do
-            success Entities::MergeRequest
+            success ::API::Entities::MergeRequest
           end
           params do
             optional :merge_commit_message, type: String, desc: 'Custom merge commit message'
@@ -209,11 +209,11 @@ module API
                 .execute(merge_request)
             end
 
-            present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
+            present merge_request, with: ::API::Entities::MergeRequest, current_user: current_user, project: user_project
           end
 
           desc 'Cancel merge if "Merge When Pipeline Succeeds" is enabled' do
-            success Entities::MergeRequest
+            success ::API::Entities::MergeRequest
           end
           post "#{path}/cancel_merge_when_build_succeeds" do
             merge_request = find_project_merge_request(params[:merge_request_id])
@@ -227,19 +227,19 @@ module API
 
           desc 'Get the comments of a merge request' do
             detail 'Duplicate. DEPRECATED and HAS BEEN REMOVED in V4'
-            success Entities::MRNote
+            success ::API::Entities::MRNote
           end
           params do
             use :pagination
           end
           get "#{path}/comments" do
             merge_request = find_merge_request_with_access(params[:merge_request_id])
-            present paginate(merge_request.notes.fresh), with: Entities::MRNote
+            present paginate(merge_request.notes.fresh), with: ::API::Entities::MRNote
           end
 
           desc 'Post a comment to a merge request' do
             detail 'Duplicate. DEPRECATED and HAS BEEN REMOVED in V4'
-            success Entities::MRNote
+            success ::API::Entities::MRNote
           end
           params do
             requires :note, type: String, desc: 'The text of the comment'
@@ -256,14 +256,14 @@ module API
             note = ::Notes::CreateService.new(user_project, current_user, opts).execute
 
             if note.save
-              present note, with: Entities::MRNote
+              present note, with: ::API::Entities::MRNote
             else
               render_api_error!("Failed to save note #{note.errors.messages}", 400)
             end
           end
 
           desc 'List issues that will be closed on merge' do
-            success Entities::MRNote
+            success ::API::Entities::MRNote
           end
           params do
             use :pagination

lib/api/v3/project_snippets.rb

+module API
+  module V3
+    class ProjectSnippets < Grape::API
+      include PaginationParams
+
+      before { authenticate! }
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+      end
+      resource :projects do
+        helpers do
+          def handle_project_member_errors(errors)
+            if errors[:project_access].any?
+              error!(errors[:project_access], 422)
+            end
+            not_found!
+          end
+
+          def snippets_for_current_user
+            finder_params = { filter: :by_project, project: user_project }
+            SnippetsFinder.new.execute(current_user, finder_params)
+          end
+        end
+
+        desc 'Get all project snippets' do
+          success ::API::V3::Entities::ProjectSnippet
+        end
+        params do
+          use :pagination
+        end
+        get ":id/snippets" do
+          present paginate(snippets_for_current_user), with: ::API::V3::Entities::ProjectSnippet
+        end
+
+        desc 'Get a single project snippet' do
+          success ::API::V3::Entities::ProjectSnippet
+        end
+        params do
+          requires :snippet_id, type: Integer, desc: 'The ID of a project snippet'
+        end
+        get ":id/snippets/:snippet_id" do
+          snippet = snippets_for_current_user.find(params[:snippet_id])
+          present snippet, with: ::API::V3::Entities::ProjectSnippet
+        end
+
+        desc 'Create a new project snippet' do
+          success ::API::V3::Entities::ProjectSnippet
+        end
+        params do
+          requires :title, type: String, desc: 'The title of the snippet'
+          requires :file_name, type: String, desc: 'The file name of the snippet'
+          requires :code, type: String, desc: 'The content of the snippet'
+          requires :visibility_level, type: Integer,
+                                      values: [Gitlab::VisibilityLevel::PRIVATE,
+                                               Gitlab::VisibilityLevel::INTERNAL,
+                                               Gitlab::VisibilityLevel::PUBLIC],
+                                      desc: 'The visibility level of the snippet'
+        end
+        post ":id/snippets" do
+          authorize! :create_project_snippet, user_project
+          snippet_params = declared_params.merge(request: request, api: true)
+          snippet_params[:content] = snippet_params.delete(:code)
+
+          snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute
+
+          if snippet.persisted?
+            present snippet, with: ::API::V3::Entities::ProjectSnippet
+          else
+            render_validation_error!(snippet)
+          end
+        end
+
+        desc 'Update an existing project snippet' do
+          success ::API::V3::Entities::ProjectSnippet
+        end
+        params do
+          requires :snippet_id, type: Integer, desc: 'The ID of a project snippet'
+          optional :title, type: String, desc: 'The title of the snippet'
+          optional :file_name, type: String, desc: 'The file name of the snippet'
+          optional :code, type: String, desc: 'The content of the snippet'
+          optional :visibility_level, type: Integer,
+                                      values: [Gitlab::VisibilityLevel::PRIVATE,
+                                               Gitlab::VisibilityLevel::INTERNAL,
+                                               Gitlab::VisibilityLevel::PUBLIC],
+                                      desc: 'The visibility level of the snippet'
+          at_least_one_of :title, :file_name, :code, :visibility_level
+        end
+        put ":id/snippets/:snippet_id" do
+          snippet = snippets_for_current_user.find_by(id: params.delete(:snippet_id))
+          not_found!('Snippet') unless snippet
+
+          authorize! :update_project_snippet, snippet
+
+          snippet_params = declared_params(include_missing: false)
+          snippet_params[:content] = snippet_params.delete(:code) if snippet_params[:code].present?
+
+          UpdateSnippetService.new(user_project, current_user, snippet,
+                                   snippet_params).execute
+
+          if snippet.persisted?
+            present snippet, with: ::API::V3::Entities::ProjectSnippet
+          else
+            render_validation_error!(snippet)
+          end
+        end
+
+        desc 'Delete a project snippet'
+        params do
+          requires :snippet_id, type: Integer, desc: 'The ID of a project snippet'
+        end
+        delete ":id/snippets/:snippet_id" do
+          snippet = snippets_for_current_user.find_by(id: params[:snippet_id])
+          not_found!('Snippet') unless snippet
+
+          authorize! :admin_project_snippet, snippet
+          snippet.destroy
+        end
+
+        desc 'Get a raw project snippet'
+        params do
+          requires :snippet_id, type: Integer, desc: 'The ID of a project snippet'
+        end
+        get ":id/snippets/:snippet_id/raw" do
+          snippet = snippets_for_current_user.find_by(id: params[:snippet_id])
+          not_found!('Snippet') unless snippet
+
+          env['api.format'] = :txt
+          content_type 'text/plain'
+          present snippet.content
+        end
+      end
+    end
+  end
+end

spec/requests/api/project_snippets_spec.rb

@@ -7,18 +7,6 @@ describe API::ProjectSnippets, api: true do
   let(:user) { create(:user) }
   let(:admin) { create(:admin) }
 
-  describe 'GET /projects/:project_id/snippets/:id' do
-    # TODO (rspeicher): Deprecated; remove in 9.0
-    it 'always exposes expires_at as nil' do
-      snippet = create(:project_snippet, author: admin)
-
-      get api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin)
-
-      expect(json_response).to have_key('expires_at')
-      expect(json_response['expires_at']).to be_nil
-    end
-  end
-
   describe 'GET /projects/:project_id/snippets/' do
     let(:user) { create(:user) }
 

spec/requests/api/v3/project_snippets_spec.rb

+require 'rails_helper'
+
+describe API::ProjectSnippets, api: true do
+  include ApiHelpers
+
+  let(:project) { create(:empty_project, :public) }
+  let(:user) { create(:user) }
+  let(:admin) { create(:admin) }
+
+  describe 'GET /projects/:project_id/snippets/:id' do
+    # TODO (rspeicher): Deprecated; remove in 9.0
+    it 'always exposes expires_at as nil' do
+      snippet = create(:project_snippet, author: admin)
+
+      get v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin)
+
+      expect(json_response).to have_key('expires_at')
+      expect(json_response['expires_at']).to be_nil
+    end
+  end
+
+  describe 'GET /projects/:project_id/snippets/' do
+    let(:user) { create(:user) }
+
+    it 'returns all snippets available to team member' do
+      project.add_developer(user)
+      public_snippet = create(:project_snippet, :public, project: project)
+      internal_snippet = create(:project_snippet, :internal, project: project)
+      private_snippet = create(:project_snippet, :private, project: project)
+
+      get v3_api("/projects/#{project.id}/snippets/", user)
+
+      expect(response).to have_http_status(200)
+      expect(json_response.size).to eq(3)
+      expect(json_response.map{ |snippet| snippet['id']} ).to include(public_snippet.id, internal_snippet.id, private_snippet.id)
+      expect(json_response.last).to have_key('web_url')
+    end
+
+    it 'hides private snippets from regular user' do
+      create(:project_snippet, :private, project: project)
+
+      get v3_api("/projects/#{project.id}/snippets/", user)
+      expect(response).to have_http_status(200)
+      expect(json_response.size).to eq(0)
+    end
+  end
+
+  describe 'POST /projects/:project_id/snippets/' do
+    let(:params) do
+      {
+        title: 'Test Title',
+        file_name: 'test.rb',
+        code: 'puts "hello world"',
+        visibility_level: Snippet::PUBLIC
+      }
+    end
+
+    it 'creates a new snippet' do
+      post v3_api("/projects/#{project.id}/snippets/", admin), params
+
+      expect(response).to have_http_status(201)
+      snippet = ProjectSnippet.find(json_response['id'])
+      expect(snippet.content).to eq(params[:code])
+      expect(snippet.title).to eq(params[:title])
+      expect(snippet.file_name).to eq(params[:file_name])
+      expect(snippet.visibility_level).to eq(params[:visibility_level])
+    end
+
+    it 'returns 400 for missing parameters' do
+      params.delete(:title)
+
+      post v3_api("/projects/#{project.id}/snippets/", admin), params
+
+      expect(response).to have_http_status(400)
+    end
+
+    context 'when the snippet is spam' do
+      def create_snippet(project, snippet_params = {})
+        project.add_developer(user)
+
+        post v3_api("/projects/#{project.id}/snippets", user), params.merge(snippet_params)
+      end
+
+      before do
+        allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+      end
+
+      context 'when the project is private' do
+        let(:private_project) { create(:project_empty_repo, :private) }
+
+        context 'when the snippet is public' do
+          it 'creates the snippet' do
+            expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }.
+              to change { Snippet.count }.by(1)
+          end
+        end
+      end
+
+      context 'when the project is public' do
+        context 'when the snippet is private' do
+          it 'creates the snippet' do
+            expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
+              to change { Snippet.count }.by(1)
+          end
+        end
+
+        context 'when the snippet is public' do
+          it 'rejects the shippet' do
+            expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+              not_to change { Snippet.count }
+            expect(response).to have_http_status(400)
+          end
+
+          it 'creates a spam log' do
+            expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+              to change { SpamLog.count }.by(1)
+          end
+        end
+      end
+    end
+  end
+
+  describe 'PUT /projects/:project_id/snippets/:id/' do
+    let(:snippet) { create(:project_snippet, author: admin) }
+
+    it 'updates snippet' do
+      new_content = 'New content'
+
+      put v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), code: new_content
+
+      expect(response).to have_http_status(200)
+      snippet.reload
+      expect(snippet.content).to eq(new_content)
+    end
+
+    it 'returns 404 for invalid snippet id' do
+      put v3_api("/projects/#{snippet.project.id}/snippets/1234", admin), title: 'foo'
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 Snippet Not Found')
+    end
+
+    it 'returns 400 for missing parameters' do
+      put v3_api("/projects/#{project.id}/snippets/1234", admin)
+
+      expect(response).to have_http_status(400)
+    end
+  end
+
+  describe 'DELETE /projects/:project_id/snippets/:id/' do
+    let(:snippet) { create(:project_snippet, author: admin) }
+
+    it 'deletes snippet' do
+      admin = create(:admin)
+      snippet = create(:project_snippet, author: admin)
+
+      delete v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin)
+
+      expect(response).to have_http_status(200)
+    end
+
+    it 'returns 404 for invalid snippet id' do
+      delete v3_api("/projects/#{snippet.project.id}/snippets/1234", admin)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 Snippet Not Found')
+    end
+  end
+
+  describe 'GET /projects/:project_id/snippets/:id/raw' do
+    let(:snippet) { create(:project_snippet, author: admin) }
+
+    it 'returns raw text' do
+      get v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/raw", admin)
+
+      expect(response).to have_http_status(200)
+      expect(response.content_type).to eq 'text/plain'
+      expect(response.body).to eq(snippet.content)
+    end
+
+    it 'returns 404 for invalid snippet id' do
+      delete v3_api("/projects/#{snippet.project.id}/snippets/1234", admin)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 Snippet Not Found')
+    end
+  end
+end