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

Setting driver wide collection options for safe, fsync and timeout. #32

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Setting driver wide collection options for safe, fsync and timeout. #32

wants to merge 33 commits into from

Conversation

elricho
Copy link
Contributor

@elricho elricho commented Sep 2, 2011

Presently there is no way to set safe, fsync and timeout collection options for creates and updates.
This patch implements a driver field 'collection_options' => array(...)
The array can take 'safe' => n, 'fsync' => true/false, 'timeout' => n
See http://www.php.net/manual/en/mongocollection.save.php for more informaiton on field settings.

@YOMorales
Copy link
Contributor

Perfect. I was about to do that myself (make update operations accept extra parameters other than 'multiple' => true). But thanks for getting ahead. I still changed something related to updates; see here: #31

@elricho
Copy link
Contributor Author

elricho commented Sep 5, 2011

Cool. :) FYI - There was a bug in my insert, which is fixed now. :)

I was also thinking of extending it a little further with a mechanism that lets colection_options be set for one request only (I dont always need safe mode set for everything). I'll model it on http://book.cakephp.org/view/1045/Creating-and-Destroying-Associations-on-the-Fly#!/view/1045/Creating-and-Destroying-Associations-on-the-Fly - Like binding the options for one request only.

@YOMorales
Copy link
Contributor

That is a better approach.

At first I thought of passing a third parameter in updateAll so that collection options could be set on demand like you say (something like: Mode->updateAll($fields, $conditions, $mongodb_options). But that would need some override of the Model class' updateAll method, and IMO this is something out of the 'scope' of this plugin.

But your suggestion is better. Something like binding.
It won't even need using a new method the way 'Binding Associations' does, but the simplest way is to set a class attribute. Something like:

$this->Model->collectionOptions = array('upsert' => true, 'safe' => true. etc.);
Then in Datasource...
if (!empty($this>collectionOptions)) {
$mongoCollectionObj->update($cond, $data, $this>collectionOptions);

@AD7six
Copy link
Contributor

AD7six commented Sep 7, 2011

It is not appropriate to use a db-wide options array on multiple collections - and on function calls that are expecting different options to be used.

  • batchInsert has only safe and fsync
  • insert has safe, fsync and timeout
  • save has safe, fsync and timeout
  • update has safe, fsync, timeout, upsert and multiple

It's likely that if you want to modify these settings, you want them different for specific collections and probably specific operations. By which I mean for example: inserts on [collection] are safe, but updating some counter/timestamp field is not a safe update. Therefore, while these changes may scratch your current itch it doesn't really solve "the problem".

I'd suggest that where currently you reference $this->config['collection_options'] it should effectively reference $Model->mongoOptions[$methodname] or similar. Unless it's possible to easily change these options at least on a per collection basis I don't see a benefit in changing hardcoded options for inflexible options.

@elricho
Copy link
Contributor Author

elricho commented Sep 7, 2011

Hi Andy,

Thanks for the feedback and fair point.

To expand on your suggestion, should the options in $Model->mongoOptions[$methodname] be set by $Model->setMongoOptions(array('save' => array('safe'), 'insert' => array('safe','fsync','upsert','timeout' => n), ...));
(ie getter/setter style) or just set it directly in the model like YOMorales' suggestion ?

Cheers
r

@AD7six
Copy link
Contributor

AD7six commented Sep 7, 2011

elricho - personally I'd go with simple - if (!empty($Model->...)) - but Ichi might have a different idea, and it's his code. However, if it works (by any means) it would be simple to make it work better before/after merging.

@ichikaway
Copy link
Owner

sorry too late reply.
I agree with using model property.
I will merge it,then change to use model property.
Now the patch fails test cases, I will fix it.

Thanks great idea and discussion.

if (!empty($data['_id'])) {
$this->_convertId($data['_id']);
$cond = array('_id' => $data['_id']);
unset($data['_id']);

$data = $this->setMongoUpdateOperator($Model, $data);

$params = array_merge($this->collectionOptions['update'], array("multiple" => true));
Copy link

Choose a reason for hiding this comment

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

$params = array_merge($this->collectionOptions['update'], array("multiple" => false));

@redthor
Copy link

redthor commented Feb 10, 2012

Hi Ichi,
Not sure if you fixed the failed test cases...
I found that if I made these changes to the patch I could get all tests to pass:

line 112:
'insert' => array('safe' => true),

line 742:
$params = array_merge($this->collectionOptions['update'], array("multiple" => false));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants