Merge pull request #912 from projecthydra/fix_query_nil

query for nil creates correct query

lib/active_fedora/relation/calculations.rb

@@ -8,12 +8,12 @@ def count(*args)
       opts = {}
       opts[:rows] = limit_value if limit_value
       opts[:sort] = order_values if order_values
-      
+
       calculate :count, where_values, opts
     end
 
     def calculate(calculation, conditions, opts={})
-      SolrService.query(create_query(conditions), :raw=>true, :rows=>0)['response']['numFound']
+      SolrService.query(create_query(conditions), raw: true, rows: 0).fetch('response'.freeze)['numFound'.freeze]
     end
   end
 end

lib/active_fedora/relation/finder_methods.rb

@@ -111,7 +111,7 @@ def find_with_conditions(conditions, opts={})
       unless opts.include?(:sort)
         opts[:sort]=@klass.default_sort_params
       end
-      SolrService.query(create_query(conditions), opts) 
+      SolrService.query(create_query(conditions), opts)
     end
 
     # Yields the found ActiveFedora::Base object to the passed block
@@ -199,7 +199,7 @@ def class_to_load(resource, cast)
         # The true class may be a subclass of @klass, so always use from_class_uri
         resource_class = Model.from_class_uri(has_model_value(resource)) || ActiveFedora::Base
         unless equivalent_class?(resource_class)
-          raise ActiveFedora::ActiveFedoraError.new("Model mismatch. Expected #{@klass}. Got: #{resource_class}") 
+          raise ActiveFedora::ActiveFedoraError.new("Model mismatch. Expected #{@klass}. Got: #{resource_class}")
         end
         resource_class
       end
@@ -254,45 +254,42 @@ def find_some(ids, cast)
     private
 
     # Returns a solr query for the supplied conditions
-    # @param[Hash] conditions solr conditions to match
+    # @param[Hash,String,Array] conditions solr conditions to match
     def create_query(conditions)
-        case conditions
-        when Hash
-          build_query([create_query_from_hash(conditions)])
-        when String
-          build_query(["(#{conditions})"])
-        else
-          build_query(conditions)
-        end
+      build_query(build_where(conditions))
     end
 
+    # @param [Array<String>] conditions
     def build_query(conditions)
-      clauses = search_model_clause ?  [search_model_clause] : []
-      clauses += conditions.reject{|c| c.blank?}
+      clauses = search_model_clause ? [search_model_clause] : []
+      clauses += conditions.reject { |c| c.blank? }
       return "*:*" if clauses.empty?
       clauses.compact.join(" AND ")
     end
 
+    # @param [Hash<Symbol,String>] conditions
+    # @returns [Array<String>]
     def create_query_from_hash(conditions)
-      conditions.map {|key,value| condition_to_clauses(key, value)}.compact.join(" AND ")
+      conditions.map { |key, value| condition_to_clauses(key, value) }.compact
     end
 
+    # @param [Symbol] key
+    # @param [String] value
     def condition_to_clauses(key, value)
-      unless value.nil?
-        # if the key is a property name, turn it into a solr field
-        if @klass.delegated_attributes.key?(key)
-          # TODO Check to see if `key' is a possible solr field for this class, if it isn't try :searchable instead
-          key = ActiveFedora::SolrQueryBuilder.solr_name(key, :stored_searchable, type: :string)
-        end
+      SolrQueryBuilder.construct_query([[field_name_for(key), value]])
+    end
 
-        if value.empty?
-          "-#{key}:['' TO *]"
-        elsif value.is_a? Array
-          value.map { |val| "#{key}:#{solr_escape(val)}" }
-        else
-          key = SOLR_DOCUMENT_ID if (key === :id || key === :id)
-          "#{key}:#{solr_escape(value)}"
-        end
+    # If the key is a property name, turn it into a solr field
+    # @param [Symbol] key
+    # @return [String]
+    def field_name_for(key)
+      if @klass.delegated_attributes.key?(key)
+        # TODO Check to see if `key' is a possible solr field for this class, if it isn't try :searchable instead
+        ActiveFedora::SolrQueryBuilder.solr_name(key, :stored_searchable, type: :string)
+      elsif key == :id
+        SOLR_DOCUMENT_ID
+      else
+        key.to_s
       end
     end
 
@@ -306,13 +303,5 @@ def search_model_clause
         clauses.size == 1 ? clauses.first : "(#{clauses.join(" OR ")})"
       end
     end
