Allow options keys to be Strings in collection.update method #162

Closed
wants to merge 1 commit into from

3 participants

@marioizquierdo

I was trying to create an update query like this:

collection.update(
  {'field' => value}, 
  {'$set' => {'field' => other_value}}
)

This was only updating the first matched element but I wanted it to modify any matched document.
I found in the mongodb documentation that I needed the option multi: true.

I guessed that the ruby driver would accept a third argument for the options, so I tried it:

collection.update(
  {'field' => value}, 
  {'$set' => {'field' => other_value}},
  {'multi' => true}
)

But this dont't work, and it also don't raise any error, it just only updates the first matched document.

It realized only after browsing the source code that update options only recognize symbols, which, in this case, was not intuitive given that the other keys 'related_worlds.world_id' and '$unset' are Strings.

I think allowing those options to be either Strings or Symbols will remove friction when using the driver.

It would be better to ...

use something like ActiveSupport HashWithIndifferentAccess all around the place, but this simple change would improve at least the experience with the update method.

@marioizquierdo

It looks like my editor also removed trailing whitespaces.
The actual changes are just lines 463 and 464

@TylerBrock

Hey Mario,

We plan on moving towards indifferent access with subsequent versions of the driver for query results. However, the reason to support it would be to gain symbol accessibility for the returned hash/document (if you stored a value using a symbol as a key and it comes back only accessible via a string it can be jarring to the programmer).

That being said, I'm not sure we want to do the reverse by adding support for strings as keys where we don't have to given that it's un idiomatic and symbols should be used here.

@marioizquierdo

@TylerBrock Are you sure of that?
I tell you I needed to dig into the codebase to see why this sentence was not working:

collection.update(
  {'field' => value}, 
  {'$set' => {'field' => other_value}},
  {'multi' => true} # THIS IS IGNORED AND NO ERROR IS RAISED!!
)

If the fix for this is so easy, why would you not want to include it?

Using indifferent access for the query results is a totally separated issue, but if the query results are going to be String/Symbol agnostic, this behavior will be even more confusing and unexpected than it is right now.

@TylerBrock

It's not about it being easy or not to fix, its about it being idiomatic ruby. You shouldn't be using strings as keys for options parameters.

@marioizquierdo

Hmm, gotcha.
Anyway, I still think that this makes sense only a part of a rule that does not make sense.

Using symbols as hash keys (not only options) is recommended because symbols are unique in memory, they are just a little more efficient.

Ruby is supposed to reduce the friction when learning, to encourage the principle of minimum surprise. Most of libraries our there try to follow this principle, and that is what makes ruby so awesome, because it allows you to easily follow it. Refusing it when it's so easy makes no sense to me.

But this is not my library 🐐 and I don't want to have a going-nowhere discussion here.
Thanks for looking at the issue

@TylerBrock
@marioizquierdo

Yes, you are right, I've been looking around for examples of not ignoring the option when the option is a String and I couldn't find much.

Rails view helpers for example use only symbols... but some times you have to make an exception, like rails search_field helper.

In general, no one uses strings for options. A method like this is very intuitive:

some_method arg1, arg2, op1: va1, opt2: val2

But in the case of the update method, using the symbol is not as intuitive, because it comes after other two hashes that usually come with string keys.

May be we should not check for string options here, may be what we need to avoid a problem like the one I had is to check the options with something like ActiveSupport hash.assert_valid_keys, to ensure it raises an error

@TylerBrock

Ok, good idea. Thanks Mario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment