Improve create picture validation

app/controllers/api/pictures_controller.rb

@@ -1,11 +1,13 @@
 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
+      raise 'requires content' unless data['content']
+      Picture.new(data.except('content')).tap do |picture|
+        picture.content = Base64.strict_decode64(data['content'])
+        picture.save!
+      end
+    rescue => err
+      raise BadRequestError, "Failed to create Picture - #{err}"
     end
   end
 end

app/models/picture.rb

@@ -4,6 +4,7 @@ class Picture < ApplicationRecord
   validates :extension,
             :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' },
             :if        => :extension
+  validates :content, :presence => true
 
   virtual_has_one :image_href, :class_name => "String"
 

spec/models/picture_spec.rb

@@ -1,15 +1,25 @@
 describe Picture do
   subject { FactoryGirl.build :picture }
 
+  before do
+    subject.content = 'foo'
+  end
+
   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
@@ -65,9 +75,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
 

spec/requests/api/collections_spec.rb

@@ -129,7 +129,7 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil)
     end
 
     it "query Pictures" do
-      FactoryGirl.create(:picture)
+      FactoryGirl.create(:picture, :content => 'foo')
       test_collection_query(:pictures, pictures_url, Picture)
     end
 

spec/requests/api/picture_spec.rb

@@ -6,9 +6,13 @@
 # - Query picture and image_href of service_requests   /api/service_requests/:id?attributes=picture,picture.image_href
 #
 describe "Pictures" do
+  # Valid base64
+  let(:content) do
+    "aW1hZ2U="
+  end
   let(:dialog1)  { FactoryGirl.create(:dialog, :label => "ServiceDialog1") }
   let(:ra1)      { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
-  let(:picture)  { FactoryGirl.create(:picture, :extension => "jpg") }
+  let(:picture)  { FactoryGirl.create(:picture, :extension => "jpg", :content => content) }
   let(:template) do
     FactoryGirl.create(:service_template,
                        :name             => "ServiceTemplate",
@@ -63,26 +67,10 @@ def expect_result_to_include_picture_href(source_id)
   end
 
   describe 'POST /api/pictures' do
-    # one pixel png image encoded in Base64
-    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"
-    end
-
     it 'rejects create without an appropriate role' do
       api_basic_authorize
 
-      run_post pictures_url, :extension => 'png', :content => content
+      run_post pictures_url, :content => content
 
       expect(response).to have_http_status(:forbidden)
     end
@@ -117,5 +105,35 @@ 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 'rejects a bad picture' do
+      api_basic_authorize collection_action_identifier(:pictures, :create)
+
+      run_post pictures_url, :extension => 'png', :content => 'bogus'
+
+      expected = {
+        'error' => a_hash_including(
+          'kind'    => 'bad_request',
+          'message' => a_string_matching(/invalid base64/),
+        )
+      }
+      expect(response.parsed_body).to include(expected)
+      expect(response).to have_http_status(:bad_request)
+    end
+
+    it 'requires content' do
+      api_basic_authorize collection_action_identifier(:pictures, :create)
+
+      run_post pictures_url, :extension => 'png'
+
+      expected = {
+        'error' => a_hash_including(
+          'kind'    => 'bad_request',
+          'message' => a_string_matching(/requires content/),
+        )
+      }
+      expect(response.parsed_body).to include(expected)
+      expect(response).to have_http_status(:bad_request)
+    end
   end
 end

spec/requests/api/service_templates_spec.rb

@@ -13,7 +13,7 @@
   let(:ra1)        { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
   let(:ra2)        { FactoryGirl.create(:resource_action, :action => "Retirement", :dialog => dialog2) }
 
-  let(:picture)    { FactoryGirl.create(:picture, :extension => "jpg") }
+  let(:picture)    { FactoryGirl.create(:picture, :extension => "jpg", :content => 'foo') }
   let(:template)   { FactoryGirl.create(:service_template, :name => "ServiceTemplate") }
 
   describe "Service Templates query" do