-
-
-    private
-      # Adds esaping for spaces which are not handled by RSolr.solr_escape
-      # See rsolr/rsolr#101
-      def solr_escape terms
-        RSolr.solr_escape(terms).gsub(/\s+/,"\\ ")
-      end
   end
 end

lib/active_fedora/relation/query_methods.rb

@@ -17,7 +17,7 @@ def where_values
     def where_values=(values)
       raise ImmutableRelation if @loaded
       @values[:where] = values
-    end   
+    end
 
     def order_values
       @values[:order] || []
@@ -62,11 +62,13 @@ def where!(values)
       self
     end
 
+    # @param [Hash,String] values
+    # @return [Array<String>] list of solr hashes
     def build_where(values)
       return [] if values.blank?
       case values
       when Hash
-        [create_query_from_hash(values)]
+        create_query_from_hash(values)
       when String
         ["(#{values})"]
       else
@@ -76,7 +78,7 @@ def build_where(values)
 
     # Order the returned records by the field and direction provided
     #
-    # @option [Array<String>] args a list of fields and directions to sort by 
+    # @option [Array<String>] args a list of fields and directions to sort by
     #
     # @example
     #  Person.where(occupation_s: 'Plumber').order('name_t desc', 'color_t asc')

lib/active_fedora/solr_query_builder.rb

@@ -1,5 +1,7 @@
 module ActiveFedora
   module SolrQueryBuilder
+    PARSED_SUFFIX = '_tesim'.freeze
+
     # Construct a solr query for a list of ids
     # This is used to get a solr response based on the list of ids in an object's RELS-EXT relationhsips
     # If the id_array is empty, defaults to a query of "id:NEVER_USE_THIS_ID", which will return an empty solr response
@@ -29,18 +31,72 @@ def self.solr_name(*args)
     #   # => _query_:"{!raw f=has_model_ssim}info:fedora/afmodel:ComplexCollection" OR _query_:"{!raw f=has_model_ssim}info:fedora/afmodel:ActiveFedora_Base"
     #
     #   construct_query_for_rel [[Book.reflect_on_association(:library), "foo/bar/baz"]]
-    def self.construct_query_for_rel(field_pairs, join_with = 'AND')
+    def self.construct_query_for_rel(field_pairs, join_with = ' AND ')
       field_pairs = field_pairs.to_a if field_pairs.kind_of? Hash
+      construct_query(property_values_to_solr(field_pairs), join_with)
+    end
 
-      clauses = pairs_to_clauses(field_pairs.reject { |_, target_uri| target_uri.blank? })
-      clauses.empty? ? "id:NEVER_USE_THIS_ID" : clauses.join(" #{join_with} ".freeze)
+    # Construct a solr query from a list of pairs (e.g. [field name, values])
+    # @param [Array<Array>] pairs a list of pairs of property name and values
+    # @param [String] join_with ('AND') the value we're joining the clauses with
+    # @returns [String] a solr query
+    # @example
+    #   construct_query([['library_id_ssim', '123'], ['owner_ssim', 'Fred']])
+    #   # => "_query_:\"{!raw f=library_id_ssim}123\" AND _query_:\"{!raw f=owner_ssim}Fred\""
+    def self.construct_query(field_pairs, join_with = ' AND ')
+      pairs_to_clauses(field_pairs).join(join_with)
     end
 
     private
-      # Given an list of 2 element lists, transform to a list of solr clauses
+      # @param [Array<Array>] pairs a list of (key, value) pairs. The value itself may
+      # @return [Array] a list of solr clauses
       def self.pairs_to_clauses(pairs)
-        pairs.map do |field, target_uri|
-          raw_query(solr_field(field), target_uri)
+        pairs.flat_map do |field, value|
+          condition_to_clauses(field, value)
+        end
+      end
+
+      # @param [String] field
+      # @param [String, Array<String>] values
+      # @return [Array<String>]
+      def self.condition_to_clauses(field, values)
+        values = Array(values)
+        values << nil if values.empty?
+        values.map do |value|
+          if value.present?
+            if parsed?(field)
+              # If you do a raw query on a parsed field you won't get the matches you expect.
+              "#{field}:#{solr_escape(value)}"
+            else
+              raw_query(field, value)
+            end
+          else
+            # Check that the field is not present. In SQL: "WHERE field IS NULL"
+            "-#{field}:[* TO *]"
+          end
+        end
+      end
+
+      def self.parsed?(field)
+        field.end_with?(PARSED_SUFFIX)
+      end
+
+      # Adds esaping for spaces which are not handled by RSolr.solr_escape
+      # See rsolr/rsolr#101
+      def self.solr_escape terms
+        RSolr.solr_escape(terms).gsub(/\s+/,"\\ ")
+      end
+
+      # Given a list of pairs (e.g. [field name, values]), convert the field names
+      # to solr names
+      # @param [Array<Array>] pairs a list of pairs of property name and values
+      # @returns [Hash] map of solr fields to values
+      # @example
+      #   property_values_to_solr([['library_id', '123'], ['owner', 'Fred']])
+      #   # => [['library_id_ssim', '123'], ['owner_ssim', 'Fred']]
+      def self.property_values_to_solr(pairs)
+        pairs.each_with_object([]) do |(property, value), list|
+          list << [solr_field(property), value]
         end
       end
 

