From d0397ab800d90e119a9647e8f47b1f19f3b76ff1 Mon Sep 17 00:00:00 2001 From: Justin Coyne Date: Fri, 16 Oct 2015 11:30:05 -0500 Subject: [PATCH] query for nil creates correct query Previously querying for a field to be nil (e.g. library_id: nil) would just ignore that restriction and return all records. Now it actually queryies for records that don't have library_id set (e.g. -library_id:[* TO *]) Fixes #910 --- lib/active_fedora/relation/calculations.rb | 4 +- lib/active_fedora/relation/finder_methods.rb | 61 +++----- lib/active_fedora/relation/query_methods.rb | 8 +- lib/active_fedora/solr_query_builder.rb | 68 ++++++++- lib/active_fedora/solr_service.rb | 4 +- spec/integration/scoped_query_spec.rb | 12 +- spec/unit/finder_methods_spec.rb | 50 ++++++ spec/unit/query_spec.rb | 153 ++++++++++++------- spec/unit/solr_config_options_spec.rb | 10 +- 9 files changed, 258 insertions(+), 112 deletions(-) create mode 100644 spec/unit/finder_methods_spec.rb diff --git a/lib/active_fedora/relation/calculations.rb b/lib/active_fedora/relation/calculations.rb index a722847bf..c9e015581 100644 --- a/lib/active_fedora/relation/calculations.rb +++ b/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 diff --git a/lib/active_fedora/relation/finder_methods.rb b/lib/active_fedora/relation/finder_methods.rb index 4f1694568..818aca851 100644 --- a/lib/active_fedora/relation/finder_methods.rb +++ b/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] 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] conditions + # @returns [Array] 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 diff --git a/lib/active_fedora/relation/query_methods.rb b/lib/active_fedora/relation/query_methods.rb index 87ddcf317..66decf1eb 100644 --- a/lib/active_fedora/relation/query_methods.rb +++ b/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] 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] args a list of fields and directions to sort by + # @option [Array] args a list of fields and directions to sort by # # @example # Person.where(occupation_s: 'Plumber').order('name_t desc', 'color_t asc') diff --git a/lib/active_fedora/solr_query_builder.rb b/lib/active_fedora/solr_query_builder.rb index b1a117666..5bc49136a 100644 --- a/lib/active_fedora/solr_query_builder.rb +++ b/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] 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] 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] values + # @return [Array] + 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] 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 diff --git a/lib/active_fedora/solr_service.rb b/lib/active_fedora/solr_service.rb index 2a6bb91e8..5f32c4251 100644 --- a/lib/active_fedora/solr_service.rb +++ b/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 diff --git a/spec/integration/scoped_query_spec.rb b/spec/integration/scoped_query_spec.rb index 8ee744dfc..2b27925c7 100644 --- a/spec/integration/scoped_query_spec.rb +++ b/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 diff --git a/spec/unit/finder_methods_spec.rb b/spec/unit/finder_methods_spec.rb new file mode 100644 index 000000000..4f4be96c4 --- /dev/null +++ b/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 diff --git a/spec/unit/query_spec.rb b/spec/unit/query_spec.rb index a0d1a044c..b9289930f 100644 --- a/spec/unit/query_spec.rb +++ b/spec/unit/query_spec.rb @@ -7,9 +7,9 @@ module SpecModel class Basic < ActiveFedora::Base end end - @model_query = "_query_:\"{!raw f=" + ActiveFedora::SolrQueryBuilder.solr_name("has_model", :symbol) + "}SpecModel::Basic" + "\"" - @sort_query = ActiveFedora::SolrQueryBuilder.solr_name("system_create", :stored_sortable, type: :date) + ' asc' end + let(:sort_query) { ActiveFedora::SolrQueryBuilder.solr_name("system_create", :stored_sortable, type: :date) + ' asc' } + let(:model_query) { "_query_:\"{!raw f=has_model_ssim}SpecModel::Basic\"" } after(:all) do Object.send(:remove_const, :SpecModel) @@ -19,28 +19,40 @@ class Basic < ActiveFedora::Base before { allow(ActiveFedora::Base).to receive(:relation).and_return(relation) } describe "called on a concrete class" do let(:relation) { ActiveFedora::Relation.new(SpecModel::Basic) } - it "should query solr for all objects with :has_model_s of self.class" do - expect(relation).to receive(:load_from_fedora).with("changeme:30", nil).and_return("Fake Object1") - expect(relation).to receive(:load_from_fedora).with("changeme:22", nil).and_return("Fake Object2") - mock_docs = [{"id" => "changeme:30"}, {"id" => "changeme:22"}] + + it "queries solr for all objects with has_model_ssim of self.class" do + expect(relation).to receive(:load_from_fedora).with("changeme:30", nil) + .and_return("Fake Object1") + expect(relation).to receive(:load_from_fedora).with("changeme:22", nil) + .and_return("Fake Object2") + mock_docs = [{ "id" => "changeme:30" }, { "id" => "changeme:22" }] expect(mock_docs).to receive(:has_next?).and_return(false) - expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate).with(1, 1000, 'select', :params=>{:q=>@model_query, :qt => 'standard', :sort => [@sort_query], :fl=> 'id', }).and_return('response'=>{'docs'=>mock_docs}) + expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate) + .with(1, 1000, 'select', + params: { q: model_query, qt: 'standard', sort: [sort_query], fl: 'id' }) + .and_return('response' => { 'docs' => mock_docs }) expect(SpecModel::Basic.all).to eq ["Fake Object1", "Fake Object2"] end end + describe "called without a specific class" do let(:relation) { ActiveFedora::Relation.new(ActiveFedora::Base) } - it "should specify a q parameter" do - expect(relation).to receive(:load_from_fedora).with("changeme:30", true).and_return("Fake Object1") - expect(relation).to receive(:load_from_fedora).with("changeme:22", true).and_return("Fake Object2") + it "specifies a q parameter" do + expect(relation).to receive(:load_from_fedora).with("changeme:30", true) + .and_return("Fake Object1") + expect(relation).to receive(:load_from_fedora).with("changeme:22", true) + .and_return("Fake Object2") mock_docs = [{"id" => "changeme:30"},{"id" => "changeme:22"}] expect(mock_docs).to receive(:has_next?).and_return(false) - expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate).with(1, 1000, 'select', :params=>{:q=>'*:*', :qt => 'standard', :sort => [@sort_query], :fl=> 'id', }).and_return('response'=>{'docs'=>mock_docs}) + expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate) + .with(1, 1000, 'select', + params: { q: '*:*', qt: 'standard', sort: [sort_query], fl: 'id' }) + .and_return('response' => { 'docs' => mock_docs }) expect(ActiveFedora::Base.all).to eq ["Fake Object1", "Fake Object2"] end end end - + describe '#find' do describe "with :cast false" do describe "and an id is specified" do @@ -66,12 +78,15 @@ class Basic < ActiveFedora::Base end let(:relation) { ActiveFedora::Relation.new(SpecModel::Basic) } let(:solr) { ActiveFedora::SolrService.instance.conn } - let(:expected_query) { "#{@model_query} AND foo:bar AND baz:quix AND baz:quack" } - let(:expected_params) { { params: { sort: [@sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } + let(:expected_query) { "#{model_query} AND " \ + "_query_:\"{!raw f=foo}bar\" AND " \ + "_query_:\"{!raw f=baz}quix\" AND " \ + "_query_:\"{!raw f=baz}quack\"" } + let(:expected_params) { { params: { sort: [sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } let(:expected_sort_params) { { params: { sort: ["title_t desc"], fl: 'id', q: expected_query, qt: 'standard' } } } let(:mock_docs) { [{"id" => "changeme:30"},{"id" => "changeme:22"}] } - it "should filter by the provided fields" do + it "filters by the provided fields" do expect(relation).to receive(:load_from_fedora).with("changeme:30", nil).and_return("Fake Object1") expect(relation).to receive(:load_from_fedora).with("changeme:22", nil).and_return("Fake Object2") @@ -80,42 +95,53 @@ class Basic < ActiveFedora::Base expect(SpecModel::Basic.where({:foo=>'bar', :baz=>['quix','quack']})).to eq ["Fake Object1", "Fake Object2"] end - it "should correctly query for empty strings" do - expect(SpecModel::Basic.where( :active_fedora_model_ssi => '').count).to eq 0 + it "queries for empty strings" do + expect(SpecModel::Basic.where(active_fedora_model_ssi: '').count).to eq 0 end - it 'should correctly query for empty arrays' do - expect(SpecModel::Basic.where( :active_fedora_model_ssi => []).count).to eq 0 + it 'queries for empty arrays' do + expect(SpecModel::Basic.where(active_fedora_model_ssi: []).count).to eq 0 end - it "should add options" do - expect(relation).to receive(:load_from_fedora).with("changeme:30", nil).and_return("Fake Object1") - expect(relation).to receive(:load_from_fedora).with("changeme:22", nil).and_return("Fake Object2") + it "adds options" do + expect(relation).to receive(:load_from_fedora).with("changeme:30", nil) + .and_return("Fake Object1") + expect(relation).to receive(:load_from_fedora).with("changeme:22", nil) + .and_return("Fake Object2") expect(mock_docs).to receive(:has_next?).and_return(false) - expect(solr).to receive(:paginate).with(1, 1000, 'select', expected_sort_params).and_return('response'=>{'docs'=>mock_docs}) - expect(SpecModel::Basic.where(:foo=>'bar', :baz=>['quix','quack']).order('title_t desc')).to eq ["Fake Object1", "Fake Object2"] + expect(solr).to receive(:paginate).with(1, 1000, 'select', expected_sort_params) + .and_return('response' => { 'docs' => mock_docs }) + expect(SpecModel::Basic.where(foo: 'bar', baz: ['quix','quack']) + .order('title_t desc')).to eq ["Fake Object1", "Fake Object2"] end end - describe '#find_each' do before { allow(ActiveFedora::Base).to receive(:relation).and_return(relation) } let(:relation) { ActiveFedora::Relation.new(SpecModel::Basic) } it "should query solr for all objects with :active_fedora_model_s of self.class" do mock_docs = [{"id" => "changeme-30"},{"id" => "changeme-22"}] expect(mock_docs).to receive(:has_next?).and_return(false) - expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate).with(1, 1000, 'select', :params=>{:q=>@model_query, :qt => 'standard', :sort => [@sort_query], :fl=> 'id', }).and_return('response'=>{'docs'=>mock_docs}) - - allow(relation).to receive(:load_from_fedora).with("changeme-30", nil).and_return(SpecModel::Basic.new('changeme-30')) - allow(relation).to receive(:load_from_fedora).with("changeme-22", nil).and_return(SpecModel::Basic.new('changeme-22')) + expect(ActiveFedora::SolrService.instance.conn).to receive(:paginate) + .with(1, 1000, 'select', + params: { q: model_query, qt: 'standard', sort: [sort_query], fl: 'id' }) + .and_return('response' => { 'docs' => mock_docs }) + + allow(relation).to receive(:load_from_fedora).with("changeme-30", nil) + .and_return(SpecModel::Basic.new('changeme-30')) + allow(relation).to receive(:load_from_fedora).with("changeme-22", nil) + .and_return(SpecModel::Basic.new('changeme-22')) SpecModel::Basic.find_each(){ |obj| obj.class == SpecModel::Basic } end describe "with conditions" do let(:solr) { ActiveFedora::SolrService.instance.conn } - let(:expected_query) { "#{@model_query} AND foo:bar AND baz:quix AND baz:quack" } - let(:expected_params) { { params: { sort: [@sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } + let(:expected_query) { "#{model_query} AND " \ + "_query_:\"{!raw f=foo}bar\" AND " \ + "_query_:\"{!raw f=baz}quix\" AND " \ + "_query_:\"{!raw f=baz}quack\"" } + let(:expected_params) { { params: { sort: [sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } let(:mock_docs) { [{"id" => "changeme-30"}, {"id" => "changeme-22"}] } it "should filter by the provided fields" do @@ -132,8 +158,11 @@ class Basic < ActiveFedora::Base describe '#find_in_batches' do describe "with conditions hash" do let(:solr) { ActiveFedora::SolrService.instance.conn } - let(:expected_query) { "#{@model_query} AND foo:bar AND baz:quix AND baz:quack" } - let(:expected_params) { { params: { sort: [@sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } + let(:expected_query) { "#{model_query} AND " \ + "_query_:\"{!raw f=foo}bar\" AND " \ + "_query_:\"{!raw f=baz}quix\" AND " \ + "_query_:\"{!raw f=baz}quack\"" } + let(:expected_params) { { params: { sort: [sort_query], fl: 'id', q: expected_query, qt: 'standard' } } } let(:mock_docs) { double('docs') } it "should filter by the provided fields" do @@ -147,22 +176,27 @@ class Basic < ActiveFedora::Base end describe '#count' do - - it "should return a count" do - mock_result = {'response'=>{'numFound'=>7}} - expect(ActiveFedora::SolrService).to receive(:query).with(@model_query, :rows=>0, :raw=>true).and_return(mock_result) + let(:mock_result) { { 'response' => { 'numFound' => 7 } } } + + it "returns a count" do + expect(ActiveFedora::SolrService).to receive(:query) + .with(model_query, rows: 0, raw: true) + .and_return(mock_result) expect(SpecModel::Basic.count).to eq 7 end - it "should allow conditions" do - mock_result = {'response'=>{'numFound'=>7}} - expect(ActiveFedora::SolrService).to receive(:query).with("#{@model_query} AND (foo:bar)", :rows=>0, :raw=>true).and_return(mock_result) - expect(SpecModel::Basic.count(:conditions=>'foo:bar')).to eq 7 + + it "allows conditions" do + expect(ActiveFedora::SolrService).to receive(:query) + .with("#{model_query} AND (foo:bar)", rows: 0, raw: true) + .and_return(mock_result) + expect(SpecModel::Basic.count(conditions: 'foo:bar')).to eq 7 end - it "should count without a class specified" do - mock_result = {'response'=>{'numFound'=>7}} - expect(ActiveFedora::SolrService).to receive(:query).with("(foo:bar)", :rows=>0, :raw=>true).and_return(mock_result) - expect(ActiveFedora::Base.count(:conditions=>'foo:bar')).to eq 7 + it "counts without a class specified" do + expect(ActiveFedora::SolrService).to receive(:query) + .with("(foo:bar)", rows: 0, raw: true) + .and_return(mock_result) + expect(ActiveFedora::Base.count(conditions: 'foo:bar')).to eq 7 end end @@ -200,37 +234,44 @@ class Basic < ActiveFedora::Base SpecModel::Basic.first != @c end end - describe 'with one object' do - it 'should equal the first object when there is only one' do + describe 'with one object' do + it 'equals the first object when there is only one' do a = SpecModel::Basic.create! SpecModel::Basic.first == SpecModel::Basic.last end end end - + describe '#find_with_conditions' do let(:mock_result) { double('Result') } let(:klass) { SpecModel::Basic } subject { klass.find_with_conditions(conditions) } before do - expect(ActiveFedora::SolrService).to receive(:query).with(expected_query, sort: [@sort_query]).and_return(mock_result) + expect(ActiveFedora::SolrService).to receive(:query) + .with(expected_query, sort: [sort_query]).and_return(mock_result) end context "with a hash of conditions" do - let(:expected_query) { "#{@model_query} AND foo:bar AND baz:quix AND baz:quack" } + let(:expected_query) { "#{model_query} AND " \ + "_query_:\"{!raw f=foo}bar\" AND " \ + "_query_:\"{!raw f=baz}quix\" AND " \ + "_query_:\"{!raw f=baz}quack\"" } let(:conditions) { { foo: 'bar', baz: ['quix', 'quack'] } } - it "should make a query to solr and return the results" do + it "makes a query to solr and returns the results" do expect(subject).to eq mock_result end end context "with quotes in the params" do - let(:expected_query) { "#{@model_query} AND foo:9\\\"\\ Nails AND baz:7\\\"\\ version AND baz:quack" } + let(:expected_query) { "#{model_query} AND " \ + "_query_:\"{!raw f=foo}9\\\" Nails\" AND " \ + "_query_:\"{!raw f=baz}7\\\" version\" AND " \ + "_query_:\"{!raw f=baz}quack\"" } let(:conditions) { { foo: '9" Nails', baz: ['7" version', 'quack']} } - it "should escape quotes" do + it "escapes quotes" do expect(subject).to eq mock_result end end @@ -240,8 +281,8 @@ class Basic < ActiveFedora::Base context "with a hash" do let(:conditions) { {:baz=>'quack'} } - let(:expected_query) { 'baz:quack' } - it "shouldn't use the class if it's called on AF:Base " do + let(:expected_query) { "_query_:\"{!raw f=baz}quack\"" } + it "doesn't use the class if it's called on AF:Base " do expect(subject).to eq mock_result end end @@ -249,7 +290,7 @@ class Basic < ActiveFedora::Base context "called with a string" do let(:conditions) { 'chunky:monkey' } let(:expected_query) { '(chunky:monkey)' } - it "should use the query string if it's provided and wrap it in parentheses" do + it "uses the query string if it's provided and wrap it in parentheses" do expect(subject).to eq mock_result end end diff --git a/spec/unit/solr_config_options_spec.rb b/spec/unit/solr_config_options_spec.rb index 6d257f8d6..efdf7a96d 100644 --- a/spec/unit/solr_config_options_spec.rb +++ b/spec/unit/solr_config_options_spec.rb @@ -27,11 +27,15 @@ class Basic < ActiveFedora::Base expect(@test_object.to_solr[:id]).to be_nil end - it "should be used by ActiveFedora::Base#find_with_conditions" do + it "is used by ActiveFedora::Base#find_with_conditions" do mock_response = double("SolrResponse") - expect(ActiveFedora::SolrService).to receive(:query).with("_query_:\"{!raw f=#{ActiveFedora::SolrQueryBuilder.solr_name("has_model", :symbol)}}SolrSpecModel::Basic\" AND " + SOLR_DOCUMENT_ID + ':changeme\\:30', {:sort => ["#{ActiveFedora::SolrQueryBuilder.solr_name("system_create", :stored_sortable, type: :date)} asc"]}).and_return(mock_response) + expect(ActiveFedora::SolrService).to receive(:query) + .with("_query_:\"{!raw f=has_model_ssim}SolrSpecModel::Basic\" AND " \ + "_query_:\"{!raw f=MY_SAMPLE_ID}changeme:30\"", + sort: ["#{ActiveFedora::SolrQueryBuilder.solr_name("system_create", :stored_sortable, type: :date)} asc"]) + .and_return(mock_response) - expect(SolrSpecModel::Basic.find_with_conditions(:id=>"changeme:30")).to equal(mock_response) + expect(SolrSpecModel::Basic.find_with_conditions(id: "changeme:30")).to equal(mock_response) end end