Improve logging when validation fails

Add guid and author to error messages if available.

lib/diaspora_federation/entities/relayable.rb

@@ -187,15 +187,21 @@ module DiasporaFederation
           type = data.fetch(:parent_type) {
             break self::PARENT_TYPE if const_defined?(:PARENT_TYPE)
 
-            raise DiasporaFederation::Entity::ValidationError, "invalid #{self}! missing 'parent_type'."
+            raise DiasporaFederation::Entity::ValidationError, error_message_missing_property(data, "parent_type")
           }
           guid = data.fetch(:parent_guid) {
-            raise DiasporaFederation::Entity::ValidationError, "invalid #{self}! missing 'parent_guid'."
+            raise DiasporaFederation::Entity::ValidationError, error_message_missing_property(data, "parent_guid")
           }
 
           data[:parent] = RelatedEntity.fetch(data[:author], type, guid)
         end
 
+        def error_message_missing_property(data, missing_property)
+          obj_str = "#{class_name}#{":#{data[:guid]}" if data.has_key?(:guid)}" \
+                    "#{" from #{data[:author]}" if data.has_key?(:author)}"
+          "Invalid #{obj_str}! Missing '#{missing_property}'."
+        end
+
         def xml_parser_class
           DiasporaFederation::Parsers::RelayableXmlParser
         end

lib/diaspora_federation/entity.rb

@@ -131,7 +131,7 @@ module DiasporaFederation
     #
     # @return [String] entity name
     def self.entity_name
-      name.rpartition("::").last.tap do |word|
+      class_name.tap do |word|
         word.gsub!(/(.)([A-Z])/, '\1_\2')
         word.downcase!
       end
@@ -154,9 +154,14 @@ module DiasporaFederation
       Entities.const_get(class_name)
     end
 
+    # @return [String] class name as string
+    def self.class_name
+      name.rpartition("::").last
+    end
+
     # @return [String] string representation of this object
     def to_s
-      "#{self.class.name.rpartition('::').last}#{":#{guid}" if respond_to?(:guid)}"
+      "#{self.class.class_name}#{":#{guid}" if respond_to?(:guid)}"
     end
 
     # Renders entity to a hash representation of the entity JSON format
@@ -190,7 +195,11 @@ module DiasporaFederation
 
     def validate_missing_props(entity_data)
       missing_props = self.class.missing_props(entity_data)
-      raise ValidationError, "missing required properties: #{missing_props.join(', ')}" unless missing_props.empty?
+      return if missing_props.empty?
+
+      obj_str = "#{self.class.class_name}#{":#{entity_data[:guid]}" if entity_data.has_key?(:guid)}" \
+                "#{" from #{entity_data[:author]}" if entity_data.has_key?(:author)}"
+      raise ValidationError, "#{obj_str}: Missing required properties: #{missing_props.join(', ')}"
     end
 
     def setable?(name, val)
@@ -246,7 +255,7 @@ module DiasporaFederation
       errors = validator.errors.map do |prop, rule|
         "property: #{prop}, value: #{public_send(prop).inspect}, rule: #{rule[:rule]}, with params: #{rule[:params]}"
       end
-      "Failed validation for properties: #{errors.join(' | ')}"
+      "Failed validation for #{self}#{" from #{author}" if respond_to?(:author)} for properties: #{errors.join(' | ')}"
     end
 
     # @return [Hash] hash with all properties

spec/entities.rb

@@ -57,6 +57,12 @@ module DiasporaFederation
       entity :multi, [OtherEntity]
     end
 
+    class TestEntityWithAuthorAndGuid < DiasporaFederation::Entity
+      property :test, :string
+      property :author, :string
+      property :guid, :string
+    end
+
     class SomeRelayable < DiasporaFederation::Entity
       PARENT_TYPE = "Parent".freeze
 
@@ -74,5 +80,13 @@ module DiasporaFederation
       rule :test2, :not_nil
       rule :test3, :boolean
     end
+
+    class TestEntityWithAuthorAndGuidValidator < Validation::Validator
+      include Validation
+
+      rule :test, :boolean
+      rule :author, %i(not_empty diaspora_id)
+      rule :guid, :guid
+    end
   end
 end

spec/lib/diaspora_federation/entities/like_spec.rb

@@ -62,28 +62,24 @@ JSON
         broken_xml = <<-XML
 <like>
   <parent_guid>#{parent.guid}</parent_guid>
-  <author_signature>#{data[:author_signature]}</author_signature>
-  <parent_author_signature>#{data[:parent_author_signature]}</parent_author_signature>
 </like>
 XML
 
         expect {
           DiasporaFederation::Entities::Like.from_xml(Nokogiri::XML(broken_xml).root)
-        }.to raise_error Entity::ValidationError, "invalid DiasporaFederation::Entities::Like! missing 'parent_type'."
+        }.to raise_error Entity::ValidationError, "Invalid Like! Missing 'parent_type'."
       end
 
       it "raises a ValidationError if the parent_guid is missing" do
         broken_xml = <<-XML
 <like>
   <target_type>#{parent.entity_type}</target_type>
-  <author_signature>#{data[:author_signature]}</author_signature>
-  <parent_author_signature>#{data[:parent_author_signature]}</parent_author_signature>
 </like>
 XML
 
         expect {
           DiasporaFederation::Entities::Like.from_xml(Nokogiri::XML(broken_xml).root)
-        }.to raise_error Entity::ValidationError, "invalid DiasporaFederation::Entities::Like! missing 'parent_guid'."
+        }.to raise_error Entity::ValidationError, "Invalid Like! Missing 'parent_guid'."
       end
     end
   end