lib/active_fedora/solr_service.rb

@@ -103,8 +103,8 @@ def construct_query_for_rel(field_pairs, join_with = 'AND')
 
       def query(query, args={})
         raw = args.delete(:raw)
-        args = args.merge(:q=>query, :qt=>'standard')
-        result = SolrService.instance.conn.get('select', :params=>args)
+        args = args.merge(q: query, qt: 'standard')
+        result = SolrService.instance.conn.get('select', params: args)
         return result if raw
         result['response']['docs']
       end

spec/integration/scoped_query_spec.rb

@@ -30,7 +30,6 @@ def to_solr(doc = {})
     Object.send(:remove_const, :ModelIntegrationSpec)
   end
 
-
   describe "When there is one object in the store" do
     let!(:test_instance) { ModelIntegrationSpec::Basic.create!()}
 
@@ -67,7 +66,7 @@ def to_solr(doc = {})
         test_instance3.delete
       end
 
-      it "should query" do
+      it "queries" do
         field = ActiveFedora::SolrQueryBuilder.solr_name('foo', type: :string)
         expect(ModelIntegrationSpec::Basic.where(field => 'Beta')).to eq [test_instance1]
         expect(ModelIntegrationSpec::Basic.where('foo' => 'Beta')).to eq [test_instance1]
@@ -90,8 +89,13 @@ def to_solr(doc = {})
         expect(ModelIntegrationSpec::Basic.where("foo:bar OR bar:baz").where_values).to eq ["(foo:bar OR bar:baz)"]
       end
 
-      it "should chain where queries" do
-        expect(ModelIntegrationSpec::Basic.where(ActiveFedora::SolrQueryBuilder.solr_name('bar', type: :string) => 'Peanuts').where("#{ActiveFedora::SolrQueryBuilder.solr_name('foo', type: :string)}:bar").where_values).to eq ["#{ActiveFedora::SolrQueryBuilder.solr_name('bar', type: :string)}:Peanuts", "(#{ActiveFedora::SolrQueryBuilder.solr_name('foo', type: :string)}:bar)"]
+      it "chains where queries" do
+        first_condition = { ActiveFedora::SolrQueryBuilder.solr_name('bar', type: :string) => 'Peanuts' }
+        second_condition = "foo_tesim:bar"
+        where_values = ModelIntegrationSpec::Basic.where(first_condition)
+                                                    .where(second_condition).where_values
+        expect(where_values).to eq ["bar_tesim:Peanuts",
+                                    "(foo_tesim:bar)"]
       end
 
       it "should chain count" do

spec/unit/finder_methods_spec.rb

@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+describe ActiveFedora::FinderMethods do
+  let(:object_class) do
+    Class.new do
+      def self.delegated_attributes
+        {}
+      end
+    end
+  end
+
+  let(:finder_class) do
+    this = self
+    Class.new do
+      include ActiveFedora::FinderMethods
+      @@klass = this.object_class
+      def initialize
+        @klass = @@klass
+      end
+    end
+  end
+
+  let(:finder) { finder_class.new }
+
+  describe "#condition_to_clauses" do
+    subject { finder.send(:condition_to_clauses, key, value) }
+    let(:key) { 'library_id' }
+
+    context "when value is nil" do
+      let(:value) { nil }
+      it { is_expected.to eq "-library_id:[* TO *]" }
+    end
+
+    context "when value is empty string" do
+      let(:value) { '' }
+      it { is_expected.to eq "-library_id:[* TO *]" }
+    end
+
+    context "when value is an id" do
+      let(:value) { 'one/two/three' }
+      it { is_expected.to eq "_query_:\"{!raw f=library_id}one/two/three\"" }
+    end
+
+    context "when value is an array" do
+      let(:value) { ['one', 'four'] }
+      it { is_expected.to eq "_query_:\"{!raw f=library_id}one\" AND " \
+                             "_query_:\"{!raw f=library_id}four\"" }
+    end
+  end
+end