Added support for options for atomic modifiers plugin. This enables pass... #395

Merged
merged 5 commits into from Mar 30, 2012

Conversation

Projects
None yet
3 participants
Contributor

hamin commented Mar 3, 2012

...ing :upsert and :safe options to the modifier operation.

Since the mongo ruby driver already supports the upsert and safe options we should be passing those along. If there are any questions I'm more than happy to explain but these are definitely useful. We had to drop down to the ruby driver because we wanted to increment keys which might not exist yet in our use case. Once the pull request is approved and merged, I will can adjust the modifier plugin docs (which I wrote ;) ) to cover these options.

@hamin hamin Added support for options for atomic modifiers plugin. This enables p…
…assing :upsert and :safe options to the modifier operation.
f4ef5fb
Contributor

bkeepers commented Mar 4, 2012

I've definitely been wanting this feature too.

I'm not sure that I love the options being passed in the same hash as the modifications. What about doing an additional hash with the options?

record.set({:foo => 'bar'}, {:safe => true})

@jnunemaker thoughts?

Contributor

hamin commented Mar 4, 2012

@bkeepers @jnunemaker yeah i was thinking about that too, i could definitely do that, it would end up looking like this

# On a class
Foo.increment({:name => 'blah'}, {:votes => 1}, {:upsert => true, :safe => true})

# On an instance
foo.increment({:votes => 1}, {:upsert => true, :safe => true})

Basically i thought adding another hash, though more 'proper', was a lil annoying and ugly :). I know that's not a good excuse, but also didn't wanna break anything else.

After I submitted the pull request i was reconsidering what I'd done and wanted to add another hash. I can change it if need be...thoughts?

@hamin hamin now passing a proper separate options argument for modifier operation…
…s instead of forcing it in the keys argument hash
62d23e9
Contributor

hamin commented Mar 4, 2012

@bkeepers @jnunemaker i feel pretty certain that we should be passing a proper options hash in the arguemnts. That is also how the ruby driver passing upsert and safe options to for modifier operations. So this is how it works with my last commit to this pull request:

# On a class
Foo.increment({:name => 'blah'}, {:votes => 1}, {:upsert => true, :safe => true})

# On an instance
foo.increment({:votes => 1}, {:upsert => true, :safe => true})

# No options still supports current syntax
Foo.increment({:name => 'blah'}, :votes => 1) # On a class

foo.increment(:votes => 1) # On an instance

This way won't be breaking any code that's not using the modifier options.

Contributor

hamin commented Mar 5, 2012

@bkeepers @jnunemaker thoughts guys? would love to see this get pulled in, definitely needing it for a production app soon :)

Contributor

hamin commented Mar 13, 2012

@bkeepers @jnunemaker mcflyyyy!!! :)

Looking good. A few nitpicky things and I'll pull. The coding style of the rest of MM has a space between parameters: hash, options=nil.

Hmm. Not a fan of explicitly allowing some keys. This would mean each time the driver updates we have to update this spot here. Also means each should be tested. Would be nice to instead somehow just work if we were dealing with options instead of criteria.

Any thoughts on how to make that work? It is difficult since we don't require passing the criteria.

Contributor

jnunemaker commented Mar 14, 2012

Loving the idea. I think there are just a few tweaks left. Thanks for your work thus far! Curious to see what you think about my suggestions.

Contributor

hamin commented Mar 14, 2012

@jnunemaker I understand your concerns regarding filtering specific keys so I implemented a way to pass criteria, updates, and options hash properly to the ruby driver method. It's again backwards compatible as the last update to the pull-request. This was actually a lot trickier than I expected and kept me up late at night :)

The only thing left at this point is adding the same support to the decrement, set, and unset modifiers since they're not using the same modifier_update method (rightly soI might add). I have to add some tests to cover them and I think we should be good.

Let me know what you think. /cc @bkeepers

@jnunemaker jnunemaker commented on the diff Mar 14, 2012

lib/mongo_mapper/plugins/modifiers.rb
end
def criteria_and_keys_from_args(args)
- keys = args.pop
- criteria = args[0].is_a?(Hash) ? args[0] : {:id => args}
- [criteria_hash(criteria).to_hash, keys]
+ if args[0].is_a?(Hash)
+ criteria = args[0]
+ updates = args[1]
+ options = args[2]
+ else
+ split_args = args.partition{|a| a.is_a?(BSON::ObjectId)}
@jnunemaker

jnunemaker Mar 14, 2012

Contributor

This won't work with string _id's, will it?

@hamin

hamin Mar 14, 2012

Contributor

i think you're right, but we can handle that pretty easily. I'll add something later tonight for this w/ test coverage of course.

Contributor

hamin commented Mar 14, 2012

@jnunemaker what do u think of the solution? I already commented on that string id's comment which should be pretty easy. Comments on the rest of the solution? /cc @bkeepers

bkeepers merged commit 0d1de63 into mongomapper:master Mar 30, 2012

Contributor

bkeepers commented Mar 30, 2012

Tests passed on Harmony. Sorry it took so long to pull this!

Contributor

bkeepers commented Mar 30, 2012

If you have time, one addition that would be awesome is having atomic modifiers automagically respect the save settings. So if you called safe! in your module, all operations would be safe.

Contributor

hamin commented Mar 30, 2012

@bkeepers oh right, I will take a look at that later, how would one inspect if safe! is turned on within a model?

Contributor

hamin commented Mar 30, 2012

@bkeepers btw thanks for accepting the pull request always feels good to give back :)

Contributor

bkeepers commented Mar 30, 2012

See safe?.

And thanks for the contribution! Always appreciated, even if it's not pulled right away!

Contributor

hamin commented Mar 30, 2012

@bkeepers i still have to add something to make sure it works for string ids, see @jnunemaker's last comment, but I think the plugin has had the issue all along, thoughts on that?

Contributor

bkeepers commented Mar 30, 2012

@hamin not really. It should probably be fixed. But if it was already broken, I'm not too worried about it. It'd still be good to fix eventually.

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