Merge pull request #620 from samvera-labs/prepare_for_2

Implement Deprecations in Preparation for Valkyrie 2.0

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:

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

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.
@@ -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
-          false
-        end
+        Array.wrap(obj[property]).include?(resource.id)
       end
     end
 

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

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]
@@ -86,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
+      DeprecatedHashWrite.new.merge(to_h).soft_freeze!
     end
 
     # @param name [Symbol] Attribute name
@@ -106,7 +163,7 @@ def column_for_attribute(name)
 
     # @return [Boolean]
     def persisted?
-      @new_record == false
+      new_record == false
     end
 
     def to_key
@@ -133,5 +190,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

lib/valkyrie/specs/shared_specs/resource.rb

@@ -67,4 +67,81 @@ 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"]
+      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 = 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)
+    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 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
+  end
 end

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

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',

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

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