spec/lib/diaspora_federation/entities/relayable_spec.rb

@@ -256,15 +256,36 @@ XML
         it "raises a ValidationError if the parent_guid is missing" do
           broken_xml = <<-XML
 <some_relayable>
-  <author_signature/>
-  <parent_author_signature/>
 </some_relayable>
 XML
 
           expect {
             Entities::SomeRelayable.from_xml(Nokogiri::XML(broken_xml).root)
-          }.to raise_error Entity::ValidationError,
+          }.to raise_error Entity::ValidationError, "Invalid SomeRelayable! Missing 'parent_guid'."
-                           "invalid DiasporaFederation::Entities::SomeRelayable! missing 'parent_guid'."
+        end
+
+        it "adds the guid to the error message if available" do
+          broken_xml = <<-XML
+<some_relayable>
+  <guid>#{guid}</guid>
+</some_relayable>
+XML
+
+          expect {
+            Entities::SomeRelayable.from_xml(Nokogiri::XML(broken_xml).root)
+          }.to raise_error Entity::ValidationError, "Invalid SomeRelayable:#{guid}! Missing 'parent_guid'."
+        end
+
+        it "adds the author to the error message if available" do
+          broken_xml = <<-XML
+<some_relayable>
+  <author>#{author}</author>
+</some_relayable>
+XML
+
+          expect {
+            Entities::SomeRelayable.from_xml(Nokogiri::XML(broken_xml).root)
+          }.to raise_error Entity::ValidationError, "Invalid SomeRelayable from #{author}! Missing 'parent_guid'."
         end
       end
     end

spec/lib/diaspora_federation/entity_spec.rb

@@ -1,6 +1,7 @@
 module DiasporaFederation
   describe Entity do
     let(:data) { {test1: "asdf", test2: 1234, test3: false, test4: false} }
+    let(:guid) { Fabricate.sequence(:guid) }
 
     it "should extend Entity" do
       expect(Entities::TestDefaultEntity).to be < Entity
@@ -12,10 +13,26 @@ module DiasporaFederation
         expect(entity).to be_frozen
       end
 
-      it "checks for required properties" do
+      context "required properties" do
-        expect {
+        it "checks for required properties" do
-          Entities::TestDefaultEntity.new({})
+          expect {
-        }.to raise_error Entity::ValidationError, "missing required properties: test1, test2"
+            Entities::TestDefaultEntity.new({})
+          }.to raise_error Entity::ValidationError, "TestDefaultEntity: Missing required properties: test1, test2"
+        end
+
+        it "adds the guid to the error message if available" do
+          expect {
+            Entities::TestDefaultEntity.new(guid: guid)
+          }.to raise_error Entity::ValidationError,
+                           "TestDefaultEntity:#{guid}: Missing required properties: test1, test2"
+        end
+
+        it "adds the author to the error message if available" do
+          expect {
+            Entities::TestDefaultEntity.new(author: alice.diaspora_id)
+          }.to raise_error Entity::ValidationError,
+                           "TestDefaultEntity from #{alice.diaspora_id}: Missing required properties: test1, test2"
+        end
       end
 
       context "defaults" do
@@ -48,7 +65,8 @@ module DiasporaFederation
         it "validates the entity and raise an error with failed properties if not valid" do
           expect {
             Entities::TestDefaultEntity.new(invalid_data)
-          }.to raise_error Entity::ValidationError, /Failed validation for properties:.*test1.*\|.*test2.*\|.*test3/
+          }.to raise_error Entity::ValidationError,
+                           /Failed validation for TestDefaultEntity for properties:.*test1.*\|.*test2.*\|.*test3/
         end
 
         it "contains the failed rule" do
@@ -62,6 +80,34 @@ module DiasporaFederation
             Entities::TestDefaultEntity.new(invalid_data)
           }.to raise_error Entity::ValidationError, /rule: regular_expression, with params: \{:regex=>.*\}/
         end
+
+        it "adds the guid to the error message if available" do
+          expect {
+            Entities::TestEntityWithAuthorAndGuid.new(test: "invalid", guid: guid, author: alice.diaspora_id)
+          }.to raise_error Entity::ValidationError,
+                           /Failed validation for TestEntityWithAuthorAndGuid:#{guid} from .* for properties:.*/
+        end
+
+        it "handles missing guid" do
+          expect {
+            Entities::TestEntityWithAuthorAndGuid.new(test: "invalid", guid: nil, author: alice.diaspora_id)
+          }.to raise_error Entity::ValidationError,
+                           /Failed validation for TestEntityWithAuthorAndGuid: from .* for properties:.*/
+        end
+
+        it "adds the author to the error message if available" do
+          expect {
+            Entities::TestEntityWithAuthorAndGuid.new(test: "invalid", guid: guid, author: alice.diaspora_id)
+          }.to raise_error Entity::ValidationError,
+                           /Failed validation for .* from #{alice.diaspora_id} for properties:.*/
+        end
+
+        it "handles missing author" do
+          expect {
+            Entities::TestEntityWithAuthorAndGuid.new(test: "invalid", guid: guid, author: nil)
+          }.to raise_error Entity::ValidationError,
+                           /Failed validation for .* from  for properties:.*/
+        end
       end
     end