From 73540ffe6b03cf27dd7738ebd157573488f376cf Mon Sep 17 00:00:00 2001
From: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Wed, 7 Jun 2017 14:09:25 -0400
Subject: [PATCH] Clean up for api/base controller (#3629)
* Move ApiController to Api/BaseController
* API controllers inherit from Api::BaseController
* Add coverage for various error cases in api/base controller
---
.../api/activitypub/activities_controller.rb | 2 +-
.../api/activitypub/notes_controller.rb | 2 +-
.../api/activitypub/outbox_controller.rb | 2 +-
.../base_controller.rb} | 2 +-
app/controllers/api/oembed_controller.rb | 2 +-
app/controllers/api/push_controller.rb | 2 +-
app/controllers/api/salmon_controller.rb | 2 +-
.../api/subscriptions_controller.rb | 2 +-
.../api/v1/accounts/credentials_controller.rb | 2 +-
.../accounts/follower_accounts_controller.rb | 2 +-
.../accounts/following_accounts_controller.rb | 2 +-
.../v1/accounts/relationships_controller.rb | 2 +-
.../api/v1/accounts/search_controller.rb | 2 +-
.../api/v1/accounts/statuses_controller.rb | 2 +-
app/controllers/api/v1/accounts_controller.rb | 2 +-
app/controllers/api/v1/apps_controller.rb | 2 +-
app/controllers/api/v1/blocks_controller.rb | 2 +-
.../api/v1/domain_blocks_controller.rb | 2 +-
.../api/v1/favourites_controller.rb | 2 +-
.../api/v1/follow_requests_controller.rb | 2 +-
app/controllers/api/v1/follows_controller.rb | 2 +-
.../api/v1/instances_controller.rb | 2 +-
app/controllers/api/v1/media_controller.rb | 2 +-
app/controllers/api/v1/mutes_controller.rb | 2 +-
.../api/v1/notifications_controller.rb | 2 +-
app/controllers/api/v1/reports_controller.rb | 2 +-
app/controllers/api/v1/search_controller.rb | 2 +-
app/controllers/api/v1/statuses_controller.rb | 2 +-
.../api/v1/streaming_controller.rb | 2 +-
.../api/v1/timelines/home_controller.rb | 2 +-
.../api/v1/timelines/public_controller.rb | 2 +-
.../api/v1/timelines/tag_controller.rb | 2 +-
.../api/web/settings_controller.rb | 2 +-
spec/controllers/api/base_controller_spec.rb | 54 +++++++++++++++++++
spec/controllers/api_controller_spec.rb | 21 --------
35 files changed, 87 insertions(+), 54 deletions(-)
rename app/controllers/{api_controller.rb => api/base_controller.rb} (98%)
create mode 100644 spec/controllers/api/base_controller_spec.rb
delete mode 100644 spec/controllers/api_controller_spec.rb
diff --git a/app/controllers/api/activitypub/activities_controller.rb b/app/controllers/api/activitypub/activities_controller.rb
index 025ab960e3..740c8589a7 100644
--- a/app/controllers/api/activitypub/activities_controller.rb
+++ b/app/controllers/api/activitypub/activities_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::Activitypub::ActivitiesController < ApiController
+class Api::Activitypub::ActivitiesController < Api::BaseController
include Authorization
# before_action :set_follow, only: [:show_follow]
diff --git a/app/controllers/api/activitypub/notes_controller.rb b/app/controllers/api/activitypub/notes_controller.rb
index ff9383413b..783c1c4edd 100644
--- a/app/controllers/api/activitypub/notes_controller.rb
+++ b/app/controllers/api/activitypub/notes_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::Activitypub::NotesController < ApiController
+class Api::Activitypub::NotesController < Api::BaseController
include Authorization
before_action :set_status
diff --git a/app/controllers/api/activitypub/outbox_controller.rb b/app/controllers/api/activitypub/outbox_controller.rb
index 7b6cbdd38b..0738d7dee5 100644
--- a/app/controllers/api/activitypub/outbox_controller.rb
+++ b/app/controllers/api/activitypub/outbox_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::Activitypub::OutboxController < ApiController
+class Api::Activitypub::OutboxController < Api::BaseController
before_action :set_account
respond_to :activitystreams2
diff --git a/app/controllers/api_controller.rb b/app/controllers/api/base_controller.rb
similarity index 98%
rename from app/controllers/api_controller.rb
rename to app/controllers/api/base_controller.rb
index 42b85865ed..c1b2ec3cf5 100644
--- a/app/controllers/api_controller.rb
+++ b/app/controllers/api/base_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class ApiController < ApplicationController
+class Api::BaseController < ApplicationController
DEFAULT_STATUSES_LIMIT = 20
DEFAULT_ACCOUNTS_LIMIT = 40
diff --git a/app/controllers/api/oembed_controller.rb b/app/controllers/api/oembed_controller.rb
index 5761883531..6e3e34d964 100644
--- a/app/controllers/api/oembed_controller.rb
+++ b/app/controllers/api/oembed_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::OEmbedController < ApiController
+class Api::OEmbedController < Api::BaseController
respond_to :json
def show
diff --git a/app/controllers/api/push_controller.rb b/app/controllers/api/push_controller.rb
index 75a1f757b4..951867140c 100644
--- a/app/controllers/api/push_controller.rb
+++ b/app/controllers/api/push_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::PushController < ApiController
+class Api::PushController < Api::BaseController
def update
response, status = process_push_request
render plain: response, status: status
diff --git a/app/controllers/api/salmon_controller.rb b/app/controllers/api/salmon_controller.rb
index f611b48a09..e9e700b18d 100644
--- a/app/controllers/api/salmon_controller.rb
+++ b/app/controllers/api/salmon_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::SalmonController < ApiController
+class Api::SalmonController < Api::BaseController
before_action :set_account
respond_to :txt
diff --git a/app/controllers/api/subscriptions_controller.rb b/app/controllers/api/subscriptions_controller.rb
index dd2f42aab7..d3ea986761 100644
--- a/app/controllers/api/subscriptions_controller.rb
+++ b/app/controllers/api/subscriptions_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::SubscriptionsController < ApiController
+class Api::SubscriptionsController < Api::BaseController
before_action :set_account
respond_to :txt
diff --git a/app/controllers/api/v1/accounts/credentials_controller.rb b/app/controllers/api/v1/accounts/credentials_controller.rb
index 8f2ded29e3..1cf52ff10c 100644
--- a/app/controllers/api/v1/accounts/credentials_controller.rb
+++ b/app/controllers/api/v1/accounts/credentials_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::CredentialsController < ApiController
+class Api::V1::Accounts::CredentialsController < Api::BaseController
before_action -> { doorkeeper_authorize! :write }, only: [:update]
before_action :require_user!
diff --git a/app/controllers/api/v1/accounts/follower_accounts_controller.rb b/app/controllers/api/v1/accounts/follower_accounts_controller.rb
index 3e9da29e34..81aae56d3f 100644
--- a/app/controllers/api/v1/accounts/follower_accounts_controller.rb
+++ b/app/controllers/api/v1/accounts/follower_accounts_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::FollowerAccountsController < ApiController
+class Api::V1::Accounts::FollowerAccountsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :set_account
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/accounts/following_accounts_controller.rb b/app/controllers/api/v1/accounts/following_accounts_controller.rb
index 732961aac1..63c6d54b29 100644
--- a/app/controllers/api/v1/accounts/following_accounts_controller.rb
+++ b/app/controllers/api/v1/accounts/following_accounts_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::FollowingAccountsController < ApiController
+class Api::V1::Accounts::FollowingAccountsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :set_account
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/accounts/relationships_controller.rb b/app/controllers/api/v1/accounts/relationships_controller.rb
index d1a4f178b6..cb923ab917 100644
--- a/app/controllers/api/v1/accounts/relationships_controller.rb
+++ b/app/controllers/api/v1/accounts/relationships_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::RelationshipsController < ApiController
+class Api::V1::Accounts::RelationshipsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :require_user!
diff --git a/app/controllers/api/v1/accounts/search_controller.rb b/app/controllers/api/v1/accounts/search_controller.rb
index 6d4c6e4cf9..c4a8f97f24 100644
--- a/app/controllers/api/v1/accounts/search_controller.rb
+++ b/app/controllers/api/v1/accounts/search_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::SearchController < ApiController
+class Api::V1::Accounts::SearchController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :require_user!
diff --git a/app/controllers/api/v1/accounts/statuses_controller.rb b/app/controllers/api/v1/accounts/statuses_controller.rb
index 1e0d2a740b..504ed8c07d 100644
--- a/app/controllers/api/v1/accounts/statuses_controller.rb
+++ b/app/controllers/api/v1/accounts/statuses_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Accounts::StatusesController < ApiController
+class Api::V1::Accounts::StatusesController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :set_account
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb
index 3b23e996d6..8fc0dd36f5 100644
--- a/app/controllers/api/v1/accounts_controller.rb
+++ b/app/controllers/api/v1/accounts_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::AccountsController < ApiController
+class Api::V1::AccountsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }, except: [:follow, :unfollow, :block, :unblock, :mute, :unmute]
before_action -> { doorkeeper_authorize! :follow }, only: [:follow, :unfollow, :block, :unblock, :mute, :unmute]
before_action :require_user!, except: [:show]
diff --git a/app/controllers/api/v1/apps_controller.rb b/app/controllers/api/v1/apps_controller.rb
index 54f8d40b20..98e9089489 100644
--- a/app/controllers/api/v1/apps_controller.rb
+++ b/app/controllers/api/v1/apps_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::AppsController < ApiController
+class Api::V1::AppsController < Api::BaseController
respond_to :json
def create
diff --git a/app/controllers/api/v1/blocks_controller.rb b/app/controllers/api/v1/blocks_controller.rb
index d15cb439cf..1702953cf7 100644
--- a/app/controllers/api/v1/blocks_controller.rb
+++ b/app/controllers/api/v1/blocks_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::BlocksController < ApiController
+class Api::V1::BlocksController < Api::BaseController
before_action -> { doorkeeper_authorize! :follow }
before_action :require_user!
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/domain_blocks_controller.rb b/app/controllers/api/v1/domain_blocks_controller.rb
index 772c046876..e93dc603bb 100644
--- a/app/controllers/api/v1/domain_blocks_controller.rb
+++ b/app/controllers/api/v1/domain_blocks_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::DomainBlocksController < ApiController
+class Api::V1::DomainBlocksController < Api::BaseController
BLOCK_LIMIT = 100
before_action -> { doorkeeper_authorize! :follow }
diff --git a/app/controllers/api/v1/favourites_controller.rb b/app/controllers/api/v1/favourites_controller.rb
index a74db92af0..fe0819a3f5 100644
--- a/app/controllers/api/v1/favourites_controller.rb
+++ b/app/controllers/api/v1/favourites_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::FavouritesController < ApiController
+class Api::V1::FavouritesController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :require_user!
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/follow_requests_controller.rb b/app/controllers/api/v1/follow_requests_controller.rb
index 8a8d40d771..eed22ef4fb 100644
--- a/app/controllers/api/v1/follow_requests_controller.rb
+++ b/app/controllers/api/v1/follow_requests_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::FollowRequestsController < ApiController
+class Api::V1::FollowRequestsController < Api::BaseController
before_action -> { doorkeeper_authorize! :follow }
before_action :require_user!
after_action :insert_pagination_headers, only: :index
diff --git a/app/controllers/api/v1/follows_controller.rb b/app/controllers/api/v1/follows_controller.rb
index 67d823398a..bcdb4e177a 100644
--- a/app/controllers/api/v1/follows_controller.rb
+++ b/app/controllers/api/v1/follows_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::FollowsController < ApiController
+class Api::V1::FollowsController < Api::BaseController
before_action -> { doorkeeper_authorize! :follow }
before_action :require_user!
diff --git a/app/controllers/api/v1/instances_controller.rb b/app/controllers/api/v1/instances_controller.rb
index 51d92838ac..ce2181879b 100644
--- a/app/controllers/api/v1/instances_controller.rb
+++ b/app/controllers/api/v1/instances_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::InstancesController < ApiController
+class Api::V1::InstancesController < Api::BaseController
respond_to :json
def show; end
diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb
index 3d7dcef420..25a3313195 100644
--- a/app/controllers/api/v1/media_controller.rb
+++ b/app/controllers/api/v1/media_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::MediaController < ApiController
+class Api::V1::MediaController < Api::BaseController
before_action -> { doorkeeper_authorize! :write }
before_action :require_user!
diff --git a/app/controllers/api/v1/mutes_controller.rb b/app/controllers/api/v1/mutes_controller.rb
index b9ac741765..2a353df039 100644
--- a/app/controllers/api/v1/mutes_controller.rb
+++ b/app/controllers/api/v1/mutes_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::MutesController < ApiController
+class Api::V1::MutesController < Api::BaseController
before_action -> { doorkeeper_authorize! :follow }
before_action :require_user!
after_action :insert_pagination_headers
diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb
index 1cd4ca40a4..20b28776db 100644
--- a/app/controllers/api/v1/notifications_controller.rb
+++ b/app/controllers/api/v1/notifications_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::NotificationsController < ApiController
+class Api::V1::NotificationsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }
before_action :require_user!
after_action :insert_pagination_headers, only: :index
diff --git a/app/controllers/api/v1/reports_controller.rb b/app/controllers/api/v1/reports_controller.rb
index e0f9ed2322..71df76e922 100644
--- a/app/controllers/api/v1/reports_controller.rb
+++ b/app/controllers/api/v1/reports_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::ReportsController < ApiController
+class Api::V1::ReportsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }, except: [:create]
before_action -> { doorkeeper_authorize! :write }, only: [:create]
before_action :require_user!
diff --git a/app/controllers/api/v1/search_controller.rb b/app/controllers/api/v1/search_controller.rb
index 1ee2589a01..8b832148c3 100644
--- a/app/controllers/api/v1/search_controller.rb
+++ b/app/controllers/api/v1/search_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::SearchController < ApiController
+class Api::V1::SearchController < Api::BaseController
RESULTS_LIMIT = 5
respond_to :json
diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb
index 7386d71582..53fb1619e7 100644
--- a/app/controllers/api/v1/statuses_controller.rb
+++ b/app/controllers/api/v1/statuses_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::StatusesController < ApiController
+class Api::V1::StatusesController < Api::BaseController
include Authorization
before_action :authorize_if_got_token, except: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite, :mute, :unmute]
diff --git a/app/controllers/api/v1/streaming_controller.rb b/app/controllers/api/v1/streaming_controller.rb
index 3779514723..66b812e761 100644
--- a/app/controllers/api/v1/streaming_controller.rb
+++ b/app/controllers/api/v1/streaming_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::StreamingController < ApiController
+class Api::V1::StreamingController < Api::BaseController
respond_to :json
def index
diff --git a/app/controllers/api/v1/timelines/home_controller.rb b/app/controllers/api/v1/timelines/home_controller.rb
index 29e570fa5e..511d2f65da 100644
--- a/app/controllers/api/v1/timelines/home_controller.rb
+++ b/app/controllers/api/v1/timelines/home_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Timelines::HomeController < ApiController
+class Api::V1::Timelines::HomeController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }, only: [:show]
before_action :require_user!, only: [:show]
after_action :insert_pagination_headers, unless: -> { @statuses.empty? }
diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb
index cd3663d5f4..305451cc7b 100644
--- a/app/controllers/api/v1/timelines/public_controller.rb
+++ b/app/controllers/api/v1/timelines/public_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Timelines::PublicController < ApiController
+class Api::V1::Timelines::PublicController < Api::BaseController
after_action :insert_pagination_headers, unless: -> { @statuses.empty? }
respond_to :json
diff --git a/app/controllers/api/v1/timelines/tag_controller.rb b/app/controllers/api/v1/timelines/tag_controller.rb
index 0481f5debe..50afca7c72 100644
--- a/app/controllers/api/v1/timelines/tag_controller.rb
+++ b/app/controllers/api/v1/timelines/tag_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::V1::Timelines::TagController < ApiController
+class Api::V1::Timelines::TagController < Api::BaseController
before_action :load_tag
after_action :insert_pagination_headers, unless: -> { @statuses.empty? }
diff --git a/app/controllers/api/web/settings_controller.rb b/app/controllers/api/web/settings_controller.rb
index 7cceb0dfc7..f6739d5062 100644
--- a/app/controllers/api/web/settings_controller.rb
+++ b/app/controllers/api/web/settings_controller.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class Api::Web::SettingsController < ApiController
+class Api::Web::SettingsController < Api::BaseController
respond_to :json
before_action :require_user!
diff --git a/spec/controllers/api/base_controller_spec.rb b/spec/controllers/api/base_controller_spec.rb
new file mode 100644
index 0000000000..7d5e0116c0
--- /dev/null
+++ b/spec/controllers/api/base_controller_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+class FakeService; end
+
+describe Api::BaseController do
+ controller do
+ def success
+ head 200
+ end
+
+ def error
+ FakeService.new
+ end
+ end
+
+ describe 'Forgery protection' do
+ before do
+ routes.draw { post 'success' => 'api/base#success' }
+ end
+
+ it 'does not protect from forgery' do
+ ActionController::Base.allow_forgery_protection = true
+ post 'success'
+ expect(response).to have_http_status(:success)
+ end
+ end
+
+ describe 'Error handling' do
+ ERRORS_WITH_CODES = {
+ ActiveRecord::RecordInvalid => 422,
+ Mastodon::ValidationError => 422,
+ ActiveRecord::RecordNotFound => 404,
+ Goldfinger::Error => 422,
+ HTTP::Error => 503,
+ OpenSSL::SSL::SSLError => 503,
+ Mastodon::NotPermittedError => 403,
+ }
+
+ before do
+ routes.draw { get 'error' => 'api/base#error' }
+ end
+
+ ERRORS_WITH_CODES.each do |error, code|
+ it "Handles error class of #{error}" do
+ expect(FakeService).to receive(:new).and_raise(error)
+
+ get 'error'
+ expect(response).to have_http_status(code)
+ end
+ end
+ end
+end
diff --git a/spec/controllers/api_controller_spec.rb b/spec/controllers/api_controller_spec.rb
deleted file mode 100644
index 44be4276ac..0000000000
--- a/spec/controllers/api_controller_spec.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-describe ApiController, type: :controller do
- controller do
- def success
- head 200
- end
- end
-
- before do
- routes.draw { post 'success' => 'api#success' }
- end
-
- it 'does not protect from forgery' do
- ActionController::Base.allow_forgery_protection = true
- post 'success'
- expect(response).to have_http_status(:success)
- end
-end
--
GitLab