From 32a49cc549e47b1e9239dd1d474f8de6c902a8a7 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Wed, 14 Feb 2018 00:53:38 +0100 Subject: [PATCH 1/3] Get optional props from validated object This is much easier and safer than "guessing" the class name based on the validator name. That can cause a problem when another class with the same name is found. The "guessing" was only added because we used OpenStruct in the tests, but we shouldn't change the code only to make tests run. I changed the tests to use the real entities, with auto-validation disabled in the constructor, so we can test the validator manually. --- lib/diaspora_federation/test/factories.rb | 2 +- .../validators/optional_aware_validator.rb | 6 ++--- .../optional_aware_validator_spec.rb | 27 +++++++++---------- spec/support/shared_validator_specs.rb | 6 +++-- 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/lib/diaspora_federation/test/factories.rb b/lib/diaspora_federation/test/factories.rb index 253d73f..3f3809b 100644 --- a/lib/diaspora_federation/test/factories.rb +++ b/lib/diaspora_federation/test/factories.rb @@ -41,7 +41,7 @@ module DiasporaFederation Fabricator(:account_migration_entity, class_name: DiasporaFederation::Entities::AccountMigration) do author { Fabricate.sequence(:diaspora_id) } - profile { Fabricate(:profile_entity) } + profile {|attrs| Fabricate(:profile_entity, author: attrs[:author]) } old_identity { Fabricate.sequence(:diaspora_id) } end diff --git a/lib/diaspora_federation/validators/optional_aware_validator.rb b/lib/diaspora_federation/validators/optional_aware_validator.rb index f0df658..1c0b467 100644 --- a/lib/diaspora_federation/validators/optional_aware_validator.rb +++ b/lib/diaspora_federation/validators/optional_aware_validator.rb @@ -13,10 +13,8 @@ module DiasporaFederation private def optional_props - entity_name = self.class.name.split("::").last.sub("Validator", "") - return [] unless Entities.const_defined?(entity_name) - - Entities.const_get(entity_name).optional_props + return [] unless @obj.class.respond_to?(:optional_props) + @obj.class.optional_props end end end diff --git a/spec/lib/diaspora_federation/validators/optional_aware_validator_spec.rb b/spec/lib/diaspora_federation/validators/optional_aware_validator_spec.rb index 164675a..049ee49 100644 --- a/spec/lib/diaspora_federation/validators/optional_aware_validator_spec.rb +++ b/spec/lib/diaspora_federation/validators/optional_aware_validator_spec.rb @@ -1,47 +1,46 @@ module DiasporaFederation describe Validators::OptionalAwareValidator do - let(:entity_data) { - {test1: "abc", test2: true, test7: "abc", multi: []} - } + def entity_stub(additional_data={}) + allow_any_instance_of(Entities::TestComplexEntity).to receive(:freeze) + allow_any_instance_of(Entities::TestComplexEntity).to receive(:validate) + entity_data = {test1: "abc", test2: true, test3: nil, test4: nil, test5: nil, test6: nil, test7: "abc", multi: []} + Entities::TestComplexEntity.new(entity_data.merge(additional_data)) + end it "validates a valid object" do - validator = Validators::TestComplexEntityValidator.new(OpenStruct.new(entity_data)) + validator = Validators::TestComplexEntityValidator.new(entity_stub) expect(validator).to be_valid expect(validator.errors).to be_empty end it "fails when a mandatory property is invalid" do ["ab", nil].each do |val| - entity = OpenStruct.new(entity_data.merge(test1: val)) - validator = Validators::TestComplexEntityValidator.new(entity) + validator = Validators::TestComplexEntityValidator.new(entity_stub(test1: val)) expect(validator).not_to be_valid expect(validator.errors).to include(:test1) end end it "fails when an optional property is invalid" do - entity = OpenStruct.new(entity_data.merge(test7: "ab")) - validator = Validators::TestComplexEntityValidator.new(entity) + validator = Validators::TestComplexEntityValidator.new(entity_stub(test7: "ab")) expect(validator).not_to be_valid expect(validator.errors).to include(:test7) end it "allows an optional property to be nil" do - entity = OpenStruct.new(entity_data.merge(test7: nil)) - validator = Validators::TestComplexEntityValidator.new(entity) + validator = Validators::TestComplexEntityValidator.new(entity_stub(test7: nil)) expect(validator).to be_valid expect(validator.errors).to be_empty end it "doesn't ignore 'not_nil' rules for an optional property" do - entity = OpenStruct.new(entity_data.merge(multi: nil)) - validator = Validators::TestComplexEntityValidator.new(entity) + validator = Validators::TestComplexEntityValidator.new(entity_stub(multi: nil)) expect(validator).not_to be_valid expect(validator.errors).to include(:multi) end - it "doesn't fail when there is no entity for this validator" do - entity = OpenStruct.new(entity_data.merge(test1: nil)) + it "doesn't fail when the entity doesn't have optional props" do + entity = OpenStruct.new(test1: nil) validator = Validators::TestUnknownEntityValidator.new(entity) expect(validator).not_to be_valid expect(validator.errors).to include(:test1) diff --git a/spec/support/shared_validator_specs.rb b/spec/support/shared_validator_specs.rb index 16def0c..97f46ee 100644 --- a/spec/support/shared_validator_specs.rb +++ b/spec/support/shared_validator_specs.rb @@ -1,6 +1,8 @@ def entity_stub(entity, data={}) - OpenStruct.new(Fabricate.schematic(entity).options[:class_name].default_values - .merge(Fabricate.attributes_for(entity)).merge(data)) + entity_class = Fabricate.schematic(entity).options[:class_name] + allow_any_instance_of(entity_class).to receive(:freeze) + allow_any_instance_of(entity_class).to receive(:validate) + entity_class.new(Fabricate.attributes_for(entity).merge(data)) end ALPHANUMERIC_RANGE = [*"0".."9", *"A".."Z", *"a".."z"].freeze From b274cc3dad65baa11bbb9052a32d6831bae9096b Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Wed, 14 Feb 2018 01:38:23 +0100 Subject: [PATCH 2/3] Fix some validators for nil values Using the real entities for the tests also uncovered some bugs where for example empty strings are converted to nil and the validation wasn't invalid in this case, but should be. --- lib/diaspora_federation/entities/account_migration.rb | 2 +- lib/diaspora_federation/validators/conversation_validator.rb | 2 +- .../validators/event_participation_validator.rb | 2 +- .../diaspora_federation/validators/web_finger_validator_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/diaspora_federation/entities/account_migration.rb b/lib/diaspora_federation/entities/account_migration.rb index 89e19c0..82deed3 100644 --- a/lib/diaspora_federation/entities/account_migration.rb +++ b/lib/diaspora_federation/entities/account_migration.rb @@ -43,7 +43,7 @@ module DiasporaFederation # Returns diaspora* ID of the new person identity. # @return [String] diaspora* ID of the new person identity def new_identity - profile.author + profile.author if profile end # @return [String] string representation of this object diff --git a/lib/diaspora_federation/validators/conversation_validator.rb b/lib/diaspora_federation/validators/conversation_validator.rb index d3d63a9..1e66994 100644 --- a/lib/diaspora_federation/validators/conversation_validator.rb +++ b/lib/diaspora_federation/validators/conversation_validator.rb @@ -9,7 +9,7 @@ module DiasporaFederation rule :subject, [:not_empty, length: {maximum: 255}] - rule :participants, diaspora_id_list: {minimum: 2} + rule :participants, [:not_empty, diaspora_id_list: {minimum: 2}] rule :messages, :not_nil end end diff --git a/lib/diaspora_federation/validators/event_participation_validator.rb b/lib/diaspora_federation/validators/event_participation_validator.rb index 9bbb9e8..8e1209f 100644 --- a/lib/diaspora_federation/validators/event_participation_validator.rb +++ b/lib/diaspora_federation/validators/event_participation_validator.rb @@ -6,7 +6,7 @@ module DiasporaFederation include RelayableValidator - rule :status, regular_expression: {regex: /\A(accepted|declined|tentative)\z/} + rule :status, [:not_empty, regular_expression: {regex: /\A(accepted|declined|tentative)\z/}] end end end diff --git a/spec/lib/diaspora_federation/validators/web_finger_validator_spec.rb b/spec/lib/diaspora_federation/validators/web_finger_validator_spec.rb index d493827..bad3334 100644 --- a/spec/lib/diaspora_federation/validators/web_finger_validator_spec.rb +++ b/spec/lib/diaspora_federation/validators/web_finger_validator_spec.rb @@ -27,7 +27,7 @@ module DiasporaFederation describe "##{prop}" do it_behaves_like "a property with a value validation/restriction" do let(:property) { prop } - let(:wrong_values) { ["", "https://asdf$%.com", "example.com"] } + let(:wrong_values) { %w[https://asdf$.com example.com] } let(:correct_values) { [nil] } end From c0b141786e5a6e3fac8c1705dd41b7c6c0cd5d83 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Wed, 14 Feb 2018 01:44:26 +0100 Subject: [PATCH 3/3] Use optional flag for discovery entities instead of default value --- lib/diaspora_federation/discovery/h_card.rb | 4 ++-- lib/diaspora_federation/discovery/web_finger.rb | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/diaspora_federation/discovery/h_card.rb b/lib/diaspora_federation/discovery/h_card.rb index 22f97f0..69d7526 100644 --- a/lib/diaspora_federation/discovery/h_card.rb +++ b/lib/diaspora_federation/discovery/h_card.rb @@ -48,7 +48,7 @@ module DiasporaFederation # @!attribute [r] nickname # The first part of the diaspora* ID # @return [String] nickname - property :nickname, :string, default: nil + property :nickname, :string, optional: true # @!attribute [r] full_name # @return [String] display name of the user @@ -60,7 +60,7 @@ module DiasporaFederation # installations). # # @return [String] link to the pod - property :url, :string, default: nil + property :url, :string, optional: true # @!attribute [r] public_key # When a user is created on the pod, the pod MUST generate a pgp keypair diff --git a/lib/diaspora_federation/discovery/web_finger.rb b/lib/diaspora_federation/discovery/web_finger.rb index c16f634..295038e 100644 --- a/lib/diaspora_federation/discovery/web_finger.rb +++ b/lib/diaspora_federation/discovery/web_finger.rb @@ -44,7 +44,7 @@ module DiasporaFederation # @!attribute [r] profile_url # @return [String] link to the users profile - property :profile_url, :string, default: nil + property :profile_url, :string, optional: true # @!attribute [r] atom_url # This atom feed is an Activity Stream of the user's public posts. diaspora* @@ -55,18 +55,18 @@ module DiasporaFederation # Note that this feed MAY also be made available through the PubSubHubbub # mechanism by supplying a in the atom feed itself. # @return [String] atom feed url - property :atom_url, :string, default: nil + property :atom_url, :string, optional: true # @!attribute [r] salmon_url # @note could be nil # @return [String] salmon endpoint url # @see https://cdn.rawgit.com/salmon-protocol/salmon-protocol/master/draft-panzer-salmon-00.html#SMLR # Panzer draft for Salmon, paragraph 3.3 - property :salmon_url, :string, default: nil + property :salmon_url, :string, optional: true # @!attribute [r] subscribe_url # This url is used to find another user on the home-pod of the user in the webfinger. - property :subscribe_url, :string, default: nil + property :subscribe_url, :string, optional: true # +hcard_url+ link relation REL_HCARD = "http://microformats.org/profile/hcard".freeze