Permalink
Browse files

Added a trinay and .dup to the return of 'with_thing_at' to avoid

return a mutable reference that allowed in memory mutation of the
@data struct inside MockRedis

Spec is probably not consistent with present spec structure but
does illustrate a red/green against method. Spec might also need
be moved as it effects more then just hgetall.

This commit done on the dime of:
Simply Measured
twitter: @SimplyMeasured
web: http://www.simplymeasured.com
  • Loading branch information...
kungfumike committed Mar 14, 2013
1 parent c3803d5 commit c19d634c2d8b00c4f3a1e38d2d0a87260295c19e
Showing with 11 additions and 1 deletion.
  1. +1 −1 lib/mock_redis/utility_methods.rb
  2. +10 −0 spec/commands/hgetall_spec.rb
@@ -9,7 +9,7 @@ def with_thing_at(key, assertion, empty_thing_generator)
data_key_ref = data[key]
ret = yield data[key]
data[key] = data_key_ref if data[key].nil?
- ret
+ ret ? ret.dup : ret
ensure
clean_up_empties_at(key)
end
@@ -19,4 +19,14 @@
end
it_should_behave_like "a hash-only command"
+
+ it "should not return a mutatble reference to the returned data" do
+ mr = MockRedis.new
+ mr.hset(@key, 'k1', 'v1')
+ mr.hset(@key, 'k2', 'v2')
+ hash = mr.hgetall(@key)
+ hash['dont'] = 'mutate'
+ new_hash = mr.hgetall(@key)
+ new_hash.keys.sort.should == ['k1', 'k2']
+ end
end

1 comment on commit c19d634

The fix looks OK, but this isn't quite ready to merge.

First of all, what's a "trinay"? Also, there's a typo in the 'it assertion: 'mutatble' -> 'mutable'.

Would you also please move it above the shared example invocation like the rest of the specs? (line 20).

I'm also a little unclear on the need for the ternary in general. If you're worried about calling #dup on a NilClass, you could do:

ret.dup if ret

which evaluates to nil if ret is nil itself.

Thanks,
Aiden

Please sign in to comment.