From 4e89226243a57df9b37e1fe8e6034b20915e30db Mon Sep 17 00:00:00 2001
From: Matt Jankowski <mjankowski@thoughtbot.com>
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 656851f43b..5761883531 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 0000000000..0ea33229ce
--- /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 511cdb4639..43631a7e5a 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 0000000000..88ba72b115
--- /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