Merge pull request #71 from SuperTux88/optional-properties

Don't add optional properties to generated XML and JSON when nil

lib/diaspora_federation/entities/event.rb

@@ -24,7 +24,7 @@ module DiasporaFederation
       # @!attribute [r] description
       #   Description of the event
       #   @return [String] event description
-      property :description, :string, default: nil
+      property :description, :string, optional: true
 
       # @!attribute [r] start
       #   The start time of the event
@@ -34,22 +34,22 @@ module DiasporaFederation
       # @!attribute [r] end
       #   The end time of the event
       #   @return [String] event end
-      property :end, :timestamp, default: nil
+      property :end, :timestamp, optional: true
 
       # @!attribute [r] all_day
       #   Points if the event is an all day event
       #   @return [Boolean] is it an all day event
-      property :all_day, :boolean, default: false
+      property :all_day, :boolean, optional: true, default: false
 
       # @!attribute [r] timezone
       #   Timezone to which the event is fixed to
       #   @return [String] timezone
-      property :timezone, :string, default: nil
+      property :timezone, :string, optional: true
 
       # @!attribute [r] location
       #   Location of the event
       #   @return [Entities::Location] location
-      entity :location, Entities::Location, default: nil
+      entity :location, Entities::Location, optional: true
     end
   end
 end

lib/diaspora_federation/entities/photo.rb

@@ -37,13 +37,13 @@ module DiasporaFederation
 
       # @!attribute [r] text
       #   @return [String] text
-      property :text, :string, default: nil
+      property :text, :string, optional: true
 
       # @!attribute [r] status_message_guid
       #   Guid of a status message this photo belongs to
       #   @see StatusMessage#guid
       #   @return [String] guid
-      property :status_message_guid, :string, default: nil
+      property :status_message_guid, :string, optional: true
 
       # @!attribute [r] height
       #   Photo height

lib/diaspora_federation/entities/post.rb

@@ -29,7 +29,7 @@ module DiasporaFederation
           property :author, :string, xml_name: :diaspora_handle
           property :guid, :string
           property :created_at, :timestamp, default: -> { Time.now.utc }
-          property :provider_display_name, :string, default: nil
+          property :provider_display_name, :string, optional: true
         end
       end
     end

lib/diaspora_federation/entities/profile.rb

@@ -20,7 +20,7 @@ module DiasporaFederation
       #   @see #full_name
       #   @see Discovery::HCard#first_name
       #   @return [String] first name
-      property :first_name, :string, default: nil
+      property :first_name, :string, optional: true
 
       # @!attribute [r] last_name
       #   @deprecated We decided to only use one name field, these should be removed
@@ -28,38 +28,38 @@ module DiasporaFederation
       #   @see #full_name
       #   @see Discovery::HCard#last_name
       #   @return [String] last name
-      property :last_name, :string, default: nil
+      property :last_name, :string, optional: true
 
       # @!attribute [r] image_url
       #   @see Discovery::HCard#photo_large_url
       #   @return [String] url to the big avatar (300x300)
-      property :image_url, :string, default: nil
+      property :image_url, :string, optional: true
       # @!attribute [r] image_url_medium
       #   @see Discovery::HCard#photo_medium_url
       #   @return [String] url to the medium avatar (100x100)
-      property :image_url_medium, :string, default: nil
+      property :image_url_medium, :string, optional: true
       # @!attribute [r] image_url_small
       #   @see Discovery::HCard#photo_small_url
       #   @return [String] url to the small avatar (50x50)
-      property :image_url_small, :string, default: nil
+      property :image_url_small, :string, optional: true
 
-      property :birthday, :string, default: nil
-      property :gender, :string, default: nil
-      property :bio, :string, default: nil
-      property :location, :string, default: nil
+      property :birthday, :string, optional: true
+      property :gender, :string, optional: true
+      property :bio, :string, optional: true
+      property :location, :string, optional: true
 
       # @!attribute [r] searchable
       #   @see Discovery::HCard#searchable
       #   @return [Boolean] searchable flag
-      property :searchable, :boolean, default: true
+      property :searchable, :boolean, optional: true, default: true
 
       # @!attribute [r] public
       #   Shows whether the profile is visible to everyone or only to contacts
       #   @return [Boolean] is it public
