-
Notifications
You must be signed in to change notification settings - Fork 269
PHPLIB-242: Refactor command operations to use field path syntax for type maps #658
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
Conversation
d68d5d2
to
6445c3a
Compare
src/functions.php
Outdated
*/ | ||
function create_field_path_type_map(array $typeMap, $fieldPath) | ||
{ | ||
if (isset($typeMap['root']) && ! isset($typeMap['fieldPaths'][$fieldPath])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we skip this just because the user has specified both a custom root
type and happens to have specified our internal field path in their own fieldPaths
option?
For instance, if we're using this with FindAndModify and the user specified a custom type for the value
field path, I imagine we should just move that to the value.value
field path. And independent of that, whatever the user specifies for root
can always can always be assigned to $typeMap['fieldPaths'][$fieldPath]
at the end (as you do later in the function).
I wonder if it'd make sense to just handle these independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. The helper function now always nests all pre-existing field paths to prefix them with the given field path. It also applies an object
type for the root
key, and an array
type for the array
key if the given field path ends in .$
. This was added because the Aggregate, Distinct, and MapReduce operations expect an array of results instead of an iterator. We may always optimise this further down the road by returning an iterator directly instead of wrapping the results after the call. For now, this works just fine though.
3f35d48
to
2071922
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question
} | ||
|
||
if ( ! is_object($result->value)) { | ||
throw new UnexpectedValueException('findAndModify command did not return a "value" document'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we no longer need to throw this exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findAndModify
will either return a document or null
depending on the operation and matched values. We're guarding against a missing value
property in the return statement, but from what I can see we don't need to guard against non-object values being returned from findAndModify
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This adds a new internal function to modify a type map given to a command and add a
fieldPath
type for the given path. This is used in theAggregate
,Distinct
,FindAndModify
, andMapReduce
operations to avoid manually applying the type map to the results. Any pre-existing field paths are prefixed with the internal field path to ensure consistent results. If a user already passes a type for the field path for the given operation, it is not overwritten as I assume the user "knows what they are doing". The root type is always set toobject
; otherwise the user would be able to break internal functionality by changing the root type. I'm not sure whether the name of the function is appropriate to convey what the function is doing; I'm open to suggestions in this regard.