Merge pull request #644 from samvera-labs/revert-628-update_dry_struc…

…t_again

Revert "2.0.0 RC1"

lib/valkyrie.rb

@@ -68,6 +68,11 @@ def logger=(logger)
     @logger = logger
   end
 
+  def warn_about_standard_queries!
+    warn "[DEPRECATION] Please enable query normalization to avoid inconsistent results between different adapters by adding `standardize_query_results: true` to your environment block" \
+     " in config\/valkyrie.yml. This will be the behavior in Valkyrie 2.0."
+  end
+
   class Config < OpenStruct
     def initialize(hsh = {})
       super(defaults.merge(hsh))
@@ -85,9 +90,10 @@ def storage_adapter
 
       def defaults
         {
+          standardize_query_result: false
         }
       end
   end
 
-  module_function :config, :logger, :logger=, :config_root_path, :environment, :config_file, :config_hash
+  module_function :config, :logger, :logger=, :config_root_path, :environment, :warn_about_standard_queries!, :config_file, :config_hash
 end

lib/valkyrie/id.rb

@@ -21,6 +21,14 @@ def eql?(other)
     end
     alias == eql?
 
+    # @deprecated Please use {.uri_for} instead
+    def to_uri
+      return RDF::Literal.new(id.to_s, datatype: RDF::URI("http://example.com/valkyrie_id")) if id.to_s.include?("://")
+      warn "[DEPRECATION] `to_uri` is deprecated and will be removed in the next major release. " \
+           "Called from #{Gem.location_of_caller.join(':')}"
+      ::RDF::URI(ActiveFedora::Base.id_to_uri(id))
+    end
+
     protected
 
       def state

lib/valkyrie/persistence/fedora.rb

@@ -4,7 +4,7 @@ module Valkyrie::Persistence
   # Implements the DataMapper Pattern to store metadata into Fedora
   module Fedora
     require 'active_triples'
-    require 'ldp'
+    require 'active_fedora'
     require 'valkyrie/persistence/fedora/permissive_schema'
     require 'valkyrie/persistence/fedora/metadata_adapter'
     require 'valkyrie/persistence/fedora/persister'

lib/valkyrie/persistence/fedora/metadata_adapter.rb

@@ -15,11 +15,13 @@ class MetadataAdapter
     # @param [String] base_path
     # @param [Valkyrie::Persistence::Fedora::PermissiveSchema] schema
     # @param [Integer] fedora_version
-    def initialize(connection:, base_path: "/", schema: Valkyrie::Persistence::Fedora::PermissiveSchema.new, fedora_version: 5)
+    def initialize(connection:, base_path: "/", schema: Valkyrie::Persistence::Fedora::PermissiveSchema.new, fedora_version: 4)
       @connection = connection
       @base_path = base_path
       @schema = schema
       @fedora_version = fedora_version
+
+      warn "[DEPRECATION] `fedora_version` will default to 5 in the next major release." unless fedora_version
     end
 
     # Construct the query service object using this adapter
@@ -76,5 +78,10 @@ def pair_path(id)
     def connection_prefix
       "#{connection.http.url_prefix}/#{base_path}"
     end
+
+    def standardize_query_result?
+      Valkyrie.warn_about_standard_queries! if Valkyrie.config.standardize_query_result != true
+      Valkyrie.config.standardize_query_result == true
+    end
   end
 end

lib/valkyrie/persistence/fedora/permissive_schema.rb

@@ -22,11 +22,27 @@ def self.id
       uri_for(:id)
     end
 
+    # @deprecated Please use {.uri_for} instead
+    def self.alternate_ids
+      warn "[DEPRECATION] `alternate_ids` is deprecated and will be removed in the next major release. " \
+           "It was never used internally - please use `uri_for(:alternate_ids)` " \
+           "Called from #{Gem.location_of_caller.join(':')}"
+      uri_for(:alternate_ids)
+    end
+
     # @return [RDF::URI]
     def self.member_ids
       uri_for(:member_ids)
     end
 
+    # @deprecated Please use {.uri_for} instead
+    def self.references
+      warn "[DEPRECATION] `references` is deprecated and will be removed in the next major release. " \
+           "It was never used internally - please use `uri_for(:references)` " \
+           "Called from #{Gem.location_of_caller.join(':')}"
+      uri_for(:references)
+    end
+
     # @return [RDF::URI]
     def self.valkyrie_bool
       uri_for(:valkyrie_bool)

lib/valkyrie/persistence/fedora/query_service.rb

@@ -29,7 +29,7 @@ def find_by_alternate_identifier(alternate_identifier:)
 
     # (see Valkyrie::Persistence::Memory::QueryService#find_many_by_ids)
     def find_many_by_ids(ids:)
-      ids = ids.uniq
+      ids = ids.uniq if adapter.standardize_query_result?
       ids.map do |id|
         begin
           find_by(id: id)
@@ -43,7 +43,7 @@ def find_many_by_ids(ids:)
     def find_parents(resource:)
       content = content_with_inbound(id: resource.id)
       parent_ids = content.graph.query([nil, RDF::Vocab::ORE.proxyFor, nil]).map(&:subject).map { |x| x.to_s.gsub(/#.*/, '') }.map { |x| adapter.uri_to_id(x) }
-      parent_ids.uniq!
+      parent_ids.uniq! if adapter.standardize_query_result?
       parent_ids.lazy.map do |id|
         find_by(id: id)
       end
@@ -131,14 +131,14 @@ def find_inverse_references_by_unordered(resource:, property:)
         content = content_with_inbound(id: resource.id)
         property_uri = adapter.schema.predicate_for(property: property, resource: nil)
         ids = content.graph.query([nil, property_uri, adapter.id_to_uri(resource.id)]).map(&:subject).map { |x| x.to_s.gsub(/#.*/, '') }.map { |x| adapter.uri_to_id(x) }
-        ids.uniq!
+        ids.uniq! if adapter.standardize_query_result?
         ids.lazy.map { |id| find_by(id: id) }
       end
 
       def find_inverse_references_by_ordered(resource:, property:)
         content = content_with_inbound(id: resource.id)
         ids = content.graph.query([nil, ::RDF::Vocab::ORE.proxyFor, adapter.id_to_uri(resource.id)]).map(&:subject).map { |x| x.to_s.gsub(/#.*/, '') }.map { |x| adapter.uri_to_id(x) }
-        ids.uniq!
+        ids.uniq! if adapter.standardize_query_result?
         ids.lazy.map { |id| find_by(id: id) }.select { |o| o[property].include?(resource.id) }
       end
 

lib/valkyrie/persistence/memory/metadata_adapter.rb

@@ -29,5 +29,10 @@ def cache
     def id
       @id ||= Valkyrie::ID.new(Digest::MD5.hexdigest(self.class.to_s))
     end
+
+    def standardize_query_result?
+      Valkyrie.warn_about_standard_queries! if Valkyrie.config.standardize_query_result != true
+      Valkyrie.config.standardize_query_result == true
+    end
   end
 end

lib/valkyrie/persistence/memory/persister.rb

@@ -86,7 +86,7 @@ def normalize_date_value(value)
       def generate_lock_token(resource)
         return unless resource.optimistic_locking_enabled?
         token = Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: adapter.id, token: Time.now.to_r)
-        resource.set_value(Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK, token)
+        resource.send("#{Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK}=", token)
       end
 
       # Check whether a resource is current.

lib/valkyrie/persistence/memory/query_service.rb

@@ -44,7 +44,7 @@ def find_by_alternate_identifier(alternate_identifier:)
     # @raise [ArgumentError] Raised when any ID is not a String or a Valkyrie::ID
     # @return [Array<Valkyrie::Resource>] All requested objects that were found
     def find_many_by_ids(ids:)
-      ids = ids.uniq
+      ids = ids.uniq if adapter.standardize_query_result?
       ids.map do |id|
         begin
           find_by(id: id)
@@ -97,7 +97,7 @@ def find_references_by(resource:, property:)
           nil
         end
       end.reject(&:nil?)
-      refs.uniq! unless ordered_property?(resource: resource, property: property)
+      refs.uniq! if adapter.standardize_query_result? && !ordered_property?(resource: resource, property: property)
       refs
     end
 

lib/valkyrie/persistence/postgres/metadata_adapter.rb

@@ -35,5 +35,10 @@ def id
         Valkyrie::ID.new(Digest::MD5.hexdigest(to_hash))
       end
     end
+
+    def standardize_query_result?
+      Valkyrie.warn_about_standard_queries! if Valkyrie.config.standardize_query_result != true
+      Valkyrie.config.standardize_query_result == true
+    end
   end
 end

lib/valkyrie/persistence/postgres/query_service.rb

@@ -189,7 +189,7 @@ def find_inverse_references_query
     # @return [String]
     def find_references_query
       <<-SQL
-        SELECT DISTINCT member.* FROM orm_resources a,
+        SELECT #{adapter.standardize_query_result? ? 'DISTINCT' : ''} member.* FROM orm_resources a,
         jsonb_array_elements(a.metadata->?) AS b(member)
         JOIN orm_resources member ON (b.member->>'id')::#{id_type} = member.id WHERE a.id = ?
       SQL

lib/valkyrie/persistence/solr/metadata_adapter.rb

@@ -64,6 +64,11 @@ def resource_factory
       Valkyrie::Persistence::Solr::ResourceFactory.new(resource_indexer: resource_indexer, adapter: self)
     end
 
+    def standardize_query_result?
+      Valkyrie.warn_about_standard_queries! if Valkyrie.config.standardize_query_result != true
+      Valkyrie.config.standardize_query_result == true
+    end
+
     # Class modeling the indexer for cases where indexing is *not* performed
     class NullIndexer
       # @note this is a no-op

lib/valkyrie/persistence/solr/queries/find_members_query.rb

@@ -3,17 +3,18 @@ module Valkyrie::Persistence::Solr::Queries
   # Responsible for returning all members of a given resource as
   # {Valkyrie::Resource}s
   class FindMembersQuery
-    attr_reader :resource, :connection, :resource_factory, :model
+    attr_reader :resource, :connection, :resource_factory, :model, :standardize_query_result
 
     # @param [Valkyrie::Resource] resource
     # @param [RSolr::Client] connection
     # @param [ResourceFactory] resource_factory
     # @param [Class] model
-    def initialize(resource:, connection:, resource_factory:, model:)
+    def initialize(resource:, connection:, resource_factory:, model:, standardize_query_result:)
       @resource = resource
       @connection = connection
       @resource_factory = resource_factory
       @model = model
+      @standardize_query_result = standardize_query_result
     end
 
     # Iterate over each Solr Document and convert each Document into a Valkyrie Resource
@@ -28,8 +29,14 @@ def run
     # @yield [Valkyrie::Resource]
     def each
       return [] unless resource.id.present?
-      member_ids.map { |id| unordered_members.find { |member| member.id == id } }.reject(&:nil?).each do |member|
-        yield member
+      if standardize_query_result
+        member_ids.map { |id| unordered_members.find { |member| member.id == id } }.reject(&:nil?).each do |member|
+          yield member
+        end
+      else
+        unordered_members.sort_by { |x| member_ids.index(x.id) }.each do |member|
+          yield member
+        end
       end
     end
 

lib/valkyrie/persistence/solr/query_service.rb

@@ -70,7 +70,8 @@ def find_members(resource:, model: nil)
         resource: resource,
         model: model,
         connection: connection,
-        resource_factory: resource_factory
+        resource_factory: resource_factory,
+        standardize_query_result: adapter.standardize_query_result?
       ).run
     end
 

lib/valkyrie/resource.rb

@@ -15,26 +15,13 @@ module Valkyrie
   # @see lib/valkyrie/specs/shared_specs/resource.rb
   class Resource < Dry::Struct
     include Draper::Decoratable
-    # Allows a Valkyrie::Resource to be instantiated without providing every
-    # available key, and makes sure the defaults are set up if no value is
-    # given.
-    def self.allow_nonexistent_keys
-      nil_2_undef = ->(v) { v.nil? ? Dry::Types::Undefined : v }
-      transform_types do |type|
-        current_meta = type.meta.merge(omittable: true)
-        if type.default?
-          type.constructor(nil_2_undef).meta(current_meta)
-        else
-          type.meta(current_meta)
-        end
-      end
-    end
+    constructor_type :schema
 
     # Overridden to provide default attributes.
     # @note The current theory is that we should use this sparingly.
     def self.inherited(subclass)
       super(subclass)
-      subclass.allow_nonexistent_keys
+      subclass.constructor_type :schema
       subclass.attribute :id, Valkyrie::Types::ID.optional, internal: true
       subclass.attribute :internal_resource, Valkyrie::Types::Any.default(subclass.to_s), internal: true
       subclass.attribute :created_at, Valkyrie::Types::DateTime.optional, internal: true
@@ -47,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]
@@ -99,12 +97,55 @@ def optimistic_locking_enabled?
       self.class.optimistic_locking_enabled?
     end
 
-    def attributes
-      super.dup.freeze
+    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
 
-    def dup
-      new({})
+    # @return [Hash] Hash of attributes
+    def attributes
+      DeprecatedHashWrite.new.merge(to_h).soft_freeze!
     end
 
     # @param name [Symbol] Attribute name
@@ -155,7 +196,7 @@ def human_readable_type
     # @param name [#to_sym] the name of the attribute to read
     def [](name)
       super(name.to_sym)
-    rescue Dry::Struct::MissingAttributeError
+    rescue NoMethodError
       nil
     end
 
@@ -164,7 +205,7 @@ def [](name)
     # @param key [#to_sym] the name of the attribute to set
     # @param value [] the value to set key to.
     def set_value(key, value)
-      @attributes[key.to_sym] = self.class.schema[key.to_sym].call(value)
+      instance_variable_set(:"@#{key}", self.class.schema[key.to_sym].call(value))
     end
   end
 end

lib/valkyrie/specs/shared_specs/queries.rb

@@ -22,6 +22,8 @@ class SecondResource < Valkyrie::Resource
   let(:persister) { adapter.persister }
   subject { adapter.query_service }
 
+  before { allow(Valkyrie.config).to receive(:standardize_query_result).and_return(true) }
+
   it { is_expected.to respond_to(:find_all).with(0).arguments }
   it { is_expected.to respond_to(:find_all_of_model).with_keywords(:model) }
   it { is_expected.to respond_to(:find_by).with_keywords(:id) }