Improve create picture validation

app/controllers/api/pictures_controller.rb

@@ -1,11 +1,9 @@
 module Api
   class PicturesController < BaseController
     def create_resource(_type, _id, data)
-      data['content'] = Base64.decode64(data['content'])
-      picture = Picture.create(data)
-      raise BadRequestError,
-            "Failed to create Picture - #{picture.errors.full_messages.join(', ')}" unless picture.valid?
-      picture
+      Picture.create_from_base64(data)
+    rescue => err
+      raise BadRequestError, "Failed to create Picture - #{err}"
     end
   end
 end

app/models/picture.rb

@@ -2,8 +2,9 @@ class Picture < ApplicationRecord
   has_one :binary_blob, :as => :resource, :dependent => :destroy, :autosave => true
 
   validates :extension,
-            :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' },
-            :if        => :extension
+            :presence  => true,
+            :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' }
+  validates :content, :presence => true
 
   virtual_has_one :image_href, :class_name => "String"
 
@@ -32,6 +33,14 @@ def self.url_path(filename, basepath = URL_ROOT)
     end
   end
 
+  def self.create_from_base64(attributes = {})
+    attributes = attributes.with_indifferent_access
+    new(attributes.except(:content)).tap do |picture|
+      picture.content = Base64.strict_decode64(attributes[:content].to_s)
+      picture.save!
+    end
+  end
+
   def content
     binary_blob.try(:binary)
   end

spec/factories/picture.rb

@@ -1,4 +1,8 @@
 FactoryGirl.define do
   factory :picture do
+    extension 'png'
+    after(:build) do |x|
+      x.content = 'foo'
+    end
   end
 end

spec/models/picture_spec.rb

@@ -1,20 +1,31 @@
 describe Picture do
-  subject { FactoryGirl.build :picture }
+  subject { FactoryGirl.build(:picture) }
 
   it "auto-creates needed directory" do
     expect(File.directory?(described_class.directory)).to be_truthy
   end
 
-  it "#content" do
-    expect(subject.content).to be_nil
-    expected = "FOOBAR"
-    subject.content         = expected.dup
-    expect(subject.content).to eq(expected)
+  context "#content" do
+    it 'returns expected content' do
+      expected = "FOOBAR"
+      subject.content = expected.dup
+      expect(subject.content).to eq(expected)
+    end
+
+    it 'requires content' do
+      subject.content = ''
+      expect(subject.valid?).to be_falsey
+    end
   end
 
   context "#extension" do
+    it 'is required' do
+      subject.extension = nil
+      expect(subject.valid?).to be_falsey
+      expect(subject.errors.messages).to eq(:extension => ["can't be blank", "must be a png, jpg, or svg"])
+    end
+
     it "accepts only png, jpg, or svg" do
-      expect(subject.extension).to be_nil
       subject.extension = "foo"
 
       expect(subject.valid?).to be_falsey
@@ -31,7 +42,6 @@
     end
 
     it "on new record" do
-      expect(subject.extension).to be_nil
       ext = "png"
       subject.extension = ext.dup
       expect(subject.extension).to eq(ext)
@@ -47,9 +57,8 @@
 
     it "on existing record" do
       subject.save
-
       subject.reload
-      expect(subject.extension).to be_nil
+
       ext = "jpg"
       subject.extension = ext.dup
       expect(subject.extension).to eq(ext)
@@ -65,9 +74,8 @@
   end
 
   it "#size" do
-    expect(subject.size).to eq(0)
     expected = "FOOBAR"
-    subject.content         = expected.dup
+    subject.content = expected.dup
     expect(subject.size).to eq(expected.length)
   end
 
@@ -77,11 +85,6 @@
     end
 
     context "works when record is saved" do
-      it "without extension" do
-        subject.save
-        expect(subject.basename).to eq("#{subject.compressed_id}.")
-      end
-
       it "with extension" do
         subject.extension = "png"
         subject.save
@@ -90,6 +93,28 @@
     end
   end
 
+  context '.create_from_base64' do
+    let(:attributes) do
+      {
+        :extension => 'png',
+        :content   => 'aW1hZ2U='
+      }
+    end
+
+    it 'creates a picture' do
+      expect do
+        Picture.create_from_base64(attributes)
+      end.to change(Picture, :count).by(1)
+    end
+
+    it 'requires valid base64' do
+      attributes[:content] = 'bogus'
+      expect do
+        Picture.create_from_base64(attributes)
+      end.to raise_error(StandardError, 'invalid base64')
+    end
+  end
+
   it "#filename" do
     basename = "foo.bar"
     allow(subject).to receive(:basename).and_return(basename)

spec/requests/api/picture_spec.rb

@@ -63,20 +63,20 @@ def expect_result_to_include_picture_href(source_id)
   end
 
   describe 'POST /api/pictures' do
-    # one pixel png image encoded in Base64
+    # Valid base64 image
     let(:content) do
-      "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAABGdBTUEAALGP\n"\
-      "C/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3Cc\n"\
-      "ulE8AAAACXBIWXMAAAsTAAALEwEAmpwYAAABWWlUWHRYTUw6Y29tLmFkb2Jl\n"\
-      "LnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIg\n"\
-      "eDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpy\n"\
-      "ZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1u\n"\
-      "cyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAg\n"\
-      "ICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYv\n"\
-      "MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp\n"\
-      "ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS\n"\
-      "REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+\n"\
-      "xQAAAABJRU5ErkJggg==\n"
+      "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAABGdBTUEAALGP"\
+      "C/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3Cc"\
+      "ulE8AAAACXBIWXMAAAsTAAALEwEAmpwYAAABWWlUWHRYTUw6Y29tLmFkb2Jl"\
+      "LnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIg"\
+      "eDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpy"\
+      "ZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1u"\
+      "cyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAg"\
+      "ICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYv"\
+      "MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp"\
+      "ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS"\
+      "REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+"\
+      "xQAAAABJRU5ErkJggg=="
     end
 
     it 'rejects create without an appropriate role' do
@@ -117,5 +117,47 @@ def expect_result_to_include_picture_href(source_id)
       expect(response.parsed_body).to include(expected)
       expect(response).to have_http_status(:ok)
     end
+
+    it 'requires an extension' do
+      api_basic_authorize collection_action_identifier(:pictures, :create)
+
+      run_post pictures_url, :content => content
+
+      expected = {
+        'error' => a_hash_including(
+          'message' => a_string_including("Extension can't be blank")
+        )
+      }
+      expect(response).to have_http_status(:bad_request)
+      expect(response.parsed_body).to include(expected)
+    end
+
+    it 'requires content' do
+      api_basic_authorize collection_action_identifier(:pictures, :create)
+
+      run_post pictures_url, :extension => 'png'
+
+      expected = {
+        'error' => a_hash_including(
+          'message' => a_string_including("Content can't be blank")
+        )
+      }
+      expect(response).to have_http_status(:bad_request)
+      expect(response.parsed_body).to include(expected)
+    end
+
+    it 'requires content with valid base64' do
+      api_basic_authorize collection_action_identifier(:pictures, :create)
+
+      run_post pictures_url, :content => 'not base64', :extension => 'png'
+
+      expected = {
+        'error' => a_hash_including(
+          'message' => a_string_including('invalid base64')
+        )
+      }
+      expect(response).to have_http_status(:bad_request)
+      expect(response.parsed_body).to include(expected)
+    end
   end
 end