Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix range_hash sym to string conversion. Add tests. #128

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions lib/dynamoid/criteria/chain.rb
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,15 @@ def index_query
end
end

# Comparison Operator for evaluating Key Conditions.
#
# @return [Hash] a hash with the comparison operator for the query.
def range_hash(key)
val = query[key]
val = query[key.to_sym]

return { :range_value => query[key] } if query[key].is_a?(Range)
return { :range_value => val } if val.is_a?(Range)

case key.split('.').last
case key.to_s.split('.').last
when 'gt'
{ :range_greater_than => val.to_f }
when 'lt'
Expand Down
54 changes: 47 additions & 7 deletions spec/dynamoid/criteria/chain_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@
end

it 'makes string symbol for query keys' do
@chain.query = {'name' => 'Josh'}
@chain.query = {:name => 'Josh'}
@chain.send(:index).should == User.indexes[[:name]]
end

it 'finds matching index for a range query' do
@chain.query = {"created_at.gt" => @time - 1.day}
@chain.query = {"created_at.gt".to_sym => @time - 1.day}
@chain.send(:index).should == User.indexes[[:created_at]]

@chain.query = {:name => 'Josh', "created_at.lt" => @time - 1.day}
@chain.query = {:name => 'Josh', "created_at.lt".to_sym => @time - 1.day}
@chain.send(:index).should == User.indexes[[:created_at, :name]]
end

Expand All @@ -50,7 +50,8 @@
@chain.query = {:name => 'Josh', :email => '[email protected]'}
@chain.send(:index_query).should == {:hash_value => '[email protected]'}

@chain.query = {:name => 'Josh', 'created_at.gt' => @time}
# All keys are converted to_sym in the where method, so we should pass them in query as Symbols.
@chain.query = {:name => 'Josh', 'created_at.gt'.to_sym => @time}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While passing 'name' as symbol instead of a string seems natural, "created_at.gt" seems like an extra burden on the caller. I would expect most people would just call it with "created_at.gt" in string form, which is probably what the test should express.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only that for the tests it has to be called this way because it writes it directly on the query object. When you use Model.where("created_at.gt" => 'whatever') it will make "created_at.gt" symbol in the where method. No one said that it has to be sym when people use them.

@chain.send(:index_query).should == {:hash_value => 'Josh', :range_greater_than => @time.to_f}
end

Expand All @@ -66,10 +67,10 @@
end

it 'returns records with an index for a ranged query' do
@chain.query = {:name => 'Josh', "created_at.gt" => @time - 1.day}
@chain.query = {:name => 'Josh', "created_at.gt".to_sym => @time - 1.day}
@chain.send(:records_with_index).should == @user

@chain.query = {:name => 'Josh', "created_at.lt" => @time + 1.day}
@chain.query = {:name => 'Josh', "created_at.lt".to_sym => @time + 1.day}
@chain.send(:records_with_index).should == @user
end

Expand All @@ -79,7 +80,7 @@
end

it "doesn't crash if it finds a nil id in the index" do
@chain.query = {:name => 'Josh', "created_at.gt" => @time - 1.day}
@chain.query = {:name => 'Josh', "created_at.gt".to_sym => @time - 1.day}
Dynamoid::Adapter.expects(:query).
with("dynamoid_tests_index_user_created_ats_and_names", kind_of(Hash)).
returns([{ids: nil}, {ids: Set.new([42])}])
Expand Down Expand Up @@ -138,6 +139,45 @@
@chain.send(:range?).should be_false
end

context 'range hash' do
before do
@chain = Dynamoid::Criteria::Chain.new(Message)
end

# All keys from query are converted automatically to_sym in where method.

it 'uses gt comparator' do
@chain.query = { 'time.gt'.to_sym => 2 }
@chain.send(:range_hash, 'time.gt').to_a.should == {:range_greater_than => 2.to_f}.to_a
end

it 'uses lt comparator' do
@chain.query = { 'time.lt'.to_sym => 2 }
@chain.send(:range_hash, 'time.lt').to_a.should == {:range_less_than => 2.to_f}.to_a
end

it 'uses gte comparator' do
@chain.query = { 'time.gte'.to_sym => 2 }
@chain.send(:range_hash, 'time.gte').to_a.should == {:range_gte => 2.to_f}.to_a
end

it 'uses lte comparator' do
@chain.query = { 'time.lte'.to_sym => 2 }
@chain.send(:range_hash, 'time.lte').to_a.should == {:range_lte => 2.to_f}.to_a
end

it 'uses begins_with comparator' do
@chain.query = { 'time.begins_with'.to_sym => 'test' }
@chain.send(:range_hash, 'time.begins_with').to_a.should == {:range_begins_with => 'test'}.to_a
end

it 'tests right convertion from sym to string' do
@chain.query = { 'time.begins_with'.to_sym => 'test' }
@chain.send(:range_hash, 'time.begins_with').to_a.should == {:range_begins_with => 'test'}.to_a
end

end

context 'range queries' do
before do
@tweet1 = Tweet.create(:tweet_id => "x", :group => "one")
Expand Down