From ae1f2183a0b86af48285d05f0edcc159ff9440bb Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 15:41:28 -0800 Subject: [PATCH 1/6] Add Resource deprecations in prep for 2.0 --- .../persistence/memory/query_service.rb | 4 +- lib/valkyrie/resource.rb | 32 +++++++- lib/valkyrie/specs/shared_specs/resource.rb | 75 +++++++++++++++++++ spec/valkyrie/resource_spec.rb | 10 ++- 4 files changed, 114 insertions(+), 7 deletions(-) diff --git a/lib/valkyrie/persistence/memory/query_service.rb b/lib/valkyrie/persistence/memory/query_service.rb index eb32a0721..79b779144 100644 --- a/lib/valkyrie/persistence/memory/query_service.rb +++ b/lib/valkyrie/persistence/memory/query_service.rb @@ -36,7 +36,7 @@ def find_by(id:) def find_by_alternate_identifier(alternate_identifier:) alternate_identifier = Valkyrie::ID.new(alternate_identifier.to_s) if alternate_identifier.is_a?(String) validate_id(alternate_identifier) - cache.select { |_key, resource| resource['alternate_ids'].include?(alternate_identifier) }.values.first || raise(::Valkyrie::Persistence::ObjectNotFoundError) + cache.select { |_key, resource| resource[:alternate_ids].include?(alternate_identifier) }.values.first || raise(::Valkyrie::Persistence::ObjectNotFoundError) end # Get a batch of resources by ID. @@ -116,7 +116,7 @@ def find_inverse_references_by(resource:, property:) begin Array.wrap(obj[property]).include?(resource.id) rescue - false + nil end end end diff --git a/lib/valkyrie/resource.rb b/lib/valkyrie/resource.rb index 43b8d5ee3..c51920ea5 100644 --- a/lib/valkyrie/resource.rb +++ b/lib/valkyrie/resource.rb @@ -34,6 +34,17 @@ def self.fields schema.keys.without(:new_record) end + def self.new(attributes = default_attributes) + if attributes.is_a?(Hash) && attributes.keys.map(&:class).uniq.include?(String) + warn "[DEPRECATION] Instantiating a Valkyrie::Resource with strings as keys has " \ + "been deprecated and will be removed in the next major release. " \ + "Please use symbols instead." \ + "Called from #{Gem.location_of_caller.join(':')}" + attributes = attributes.symbolize_keys + end + super + end + # Define an attribute. Attributes are used to describe resources. # @param name [Symbol] # @param type [Dry::Types::Type] @@ -88,7 +99,7 @@ def optimistic_locking_enabled? # @return [Hash] Hash of attributes def attributes - to_h + to_h.freeze end # @param name [Symbol] Attribute name @@ -106,7 +117,7 @@ def column_for_attribute(name) # @return [Boolean] def persisted? - @new_record == false + new_record == false end def to_key @@ -133,5 +144,22 @@ def to_s def human_readable_type self.class.human_readable_type end + + ## + # Return an attribute's value. + # @param name [#to_sym] the name of the attribute to read + def [](name) + super(name.to_sym) + rescue NoMethodError + nil + end + + ## + # Set an attribute's value. + # @param key [#to_sym] the name of the attribute to set + # @param value [] the value to set key to. + def set_value(key, value) + instance_variable_set(:"@#{key}", self.class.schema[key.to_sym].call(value)) + end end end diff --git a/lib/valkyrie/specs/shared_specs/resource.rb b/lib/valkyrie/specs/shared_specs/resource.rb index 257446cad..c546c4270 100644 --- a/lib/valkyrie/specs/shared_specs/resource.rb +++ b/lib/valkyrie/specs/shared_specs/resource.rb @@ -67,4 +67,79 @@ class MyCustomResource < Valkyrie::Resource expect(my_custom_resource.human_readable_type).to eq "My Custom Resource" end end + + describe "#[]" do + it "allows access to properties which are set" do + resource_klass.attribute :my_property + resource = resource_klass.new + + resource.my_property = "test" + + expect(resource[:my_property]).to eq ["test"] + resource_klass.schema.delete(:my_property) + end + it "returns nil for non-existent properties" do + resource = resource_klass.new + + expect(resource[:bad_property]).to eq nil + end + it "can be accessed via a string" do + resource_klass.attribute :other_property + resource = resource_klass.new + + resource.other_property = "test" + + expect(resource["other_property"]).to eq ["test"] + expect { resource["other_property"] }.to output(/\[DEPRECATION\]/).to_stderr + resource_klass.schema.delete(:other_property) + end + end + + describe "#set_value" do + it "can set a value" do + resource_klass.attribute :set_value_property + resource = resource_klass.new + + resource.set_value(:set_value_property, "test") + + expect(resource.set_value_property).to eq ["test"] + resource.set_value("set_value_property", "testing") + expect(resource.set_value_property).to eq ["testing"] + resource_klass.schema.delete(:set_value_property) + end + end + + describe ".new" do + it "can set values with symbols" do + resource_klass.attribute :symbol_property + + resource = resource_klass.new(symbol_property: "bla") + + expect(resource.symbol_property).to eq ["bla"] + resource_klass.schema.delete(:symbol_property) + end + it "can set values with string properties, but will throw a deprecation error" do + resource_klass.attribute :string_property + + resource = resource_klass.new("string_property" => "bla") + expect { resource_klass.new("string_property" => "bla") }.to output(/\[DEPRECATION\]/).to_stderr + + expect(resource.string_property).to eq ["bla"] + resource_klass.schema.delete(:string_property) + end + end + + describe "#attributes" do + it "returns all defined attributs, including nil keys" do + resource_klass.attribute :bla + + resource = resource_klass.new + + expect(resource.attributes).to have_key(:bla) + expect(resource.attributes[:internal_resource]).to eq resource_klass.to_s + expect { resource.attributes[:internal_resource] = "bla" }.to raise_error "can't modify frozen Hash" + + resource_klass.schema.delete(:bla) + end + end end diff --git a/spec/valkyrie/resource_spec.rb b/spec/valkyrie/resource_spec.rb index c5926cfe5..95ea8c136 100644 --- a/spec/valkyrie/resource_spec.rb +++ b/spec/valkyrie/resource_spec.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true require 'spec_helper' +require 'valkyrie/specs/shared_specs' RSpec.describe Valkyrie::Resource do before do @@ -11,9 +12,11 @@ class Resource < Valkyrie::Resource Object.send(:remove_const, :Resource) end subject(:resource) { Resource.new } + let(:resource_klass) { Resource } + it_behaves_like "a Valkyrie::Resource" describe "#fields" do it "returns all configured fields as an array of symbols" do - expect(Resource.fields).to eq [:id, :internal_resource, :created_at, :updated_at, :title] + expect(Resource.fields).to contain_exactly :id, :internal_resource, :created_at, :updated_at, :title end end @@ -106,8 +109,9 @@ class MyResource < Resource end describe "defining an internal attribute" do it "warns you and changes the type" do - expect { MyResource.attribute(:id) }.to output(/is a reserved attribute/).to_stderr - expect(MyResource.schema[:id]).to eq Valkyrie::Types::Set.optional + old_type = MyResource.schema[:id] + expect { MyResource.attribute(:id, Valkyrie::Types::Set) }.to output(/is a reserved attribute/).to_stderr + expect(MyResource.schema[:id]).not_to eq old_type end end end From c622fac583eb2ab6074f1a6b217cbe9fe03f72fd Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 15:42:51 -0800 Subject: [PATCH 2/6] Add missing methods to Resource in solr spec --- spec/valkyrie/persistence/solr/model_converter_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/valkyrie/persistence/solr/model_converter_spec.rb b/spec/valkyrie/persistence/solr/model_converter_spec.rb index fc2870e48..4344efb97 100644 --- a/spec/valkyrie/persistence/solr/model_converter_spec.rb +++ b/spec/valkyrie/persistence/solr/model_converter_spec.rb @@ -13,6 +13,7 @@ class Resource < Valkyrie::Resource attribute :author, Valkyrie::Types::Set attribute :birthday, Valkyrie::Types::DateTime.optional attribute :creator, Valkyrie::Types::String + attribute :birthdate, Valkyrie::Types::DateTime.optional end end after do @@ -22,6 +23,9 @@ class Resource < Valkyrie::Resource instance_double(Resource, id: "1", internal_resource: 'Resource', + title: ["Test", RDF::Literal.new("French", language: :fr)], + author: ["Author"], + creator: "Creator", attributes: { created_at: created_at, @@ -74,6 +78,7 @@ class Resource < Valkyrie::Resource instance_double(Resource, id: "1", internal_resource: 'Resource', + birthdate: Date.parse('1930-10-20'), attributes: { internal_resource: 'Resource', From dbcb67af3f8595cac8e23f5f9eaf2d0b5a110b10 Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 15:53:20 -0800 Subject: [PATCH 3/6] Converter changes to support 2.0 --- lib/valkyrie/persistence/fedora/persister.rb | 2 +- .../persistence/fedora/persister/model_converter.rb | 2 +- lib/valkyrie/persistence/memory/query_service.rb | 6 +----- lib/valkyrie/persistence/solr/model_converter.rb | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/valkyrie/persistence/fedora/persister.rb b/lib/valkyrie/persistence/fedora/persister.rb index e7489cf24..a2e3cb9a3 100644 --- a/lib/valkyrie/persistence/fedora/persister.rb +++ b/lib/valkyrie/persistence/fedora/persister.rb @@ -165,7 +165,7 @@ def validate_lock_token(resource) # @return [Valkyrie::Persistence::OptimisticLockToken] def native_lock_token(resource) return unless resource.optimistic_locking_enabled? - resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK].find { |lock_token| lock_token.adapter_id == "native-#{adapter.id}" } + resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK].find { |lock_token| lock_token.adapter_id.to_s == "native-#{adapter.id}" } end # Set Fedora request headers: diff --git a/lib/valkyrie/persistence/fedora/persister/model_converter.rb b/lib/valkyrie/persistence/fedora/persister/model_converter.rb index 839d216c8..94cc240ae 100644 --- a/lib/valkyrie/persistence/fedora/persister/model_converter.rb +++ b/lib/valkyrie/persistence/fedora/persister/model_converter.rb @@ -296,7 +296,7 @@ class NestedProperty < ::Valkyrie::ValueMapper # @param [Object] value # @return [Boolean] def self.handles?(value) - value.is_a?(Property) && value.value.is_a?(Hash) && value.value[:internal_resource] + value.is_a?(Property) && (value.value.is_a?(Hash) || value.value.is_a?(Valkyrie::Resource)) && value.value[:internal_resource] end # Generate a new parent graph containing the child graph generated from the ModelConverter::Property objects diff --git a/lib/valkyrie/persistence/memory/query_service.rb b/lib/valkyrie/persistence/memory/query_service.rb index 79b779144..1c704f9ab 100644 --- a/lib/valkyrie/persistence/memory/query_service.rb +++ b/lib/valkyrie/persistence/memory/query_service.rb @@ -113,11 +113,7 @@ def find_references_by(resource:, property:) def find_inverse_references_by(resource:, property:) ensure_persisted(resource) find_all.select do |obj| - begin - Array.wrap(obj[property]).include?(resource.id) - rescue - nil - end + Array.wrap(obj[property]).include?(resource.id) end end diff --git a/lib/valkyrie/persistence/solr/model_converter.rb b/lib/valkyrie/persistence/solr/model_converter.rb index 9a196bc99..9340d441f 100644 --- a/lib/valkyrie/persistence/solr/model_converter.rb +++ b/lib/valkyrie/persistence/solr/model_converter.rb @@ -222,7 +222,7 @@ class NestedObjectValue < ::Valkyrie::ValueMapper # @param [Object] value # @return [Boolean] def self.handles?(value) - value.value.is_a?(Hash) + value.value.is_a?(Hash) || value.value.is_a?(Valkyrie::Resource) end # Constructs a SolrRow object for a Property with a Hash value From 88ec16992f17f7534fe4a6dc54a7066fc4c4ef2f Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 15:58:49 -0800 Subject: [PATCH 4/6] Deprecate Valkyrie::Types::Int --- lib/valkyrie/types.rb | 16 ++++++++++++++++ spec/valkyrie/types_spec.rb | 14 ++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/lib/valkyrie/types.rb b/lib/valkyrie/types.rb index a87849cca..82ec6a8bd 100644 --- a/lib/valkyrie/types.rb +++ b/lib/valkyrie/types.rb @@ -98,5 +98,21 @@ def member(type) SingleValuedString = Valkyrie::Types::String.constructor do |value| ::Array.wrap(value).first.to_s end + Valkyrie::Types::Integer = Dry::Types["int"] + Valkyrie::Types::Coercible::Integer = Dry::Types["coercible.int"] + Int = Dry::Types["int"].constructor do |value| + warn "[DEPRECATION] Valkyrie::Types::Int has been renamed in dry-types and this " \ + "reference will be removed in the next major version of Valkyrie. Please use " \ + "Valkyrie::Types::Integer instead. " \ + "Called from #{Gem.location_of_caller.join(':')}" + Dry::Types["int"][value] + end + Coercible::Int = Dry::Types["coercible.int"].constructor do |value| + warn "[DEPRECATION] Valkyrie::Types::Coercible::Int has been renamed in dry-types and this " \ + "reference will be removed in the next major version of Valkyrie. Please use " \ + "Valkyrie::Types::Coercible::Integer instead. " \ + "Called from #{Gem.location_of_caller.join(':')}" + Dry::Types["coercible.int"][value] + end end end diff --git a/spec/valkyrie/types_spec.rb b/spec/valkyrie/types_spec.rb index 3ca3dcca1..2d9d27961 100644 --- a/spec/valkyrie/types_spec.rb +++ b/spec/valkyrie/types_spec.rb @@ -135,4 +135,18 @@ class Resource < Valkyrie::Resource expect(resource.my_flag).to be true end end + + describe "The INT type" do + it "works, but says it's deprecated" do + expect { Valkyrie::Types::Int[1] }.to output(/DEPRECATION/).to_stderr + expect { Valkyrie::Types::Coercible::Int[1] }.to output(/DEPRECATION/).to_stderr + end + end + + describe "the INTEGER type" do + it "works and doesn't say it's deprecated" do + expect { Valkyrie::Types::Integer[1] }.not_to output(/DEPRECATION/).to_stderr + expect { Valkyrie::Types::Coercible::Integer[1] }.not_to output(/DEPRECATION/).to_stderr + end + end end From cc1e77efe3c49a05366c519b66187fccd608b41b Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 16:17:38 -0800 Subject: [PATCH 5/6] Deprecate writing directly to attributes hash. --- lib/valkyrie/resource.rb | 48 ++++++++++++++++++++- lib/valkyrie/specs/shared_specs/resource.rb | 5 ++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/lib/valkyrie/resource.rb b/lib/valkyrie/resource.rb index c51920ea5..92edb45b2 100644 --- a/lib/valkyrie/resource.rb +++ b/lib/valkyrie/resource.rb @@ -97,9 +97,55 @@ def optimistic_locking_enabled? self.class.optimistic_locking_enabled? end + class DeprecatedHashWrite < Hash + def []=(_k, _v) + if @soft_frozen + warn "[DEPRECATION] Writing directly to attributes has been deprecated." \ + " Please use #set_value(k, v) instead or #dup first." \ + " In the next major version, this hash will be frozen. \n" \ + "Called from #{Gem.location_of_caller.join(':')}" + end + super + end + + def delete(*_args) + if @soft_frozen + warn "[DEPRECATION] Writing directly to attributes has been deprecated." \ + " Please use #set_value(k, v) instead or #dup first." \ + " In the next major version, this hash will be frozen. \n" \ + "Called from #{Gem.location_of_caller.join(':')}" + end + super + end + + def delete_if(*_args) + if @soft_frozen + warn "[DEPRECATION] Writing directly to attributes has been deprecated." \ + " Please use #set_value(k, v) instead or #dup first." \ + " In the next major version, this hash will be frozen. \n" \ + "Called from #{Gem.location_of_caller.join(':')}" + end + super + end + + def soft_freeze! + @soft_frozen = true + self + end + + def soft_thaw! + @soft_frozen = false + self + end + + def dup + super.soft_thaw! + end + end + # @return [Hash] Hash of attributes def attributes - to_h.freeze + DeprecatedHashWrite.new.merge(to_h).soft_freeze! end # @param name [Symbol] Attribute name diff --git a/lib/valkyrie/specs/shared_specs/resource.rb b/lib/valkyrie/specs/shared_specs/resource.rb index c546c4270..97f3b958b 100644 --- a/lib/valkyrie/specs/shared_specs/resource.rb +++ b/lib/valkyrie/specs/shared_specs/resource.rb @@ -137,7 +137,10 @@ class MyCustomResource < Valkyrie::Resource expect(resource.attributes).to have_key(:bla) expect(resource.attributes[:internal_resource]).to eq resource_klass.to_s - expect { resource.attributes[:internal_resource] = "bla" }.to raise_error "can't modify frozen Hash" + expect { resource.attributes[:internal_resource] = "bla" }.to output(/\[DEPRECATION\]/).to_stderr + expect { resource.attributes.delete_if { true } }.to output(/\[DEPRECATION\]/).to_stderr + expect { resource.attributes.delete(:internal_resource) }.to output(/\[DEPRECATION\]/).to_stderr + expect { resource.attributes.dup[:internal_resource] = "bla" }.not_to output.to_stderr resource_klass.schema.delete(:bla) end From 28be265b515a59449a17e9d9ad238e3c3f4dab39 Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Fri, 30 Nov 2018 16:54:06 -0800 Subject: [PATCH 6/6] Quiet down resource specs. --- lib/valkyrie/specs/shared_specs/resource.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/valkyrie/specs/shared_specs/resource.rb b/lib/valkyrie/specs/shared_specs/resource.rb index 97f3b958b..ba6dd8c1b 100644 --- a/lib/valkyrie/specs/shared_specs/resource.rb +++ b/lib/valkyrie/specs/shared_specs/resource.rb @@ -90,7 +90,6 @@ class MyCustomResource < Valkyrie::Resource resource.other_property = "test" expect(resource["other_property"]).to eq ["test"] - expect { resource["other_property"] }.to output(/\[DEPRECATION\]/).to_stderr resource_klass.schema.delete(:other_property) end end @@ -121,8 +120,8 @@ class MyCustomResource < Valkyrie::Resource it "can set values with string properties, but will throw a deprecation error" do resource_klass.attribute :string_property - resource = resource_klass.new("string_property" => "bla") - expect { resource_klass.new("string_property" => "bla") }.to output(/\[DEPRECATION\]/).to_stderr + resource = nil + expect { resource = resource_klass.new("string_property" => "bla") }.to output(/\[DEPRECATION\]/).to_stderr expect(resource.string_property).to eq ["bla"] resource_klass.schema.delete(:string_property)