api: Raise errors from saving logs.

With this change, I realized it'd be cleaner to write the
implementation as after_* callbacks, so this includes that.  But the
fundamental change is to use log.save! instead of plain log.save.

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 731eecf8381e9c5c81f4782c46d1e1a5d5aecd54..a87e2d55f6830af42df1e110d957cf2a9ade4ee8 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -14,9 +14,9 @@ class ArvadosModel < ActiveRecord::Base
   before_destroy :ensure_permission_to_destroy
   before_create :update_modified_by_fields
   before_update :maybe_update_modified_by_fields
-  around_create { |&block| make_log_around(:create, nil, self, &block) }
-  around_update { |&block| make_log_around(:update, self, self, &block) }
-  around_destroy { |&block| make_log_around(:destroy, self, nil, &block) }
+  after_create :log_create
+  after_update :log_update
+  after_destroy :log_destroy
   validate :ensure_serialized_attribute_type
   validate :normalize_collection_uuids
 
@@ -219,19 +219,31 @@ class ArvadosModel < ActiveRecord::Base
     @old_attributes = attributes
   end
 
-  def make_log_around(event_type, old_thing, new_thing)
-    if self.is_a? Log
-      yield
-    else
-      log = Log.start_from(old_thing, event_type.to_s)
-      if not old_thing.nil?
-        log.properties['old_etag'] = @old_etag
-        log.properties['old_attributes'] = @old_attributes
-      end
-      yield
-      log.update_to new_thing
-      log_start_state
-      log.save
+  def log_change(event_type)
+    log = Log.new(event_type: event_type).fill_object(self)
+    yield log
+    log.save!
+    log_start_state
+  end
+
+  def log_create
+    log_change('create') do |log|
+      log.fill_properties('old', nil, nil)
+      log.update_to self
+    end
+  end
+
+  def log_update
+    log_change('update') do |log|
+      log.fill_properties('old', @old_etag, @old_attributes)
+      log.update_to self
+    end
+  end
+
+  def log_destroy
+    log_change('destroy') do |log|
+      log.fill_properties('old', @old_etag, @old_attributes)
+      log.update_to nil
     end
   end
 end

diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 223d125d4b7f5f60517907c0b3a1d0539b4a7e57..e9fe35267590c77459b55278edf5085975bc92dc 100644 (file)
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -16,22 +16,21 @@ class Log < ArvadosModel
     t.add :properties
   end
 
-  def self.start_from(thing, event_type)
-    self.new do |log|
-      log.event_type = event_type
-      log.properties = {
-        'old_etag' => nil,
-        'old_attributes' => nil,
-      }
-      log.seed_basics_from thing
-    end
+  def fill_object(thing)
+    self.object_kind ||= thing.kind
+    self.object_uuid ||= thing.uuid
+    self.summary ||= "#{self.event_type} of #{thing.uuid}"
+    self
+  end
+
+  def fill_properties(age, etag_prop, attrs_prop)
+    self.properties.merge!({"#{age}_etag" => etag_prop,
+                             "#{age}_attributes" => attrs_prop})
   end
 
   def update_to(thing)
-    self.seed_basics_from thing
-    self.properties["new_etag"] = thing.andand.etag
-    self.properties["new_attributes"] = thing.andand.attributes
-    case self.event_type
+    fill_properties('new', thing.andand.etag, thing.andand.attributes)
+    case event_type
     when "create"
       self.event_at = thing.created_at
     when "update"
@@ -39,14 +38,7 @@ class Log < ArvadosModel
     when "destroy"
       self.event_at = Time.now
     end
-  end
-
-  def seed_basics_from(thing)
-    if not thing.nil?
-      self.object_kind ||= thing.kind
-      self.object_uuid ||= thing.uuid
-      self.summary ||= "#{self.event_type} of #{thing.uuid}"
-    end
+    self
   end
 
   protected
@@ -64,4 +56,8 @@ class Log < ArvadosModel
   def set_default_event_at
     self.event_at ||= Time.now
   end
+
+  def log_change(event_type)
+    # Don't log changes to logs.
+  end
 end

diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb
index 7a27767d5ca6b0a05364e99b37e90b087533c42e..0939c28f9c6e2e77baf75ccea2a39eb16ffb94a0 100644 (file)
--- a/services/api/test/unit/log_test.rb
+++ b/services/api/test/unit/log_test.rb
@@ -165,4 +165,23 @@ class LogTest < ActiveSupport::TestCase
     assert_nothing_raised { log.save! }
     assert_nothing_raised { log.destroy }
   end
+
+  test "failure saving log causes failure saving object" do
+    Log.class_eval do
+      alias_method :_orig_validations, :perform_validations
+      def perform_validations(options)
+        false
+      end
+    end
+    begin
+      set_user_from_auth :active_trustedclient
+      user = users(:active)
+      user.first_name = 'Test'
+      assert_raise(ActiveRecord::RecordInvalid) { user.save! }
+    ensure
+      Log.class_eval do
+        alias_method :perform_validations, :_orig_validations
+      end
+    end
+  end
 end