From 3576fa0d591db69a1727153a1130ff5bebf37167 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Tue, 30 May 2017 16:30:06 -0400 Subject: [PATCH] Improve api oembed controller (#3450) * Add StreamEntryFinder class to parse URLs * Use StreamEntryFinder and clean up api/oembed controller --- app/controllers/api/oembed_controller.rb | 26 ++++------ app/lib/stream_entry_finder.rb | 34 ++++++++++++++ .../controllers/api/oembed_controller_spec.rb | 2 + spec/lib/stream_entry_finder_spec.rb | 47 +++++++++++++++++++ 4 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 app/lib/stream_entry_finder.rb create mode 100644 spec/lib/stream_entry_finder_spec.rb diff --git a/app/controllers/api/oembed_controller.rb b/app/controllers/api/oembed_controller.rb index 656851f43b9..57618835312 100644 --- a/app/controllers/api/oembed_controller.rb +++ b/app/controllers/api/oembed_controller.rb @@ -4,30 +4,22 @@ class Api::OEmbedController < ApiController respond_to :json def show - @stream_entry = stream_entry_from_url(params[:url]) - @width = params[:maxwidth].present? ? params[:maxwidth].to_i : 400 - @height = params[:maxheight].present? ? params[:maxheight].to_i : nil + @stream_entry = find_stream_entry.stream_entry + @width = maxwidth_or_default + @height = maxheight_or_default end private - def stream_entry_from_url(url) - params = Rails.application.routes.recognize_path(url) - - raise ActiveRecord::RecordNotFound unless recognized_stream_entry_url?(params) - - stream_entry(params) + def find_stream_entry + StreamEntryFinder.new(params[:url]) end - def recognized_stream_entry_url?(params) - %w(stream_entries statuses).include?(params[:controller]) && params[:action] == 'show' + def maxwidth_or_default + (params[:maxwidth].presence || 400).to_i end - def stream_entry(params) - if params[:controller] == 'stream_entries' - StreamEntry.find(params[:id]) - else - Status.find(params[:id]).stream_entry - end + def maxheight_or_default + params[:maxheight].present? ? params[:maxheight].to_i : nil end end diff --git a/app/lib/stream_entry_finder.rb b/app/lib/stream_entry_finder.rb new file mode 100644 index 00000000000..0ea33229ce9 --- /dev/null +++ b/app/lib/stream_entry_finder.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class StreamEntryFinder + attr_reader :url + + def initialize(url) + @url = url + end + + def stream_entry + verify_action! + + case recognized_params[:controller] + when 'stream_entries' + StreamEntry.find(recognized_params[:id]) + when 'statuses' + Status.find(recognized_params[:id]).stream_entry + else + raise ActiveRecord::RecordNotFound + end + end + + private + + def recognized_params + Rails.application.routes.recognize_path(url) + end + + def verify_action! + unless recognized_params[:action] == 'show' + raise ActiveRecord::RecordNotFound + end + end +end diff --git a/spec/controllers/api/oembed_controller_spec.rb b/spec/controllers/api/oembed_controller_spec.rb index 511cdb4639c..43631a7e5a0 100644 --- a/spec/controllers/api/oembed_controller_spec.rb +++ b/spec/controllers/api/oembed_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Api::OEmbedController, type: :controller do + render_views + let(:alice) { Fabricate(:account, username: 'alice') } let(:status) { Fabricate(:status, text: 'Hello world', account: alice) } diff --git a/spec/lib/stream_entry_finder_spec.rb b/spec/lib/stream_entry_finder_spec.rb new file mode 100644 index 00000000000..88ba72b1151 --- /dev/null +++ b/spec/lib/stream_entry_finder_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe StreamEntryFinder do + include RoutingHelper + + describe '#stream_entry' do + context 'with a status url' do + let(:status) { Fabricate(:status) } + let(:url) { short_account_status_url(account_username: status.account.username, id: status.id) } + subject { described_class.new(url) } + + it 'finds the stream entry' do + expect(subject.stream_entry).to eq(status.stream_entry) + end + end + + context 'with a stream entry url' do + let(:stream_entry) { Fabricate(:stream_entry) } + let(:url) { account_stream_entry_url(stream_entry.account, stream_entry) } + subject { described_class.new(url) } + + it 'finds the stream entry' do + expect(subject.stream_entry).to eq(stream_entry) + end + end + + context 'with a plausible url' do + let(:url) { 'https://example.com/users/test/updates/123/embed' } + subject { described_class.new(url) } + + it 'raises an error' do + expect { subject.stream_entry }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'with an unrecognized url' do + let(:url) { 'https://example.com/about' } + subject { described_class.new(url) } + + it 'raises an error' do + expect { subject.stream_entry }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end