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

Apply primary readPreference to certain commands #187

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

alcaeus
Copy link
Owner

@alcaeus alcaeus commented Aug 19, 2017

Fixes #181.

@alcaeus alcaeus added the bug label Aug 19, 2017
@alcaeus alcaeus added this to the 1.1.3 milestone Aug 19, 2017
@alcaeus alcaeus force-pushed the commands-apply-primary-readpreference branch from ac689fc to e2c5e65 Compare August 19, 2017 19:59
private function supportsReadPreference()
{
if ($this->command === []) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this merely guarding against the assumption below that there is at least one key in the command document?

Is the command document always a PHP array here, or might the user have supplied a stdClass? ext-mongo uses PHP's Hash API to access keys and would work with either.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point about the object - I'll have to check that. I've only used arrays, but it's worth adding a test against objects as well 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

I took a look and I typehinted array in the constructor of MongoCommandCursor, although I might have to loosen that up depending on what ext-mongo does. For now, the check against an empty array works.


case 'mapreduce':
case 'mapReduce':
return !isset($this->command['out']) || isset($this->command['out']['inline']);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're mimicking ext-mongo exactly (here), this should only return true when out is a PHP array and inline exists as an array key.

For what it's worth, both of ext-mongo's checks are flawed. Ideally, out could be either an array or object and inline should probably have a true value (in the BSON sense, not PHP -- although that really depends on the server's mapReduce handling).

Copy link
Owner Author

Choose a reason for hiding this comment

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

I noticed the original check was flawed. Isn't omitting out the same as providing out with inline set? In that case, the absence of out would indicate the command supports readPreference since it's not required to run on a primary server to write results to a new collection.

That's why the check was written this way - when out isn't set or when inline is set (and ideally set to true, I agree on that) the command supports a readPreference. In all other cases we need to enforce a primary readPreference on the command.

Copy link
Contributor

@jmikola jmikola Sep 21, 2017

Choose a reason for hiding this comment

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

Isn't omitting out the same as providing out with inline set?

Omitting out results in the following server error on 3.4: "out has to be a string or an object".

Moreover, the server doesn't seem to care about the value of inline, so even { inline: false } is enough to produce an inline result.

So, if you'd like to mimic ext-mongo exactly, this can be changed to:

return (isset($this->command['out']) &&
        is_array($this->command['out']) &&
        array_key_exists('inline', $this->command['out']));

And if you'd like a robust approach:

$out = isset($this->command['out']) ? $this->command['out'] : null;

return (is_array($out) || is_object($out)) && array_key_exists('inline', (array) $out);

Copy link
Owner Author

@alcaeus alcaeus Sep 22, 2017

Choose a reason for hiding this comment

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

I've updated the code to mimic ext-mongo, even if it may be flawed. Thanks for your help!

@alcaeus alcaeus force-pushed the commands-apply-primary-readpreference branch from e2c5e65 to 28af2b0 Compare September 21, 2017 04:57
@alcaeus alcaeus force-pushed the commands-apply-primary-readpreference branch from f09376c to ca46570 Compare September 22, 2017 06:23
@alcaeus alcaeus merged commit c139c0f into master Sep 22, 2017
@alcaeus alcaeus deleted the commands-apply-primary-readpreference branch September 22, 2017 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants