From 9aecc0f48a046e0a05b8ca69511f8b72756fb431 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 8 Nov 2016 23:22:44 +0100 Subject: [PATCH] Move timelines API from statuses to its own controller, add a check for resources that require a user context vs those that don't (such as public timeline) /api/v1/statuses/public -> /api/v1/timelines/public /api/v1/statuses/home -> /api/v1/timelines/home /api/v1/statuses/mentions -> /api/v1/timelines/mentions /api/v1/statuses/tag/:tag -> /api/v1/timelines/tag/:tag --- .../components/actions/timelines.jsx | 4 +- app/controllers/api/v1/accounts_controller.rb | 3 +- app/controllers/api/v1/follows_controller.rb | 2 + app/controllers/api/v1/media_controller.rb | 2 + app/controllers/api/v1/statuses_controller.rb | 35 +------- .../api/v1/timelines_controller.rb | 37 +++++++++ app/controllers/api_controller.rb | 18 ++++- app/models/status.rb | 26 +++--- app/views/api/v1/timelines/index.rabl | 2 + config/routes.rb | 16 ++-- .../api/v1/statuses_controller_spec.rb | 32 -------- .../api/v1/timelines_controller_spec.rb | 80 +++++++++++++++++++ 12 files changed, 170 insertions(+), 87 deletions(-) create mode 100644 app/controllers/api/v1/timelines_controller.rb create mode 100644 app/views/api/v1/timelines/index.rabl create mode 100644 spec/controllers/api/v1/timelines_controller_spec.rb diff --git a/app/assets/javascripts/components/actions/timelines.jsx b/app/assets/javascripts/components/actions/timelines.jsx index 1dd77084893..0f23ca7fce5 100644 --- a/app/assets/javascripts/components/actions/timelines.jsx +++ b/app/assets/javascripts/components/actions/timelines.jsx @@ -73,7 +73,7 @@ export function refreshTimeline(timeline, replace = false, id = null) { path = `${path}/${id}` } - api(getState).get(`/api/v1/statuses/${path}${params}`).then(function (response) { + api(getState).get(`/api/v1/timelines/${path}${params}`).then(function (response) { dispatch(refreshTimelineSuccess(timeline, response.data, replace)); }).catch(function (error) { dispatch(refreshTimelineFail(timeline, error)); @@ -101,7 +101,7 @@ export function expandTimeline(timeline, id = null) { path = `${path}/${id}` } - api(getState).get(`/api/v1/statuses/${path}?max_id=${lastId}`).then(response => { + api(getState).get(`/api/v1/timelines/${path}?max_id=${lastId}`).then(response => { dispatch(expandTimelineSuccess(timeline, response.data)); }).catch(error => { dispatch(expandTimelineFail(timeline, error)); diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index bb06ddac90b..4140439a84b 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,8 +1,9 @@ class Api::V1::AccountsController < ApiController before_action -> { doorkeeper_authorize! :read }, except: [:follow, :unfollow, :block, :unblock] before_action -> { doorkeeper_authorize! :follow }, only: [:follow, :unfollow, :block, :unblock] - + before_action :require_user!, except: [:show, :following, :followers, :statuses] before_action :set_account, except: [:verify_credentials, :suggestions] + respond_to :json def show diff --git a/app/controllers/api/v1/follows_controller.rb b/app/controllers/api/v1/follows_controller.rb index 526316531a8..80a5aedf269 100644 --- a/app/controllers/api/v1/follows_controller.rb +++ b/app/controllers/api/v1/follows_controller.rb @@ -1,5 +1,7 @@ class Api::V1::FollowsController < ApiController before_action -> { doorkeeper_authorize! :follow } + before_action :require_user! + respond_to :json def create diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index dffc797fe79..ab216f9c94e 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -1,5 +1,7 @@ class Api::V1::MediaController < ApiController before_action -> { doorkeeper_authorize! :write } + before_action :require_user! + respond_to :json def create diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 0a823e3e665..51a044a6cc4 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -1,8 +1,8 @@ class Api::V1::StatusesController < ApiController before_action -> { doorkeeper_authorize! :read }, except: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite] before_action -> { doorkeeper_authorize! :write }, only: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite] - - before_action :set_status, only: [:show, :context, :reblogged_by, :favourited_by] + before_action :require_user!, except: [:show, :context, :reblogged_by, :favourited_by] + before_action :set_status, only: [:show, :context, :reblogged_by, :favourited_by] respond_to :json @@ -56,37 +56,6 @@ class Api::V1::StatusesController < ApiController render action: :show end - def home - @statuses = Feed.new(:home, current_user.account).get(20, params[:max_id], params[:since_id]).to_a - set_maps(@statuses) - render action: :index - end - - def mentions - @statuses = Feed.new(:mentions, current_user.account).get(20, params[:max_id], params[:since_id]).to_a - set_maps(@statuses) - render action: :index - end - - def public - @statuses = Status.as_public_timeline(current_user.account).paginate_by_max_id(20, params[:max_id], params[:since_id]).to_a - set_maps(@statuses) - render action: :index - end - - def tag - @tag = Tag.find_by(name: params[:id].downcase) - - if @tag.nil? - @statuses = [] - else - @statuses = Status.as_tag_timeline(@tag, current_user.account).paginate_by_max_id(20, params[:max_id], params[:since_id]).to_a - set_maps(@statuses) - end - - render action: :index - end - private def set_status diff --git a/app/controllers/api/v1/timelines_controller.rb b/app/controllers/api/v1/timelines_controller.rb new file mode 100644 index 00000000000..e5176dd4bee --- /dev/null +++ b/app/controllers/api/v1/timelines_controller.rb @@ -0,0 +1,37 @@ +class Api::V1::TimelinesController < ApiController + before_action -> { doorkeeper_authorize! :read } + before_action :require_user!, only: [:home, :mentions] + + respond_to :json + + def home + @statuses = Feed.new(:home, current_account).get(20, params[:max_id], params[:since_id]).to_a + set_maps(@statuses) + render action: :index + end + + def mentions + @statuses = Feed.new(:mentions, current_account).get(20, params[:max_id], params[:since_id]).to_a + set_maps(@statuses) + render action: :index + end + + def public + @statuses = Status.as_public_timeline(current_account).paginate_by_max_id(20, params[:max_id], params[:since_id]).to_a + set_maps(@statuses) + render action: :index + end + + def tag + @tag = Tag.find_by(name: params[:id].downcase) + + if @tag.nil? + @statuses = [] + else + @statuses = Status.as_tag_timeline(@tag, current_account).paginate_by_max_id(20, params[:max_id], params[:since_id]).to_a + set_maps(@statuses) + end + + render action: :index + end +end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 273aaff857e..db4035a96e2 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -60,6 +60,14 @@ class ApiController < ApplicationController def current_user super || current_resource_owner + rescue ActiveRecord::RecordNotFound + nil + end + + def require_user! + current_resource_owner + rescue ActiveRecord::RecordNotFound + render json: { error: 'This method requires an authenticated user' }, status: 422 end def render_empty @@ -67,8 +75,14 @@ class ApiController < ApplicationController end def set_maps(statuses) + if current_account.nil? + @reblogs_map = {} + @favourites_map = {} + return + end + status_ids = statuses.flat_map { |s| [s.id, s.reblog_of_id] }.compact.uniq - @reblogs_map = Status.reblogs_map(status_ids, current_user.account) - @favourites_map = Status.favourites_map(status_ids, current_user.account) + @reblogs_map = Status.reblogs_map(status_ids, current_account) + @favourites_map = Status.favourites_map(status_ids, current_account) end end diff --git a/app/models/status.rb b/app/models/status.rb index d68b7afa60f..07aef26ee23 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -95,23 +95,29 @@ class Status < ApplicationRecord where(id: Mention.where(account: account).pluck(:status_id)).with_includes.with_counters end - def as_public_timeline(account) - joins('LEFT OUTER JOIN statuses AS reblogs ON statuses.reblog_of_id = reblogs.id') + def as_public_timeline(account = nil) + query = joins('LEFT OUTER JOIN statuses AS reblogs ON statuses.reblog_of_id = reblogs.id') .joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id') .where('accounts.silenced = FALSE') - .where('(reblogs.account_id IS NULL OR reblogs.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)) AND statuses.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)', account.id, account.id) - .with_includes - .with_counters + + unless account.nil? + query = query.where('(reblogs.account_id IS NULL OR reblogs.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)) AND statuses.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)', account.id, account.id) + end + + query.with_includes.with_counters end - def as_tag_timeline(tag, account) - tag.statuses + def as_tag_timeline(tag, account = nil) + query = tag.statuses .joins('LEFT OUTER JOIN statuses AS reblogs ON statuses.reblog_of_id = reblogs.id') .joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id') .where('accounts.silenced = FALSE') - .where('(reblogs.account_id IS NULL OR reblogs.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)) AND statuses.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)', account.id, account.id) - .with_includes - .with_counters + + unless account.nil? + query = query.where('(reblogs.account_id IS NULL OR reblogs.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)) AND statuses.account_id NOT IN (SELECT target_account_id FROM blocks WHERE account_id = ?)', account.id, account.id) + end + + query.with_includes.with_counters end def favourites_map(status_ids, account_id) diff --git a/app/views/api/v1/timelines/index.rabl b/app/views/api/v1/timelines/index.rabl new file mode 100644 index 00000000000..0a0ed13c5b2 --- /dev/null +++ b/app/views/api/v1/timelines/index.rabl @@ -0,0 +1,2 @@ +collection @statuses +extends('api/v1/statuses/show') diff --git a/config/routes.rb b/config/routes.rb index 0a20d165522..26b61b8e1ed 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -55,13 +55,6 @@ Rails.application.routes.draw do # JSON / REST API namespace :v1 do resources :statuses, only: [:create, :show, :destroy] do - collection do - get :home - get :mentions - get :public - get '/tag/:id', action: :tag - end - member do get :context get :reblogged_by @@ -74,6 +67,15 @@ Rails.application.routes.draw do end end + resources :timelines, only: [] do + collection do + get :home + get :mentions + get :public + get '/tag/:id', action: :tag + end + end + resources :follows, only: [:create] resources :media, only: [:create] resources :apps, only: [:create] diff --git a/spec/controllers/api/v1/statuses_controller_spec.rb b/spec/controllers/api/v1/statuses_controller_spec.rb index 9f9bb0c4f24..9b027daf8bc 100644 --- a/spec/controllers/api/v1/statuses_controller_spec.rb +++ b/spec/controllers/api/v1/statuses_controller_spec.rb @@ -59,38 +59,6 @@ RSpec.describe Api::V1::StatusesController, type: :controller do end end - describe 'GET #home' do - it 'returns http success' do - get :home - expect(response).to have_http_status(:success) - end - end - - describe 'GET #mentions' do - it 'returns http success' do - get :mentions - expect(response).to have_http_status(:success) - end - end - - describe 'GET #public' do - it 'returns http success' do - get :public - expect(response).to have_http_status(:success) - end - end - - describe 'GET #tag' do - before do - post :create, params: { status: 'It is a #test' } - end - - it 'returns http success' do - get :tag, params: { id: 'test' } - expect(response).to have_http_status(:success) - end - end - describe 'POST #create' do before do post :create, params: { status: 'Hello world' } diff --git a/spec/controllers/api/v1/timelines_controller_spec.rb b/spec/controllers/api/v1/timelines_controller_spec.rb new file mode 100644 index 00000000000..c94519ac5c2 --- /dev/null +++ b/spec/controllers/api/v1/timelines_controller_spec.rb @@ -0,0 +1,80 @@ +require 'rails_helper' + +RSpec.describe Api::V1::TimelinesController, type: :controller do + render_views + + let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } + + before do + stub_request(:post, "https://pubsubhubbub.superfeedr.com/").to_return(:status => 200, :body => "", :headers => {}) + allow(controller).to receive(:doorkeeper_token) { token } + end + + context 'with a user context' do + let(:token) { double acceptable?: true, resource_owner_id: user.id } + + describe 'GET #home' do + it 'returns http success' do + get :home + expect(response).to have_http_status(:success) + end + end + + describe 'GET #mentions' do + it 'returns http success' do + get :mentions + expect(response).to have_http_status(:success) + end + end + + describe 'GET #public' do + it 'returns http success' do + get :public + expect(response).to have_http_status(:success) + end + end + + describe 'GET #tag' do + before do + PostStatusService.new.call(user.account, 'It is a #test') + end + + it 'returns http success' do + get :tag, params: { id: 'test' } + expect(response).to have_http_status(:success) + end + end + end + + context 'without a user context' do + let(:token) { double acceptable?: true, resource_owner_id: nil } + + describe 'GET #home' do + it 'returns http unprocessable entity' do + get :home + expect(response).to have_http_status(:unprocessable_entity) + end + end + + describe 'GET #mentions' do + it 'returns http unprocessable entity' do + get :mentions + expect(response).to have_http_status(:unprocessable_entity) + end + end + + describe 'GET #public' do + it 'returns http success' do + get :public + expect(response).to have_http_status(:success) + end + end + + describe 'GET #tag' do + it 'returns http success' do + get :tag, params: { id: 'test' } + expect(response).to have_http_status(:success) + end + end + end +end