do not destroy location relations with invalid location updates

app/controllers/api/v1/subjects_controller.rb

@@ -42,25 +42,19 @@ def build_resource_for_create(create_params)
     subject
   end
 
-  def build_update_hash(update_params, id)
+  def build_update_hash(update_params, resource)
     locations = update_params.delete(:locations)
-    subject = Subject.find(id)
-    subject.locations = add_locations(locations, subject)
+    new_locations = add_locations(locations, resource)
     subject.save!
-    super(update_params, id)
+    subject.locations = new_locations if new_locations
+    super(update_params, resource)
   end
 
   def add_locations(locations, subject)
-    (locations || []).map.with_index do |loc, i|
-      location_params = case loc
-                        when String
-                          { content_type: loc }
-                        when Hash
-                          { content_type: loc.keys.first, external_link: true, src: loc.values.first }
-                        end
-      location_params.merge!(metadata: { index: i })
-      location_params.merge!(allow_any_content_type: true) if api_user.is_admin?
-      subject.locations.build(location_params)
+    if locations.blank?
+      nil
+    else
+      subject.locations.build(location_params(locations))
     end
   end
 
@@ -79,4 +73,22 @@ def selector
       params,
       controlled_resources)
   end
+
+  def location_params(locations)
+    (locations || []).map.with_index do |loc, i|
+      location_params = case loc
+                        when String
+                          { content_type: loc }
+                        when Hash
+                          {
+                            content_type: loc.keys.first,
+                            external_link: true,
+                            src: loc.values.first
+                          }
+                        end
+      location_params[:metadata] = { index: i }
+      location_params[:allow_any_content_type] = true if api_user.is_admin?
+      location_params
+    end
+  end
 end

spec/controllers/api/v1/subjects_controller_spec.rb

@@ -310,39 +310,76 @@
   end
 
   describe "#update" do
-    let(:resource) { create(:subject, project: create(:project, owner: user)) }
+    let(:resource) do
+      create(:subject, :with_mediums, num_media: 1, project: create(:project, owner: user))
+    end
     let(:test_attr) { :metadata }
     let(:test_attr_value) do
       {
        "interesting_data" => "Tested Collection",
        "an_interesting_array" => ["1", "2", "asdf", "99.99"]
       }
     end
+    let(:locations) { [ "image/jpeg" ] }
     let(:update_params) do
       {
        subjects: {
                   metadata: {
                              interesting_data: "Tested Collection",
                              an_interesting_array: ["1", "2", "asdf", "99.99"]
                             },
-                  locations: [ "image/jpeg" ]
+                  locations: locations
                  }
       }
     end
 
     it_behaves_like "is updatable"
 
+    context "with an authorized user" do
+      before do
+        default_request user_id: authorized_user.id, scopes: scopes
+      end
 
-    it 'should create externally linked media resources' do
-      default_request user_id: authorized_user.id, scopes: scopes
-      external_locs = [{"image/jpeg" => "http://example.com/1.jpg"}, {"image/jpeg" => "http://example.com/2.jpg"}]
-      external_src_urls = external_locs.map { |loc| loc.to_a.flatten[1] }
-      update_params[:subjects].merge!(locations: external_locs)
-      put :update, update_params.merge(id: resource.id)
-      locations = Subject.find(created_instance_id("subjects")).locations
-      aggregate_failures "external srcs" do
-        expect(locations.map(&:external_link)).to all( be true )
-        expect(locations.map(&:src)).to match_array(external_src_urls)
+      describe "using external urls" do
+        let(:external_locs) do
+          [
+            {"image/jpeg" => "http://example.com/1.jpg"},
+            {"image/jpeg" => "http://example.com/2.jpg"}
+          ]
+        end
+        let(:external_src_urls) do
+          external_locs.map { |loc| loc.to_a.flatten[1] }
+        end
+
+        it 'should create externally linked media resources' do
+          update_params[:subjects][:locations] = external_locs
+          put :update, update_params.merge(id: resource.id)
+          locations = Subject.find(created_instance_id("subjects")).locations
+          aggregate_failures "external srcs" do
+            expect(locations.map(&:external_link)).to all( be true )
+            expect(locations.map(&:src)).to match_array(external_src_urls)
+          end
+        end
+      end
+
+      context "when the mime type is not allowed" do
+        let(:locations) { [ "text/plain" ] }
+
+        it "should not overwrite existing locations" do
+          loc_ids = resource.locations.map(&:id)
+          put :update, update_params.merge(id: resource.id)
+          expect(loc_ids).to match_array(resource.reload.locations.map(&:id))
+        end
+      end
+
+      context "when the locations array is empty" do
+        let(:locations) { [] }
+
+        it "should not overwrite existing locations" do
+          loc_ids = resource.locations.map(&:id)
+          put :update, update_params.merge(id: resource.id)
+          expect(loc_ids).to match_array(resource.reload.locations.map(&:id))
+        end
       end
     end
   end

spec/factories/subjects.rb

@@ -9,8 +9,12 @@
               loudness: 11})
 
     trait :with_mediums do
-      after(:create) do |s|
-        create_list(:medium, 2, linked: s)
+      ignore do
+        num_media 2
+      end
+
+      after :create do |s, evaluator|
+        create_list(:medium, evaluator.num_media, linked: s)
       end
     end