-      property :public, :boolean, default: false
+      property :public, :boolean, optional: true, default: false
 
-      property :nsfw, :boolean, default: false
-      property :tag_string, :string, default: nil
+      property :nsfw, :boolean, optional: true, default: false
+      property :tag_string, :string, optional: true
 
       # @return [String] string representation of this object
       def to_s

lib/diaspora_federation/entities/relayable.rb

@@ -100,7 +100,9 @@ module DiasporaFederation
       # The order for signing
       # @return [Array]
       def signature_order
-        @signature_order || self.class.class_props.keys - %i[author_signature parent_author_signature parent]
+        @signature_order || self.class.class_props.keys.reject {|key|
+          self.class.optional_props.include?(key) && public_send(key).nil?
+        } - %i[author_signature parent_author_signature parent]
       end
 
       private

lib/diaspora_federation/entities/reshare.rb

@@ -21,7 +21,7 @@ module DiasporaFederation
       # @!attribute [r] public
       #   Has no meaning at the moment
       #   @return [Boolean] public
-      property :public, :boolean, default: true # always true? (we only reshare public posts)
+      property :public, :boolean, optional: true, default: true # always true? (we only reshare public posts)
 
       # @return [String] string representation of this object
       def to_s

lib/diaspora_federation/entities/status_message.rb

@@ -14,22 +14,22 @@ module DiasporaFederation
       # @!attribute [r] photos
       #   Optional photos attached to the status message
       #   @return [[Entities::Photo]] photos
-      entity :photos, [Entities::Photo], default: []
+      entity :photos, [Entities::Photo], optional: true, default: []
 
       # @!attribute [r] location
       #   Optional location attached to the status message
       #   @return [Entities::Location] location
-      entity :location, Entities::Location, default: nil
+      entity :location, Entities::Location, optional: true
 
       # @!attribute [r] poll
       #   Optional poll attached to the status message
       #   @return [Entities::Poll] poll
-      entity :poll, Entities::Poll, default: nil
+      entity :poll, Entities::Poll, optional: true
 
       # @!attribute [r] event
       #   Optional event attached to the status message
       #   @return [Entities::Event] event
-      entity :event, Entities::Event, default: nil
+      entity :event, Entities::Event, optional: true
 
       # @!attribute [r] public
       #   Shows whether the status message is visible to everyone or only to some aspects

lib/diaspora_federation/entity.rb

@@ -270,6 +270,8 @@ module DiasporaFederation
     end
 
     def normalize_property(name, value)
+      return nil if optional_nil_value?(name, value)
+
       case self.class.class_props[name]
       when :string
         value.to_s
@@ -315,8 +317,9 @@ module DiasporaFederation
     def json_data
       enriched_properties.map {|key, value|
         type = self.class.class_props[key]
+        next if optional_nil_value?(key, value)
 
-        if !value.nil? && type.instance_of?(Class) && value.respond_to?(:to_json)
+        if !value.nil? && type.instance_of?(Class)
           entity_data = value.to_json
           [key, entity_data] unless entity_data.nil?
         elsif type.instance_of?(Array)
@@ -328,6 +331,10 @@ module DiasporaFederation
       }.compact.to_h
     end
 
+    def optional_nil_value?(name, value)
+      value.nil? && self.class.optional_props.include?(name)
+    end
+
     # Raised, if entity is not valid
     class ValidationError < RuntimeError
     end

lib/diaspora_federation/properties_dsl.rb

@@ -46,16 +46,20 @@ module DiasporaFederation
     # Return array of missing required property names
     # @return [Array<Symbol>] missing required property names
     def missing_props(args)
-      class_props.keys - default_props.keys - args.keys
+      class_props.keys - default_props.keys - optional_props - args.keys
+    end
+
+    def optional_props
+      @optional_props ||= []
     end
 
     # Return a new hash of default values, with dynamic values
     # resolved on each call
     # @return [Hash] default values
     def default_values
-      default_props.each_with_object({}) {|(name, prop), hash|
-        hash[name] = prop.respond_to?(:call) ? prop.call : prop
-      }
+      optional_props.map {|name| [name, nil] }.to_h.merge(default_props).map {|name, prop|
+        [name, prop.respond_to?(:call) ? prop.call : prop]
+      }.to_h
     end
 
     # @param [Hash] data entity data
@@ -111,6 +115,7 @@ module DiasporaFederation
       raise InvalidName unless name_valid?(name)
 
       class_props[name] = type
+      optional_props << name if opts[:optional]
       default_props[name] = opts[:default] if opts.has_key? :default
       xml_names[name] = determine_xml_name(name, type, opts)
 

lib/diaspora_federation/test/factories.rb

@@ -55,7 +55,7 @@ module DiasporaFederation
       Fabricator(:profile_entity, class_name: DiasporaFederation::Entities::Profile) do
         author { Fabricate.sequence(:diaspora_id) }
         first_name "my name"
-        last_name ""
+        last_name nil
         image_url "/assets/user/default.png"
         image_url_medium "/assets/user/default.png"
         image_url_small "/assets/user/default.png"

spec/entities.rb

@@ -11,6 +11,11 @@ module DiasporaFederation
       property :test4, :boolean, default: -> { true }
     end
 
+    class TestOptionalEntity < DiasporaFederation::Entity
+      property :test1, :string, optional: true
+      property :test2, :string
+    end
+
     class OtherEntity < DiasporaFederation::Entity
       property :asdf, :string
     end
@@ -54,6 +59,7 @@ module DiasporaFederation
       property :test4, :integer
       property :test5, :timestamp
       entity :test6, TestEntity
+      property :test7, :string, optional: true
       entity :multi, [OtherEntity]
     end
 
@@ -68,7 +74,7 @@ module DiasporaFederation
 
       include Entities::Relayable
 
-      property :property, :string
+      property :property, :string, optional: true
     end
   end
 

spec/lib/diaspora_federation/entities/account_migration_spec.rb

@@ -21,7 +21,6 @@ module DiasporaFederation
   <profile>
     <author>#{data[:profile].author}</author>
     <first_name>#{data[:profile].first_name}</first_name>
-    <last_name/>
     <image_url>#{data[:profile].image_url}</image_url>
     <image_url_medium>#{data[:profile].image_url}</image_url_medium>
     <image_url_small>#{data[:profile].image_url}</image_url_small>

spec/lib/diaspora_federation/entities/person_spec.rb

@@ -10,7 +10,6 @@ module DiasporaFederation
   <profile>
     <author>#{data[:profile].author}</author>
     <first_name>#{data[:profile].first_name}</first_name>
-    <last_name/>
     <image_url>#{data[:profile].image_url}</image_url>
     <image_url_medium>#{data[:profile].image_url}</image_url_medium>
     <image_url_small>#{data[:profile].image_url}</image_url_small>

spec/lib/diaspora_federation/entities/profile_spec.rb

@@ -6,7 +6,6 @@ module DiasporaFederation
 <profile>
   <author>#{data[:author]}</author>
   <first_name>#{data[:first_name]}</first_name>
-  <last_name/>
   <image_url>#{data[:image_url]}</image_url>
   <image_url_medium>#{data[:image_url]}</image_url_medium>
   <image_url_small>#{data[:image_url]}</image_url_small>
@@ -27,7 +26,6 @@ XML
   "entity_data": {
     "author": "#{data[:author]}",
     "first_name": "#{data[:first_name]}",
-    "last_name": "",
     "image_url": "#{data[:image_url]}",
     "image_url_medium": "#{data[:image_url]}",
     "image_url_small": "#{data[:image_url]}",

spec/lib/diaspora_federation/entities/relayable_spec.rb

@@ -148,6 +148,43 @@ XML
         expect(xml.at_xpath("new_property").text).to be_empty
       end
 
+      it "adds nil properties to xml when needed for signature_order" do
+        expected_xml = <<-XML
+<some_relayable>
+  <author>#{author}</author>
+  <guid>#{guid}</guid>
+  <parent_guid>#{parent_guid}</parent_guid>
+  <property/>
+  <new_property>#{new_property}</new_property>
+  <author_signature>aa</author_signature>
+  <parent_author_signature>bb</parent_author_signature>
+</some_relayable>
+XML
+
+        signature_order = [:author, :guid, :parent_guid, :property, "new_property"]
+        xml = Entities::SomeRelayable.new(
+          hash_with_fake_signatures.merge(property: nil), signature_order, "new_property" => new_property
+        ).to_xml
+
+        expect(xml.to_s.strip).to eq(expected_xml.strip)
+      end
+
+      it "does not add nil properties to xml when not needed for signature_order" do
+        expected_xml = <<-XML
+<some_relayable>
+  <author>#{author}</author>
+  <guid>#{guid}</guid>
+  <parent_guid>#{parent_guid}</parent_guid>
+  <author_signature>aa</author_signature>
+  <parent_author_signature>bb</parent_author_signature>
+</some_relayable>
+XML
+
+        xml = Entities::SomeRelayable.new(hash_with_fake_signatures.merge(property: nil)).to_xml
+
+        expect(xml.to_s.strip).to eq(expected_xml.strip)
+      end
+
       it "computes correct signatures for the entity" do
         expect_callback(:fetch_private_key, author).and_return(author_pkey)
         expect_callback(:fetch_private_key, local_parent.author).and_return(parent_pkey)

spec/lib/diaspora_federation/entities/status_message_spec.rb

@@ -103,8 +103,6 @@ XML
         "lng": "#{location.lng}"
       }
     },
-    "poll": null,
-    "event": null,
     "public": #{data[:public]}
   }
 }

spec/lib/diaspora_federation/entity_spec.rb

@@ -142,6 +142,35 @@ module DiasporaFederation
         end
       end
 
+      context "optional properties" do
+        it "contains nodes for optional properties when not nil" do
+          entity = Entities::TestOptionalEntity.new(test1: "aa", test2: "bb")
+          xml_children = entity.to_xml.children
+          expect(xml_children).to have_exactly(2).items
+          xml_children.each do |node|
+            expect(%w[test1 test2]).to include(node.name)
+          end
+        end
+
+        it "contains no nodes for optional nil properties" do
+          entity = Entities::TestOptionalEntity.new(test2: "bb")
+          xml_children = entity.to_xml.children
+          expect(xml_children).to have_exactly(1).items
+          xml_children.each do |node|
+            expect(%w[test2]).to include(node.name)
+          end
+        end
+
+        it "contains nodes for non optional properties when nil" do
+          entity = Entities::TestOptionalEntity.new(test1: "aa", test2: nil)
+          xml_children = entity.to_xml.children
+          expect(xml_children).to have_exactly(2).items
+          xml_children.each do |node|
+            expect(%w[test1 test2]).to include(node.name)
+          end
+        end
+      end
+
       it "replaces invalid XML characters" do
         entity = Entities::TestEntity.new(test: "asdfasdf asdf💩asdf\nasdf")
         xml = entity.to_xml.to_xml

spec/lib/diaspora_federation/properties_dsl_spec.rb

@@ -105,6 +105,18 @@ module DiasporaFederation
       end
     end
 
+    describe ".optional_props" do
+      it "contains an optional property" do
+        dsl.property :test, :string, optional: true
+        expect(dsl.optional_props).to include(:test)
+      end
+
+      it "doesn't contain normal properties" do
+        dsl.property :test, :string
+        expect(dsl.optional_props).to eq([])
+      end
+    end
+
     describe ".default_values" do
       it "can accept default values" do
         dsl.property :test, :string, default: :foobar
@@ -117,6 +129,19 @@ module DiasporaFederation
         defaults = dsl.default_values
         expect(defaults[:test]).to eq("default")
       end
+
+      it "contains nil as default value for optional properties" do
+        dsl.property :test, :string, optional: true
+        defaults = dsl.default_values
+        expect(defaults).to include(:test)
+        expect(defaults[:test]).to be_nil
+      end
+
+      it "contains the default value for optional properties with default value" do
+        dsl.property :test, :string, optional: true, default: "default"
+        defaults = dsl.default_values
+        expect(defaults[:test]).to eq("default")
+      end
     end
 
     describe ".resolv_aliases" do

spec/support/shared_entity_specs.rb

@@ -139,7 +139,8 @@ shared_examples "a JSON Entity" do
       entity_data.delete(:parent)
       nested_elements, simple_props = entity_data.partition {|_key, value| value.is_a?(Array) || value.is_a?(Hash) }
 
-      expect(to_json_output).to include_json(entity_data: simple_props.to_h)
+      expect(to_json_output).to include_json(entity_data: simple_props.reject {|_key, value| value.nil? }.to_h)
+
       nested_elements.each {|key, value|
         type = described_class.class_props[key]
         if value.is_a?(Array)