-
Notifications
You must be signed in to change notification settings - Fork 208
PHPC-2350: Implement JSON methods for PackedArray #1645
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
a14469f
to
3e59ebe
Compare
var_dump($bson); | ||
hex_dump((string) $bson); | ||
echo $bson->toCanonicalExtendedJSON(), PHP_EOL; | ||
echo MongoDB\BSON\Document::fromPHP(['array' => $bson])->toCanonicalExtendedJSON(), PHP_EOL; |
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.
Note that since
bson_new_from_json
was intended to read documents, the resulting PackedArray instance will retain the original keys in the object, but iterating it or converting it to JSON will reindex the array. However, when using the PackedArray instance in certain ways will cause errors.
I'm having trouble understanding why this fails but the preceding $bson->toCanonicalExtendedJSON()
does not.
Does bson_json_opts_set_outermost_array()
apply some logic that disregards invalid keys, while the code path for converting a nested array to extended JSON does not?
The only option I see is to iterate the resulting bson_t and either re-index it (essentially re-writing a bson_t array) or to throw if one of the keys isn't what we expect. Since we have the mantra of "be lenient in what you accept, but strict in what you produce" I'm inclined to go with the first option, despite this potentially causing some overhead.
If PackedArray::fromPHP() requires a well-formed list (see: bson-packedarray-fromPHP_error-001.phpt), I don't see why we can't require that in fromJSON()
as well.
Perhaps we can't prevent libbson from accepting a JSON object with sequential numeric string keys starting at zero, but if it's easy to detect something like { "foo": "bar" }
I'm all for throwing an error there.
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.
I added logic to iterate the array and compare the keys. I've used strtoll
to parse the string, as I didn't want to allocate memory to convert the expected key to a string. I'm not sure if that has any performance implications, but I don't expect this method to be somewhere in the hot path given its limited usefulness.
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.
Recapping our Slack discussion, we discussed a caveat with strtoll()
accepting non-canonical numeric strings. I'll review the second approach.
3e59ebe
to
9b020ac
Compare
PHPC-2350
We can leverage
bson_new_from_json
to create abson_t
for an array, andbson_as_json_with_opts
lets us export thebson_t
to JSON with an array being the root object.Note that since
bson_new_from_json
was intended to read documents, the resultingPackedArray
instance will retain the original keys in the object, but iterating it or converting it to JSON will reindex the array. However, when using thePackedArray
instance in certain ways will cause errors (seebson-packedarray-fromJSON-003.phpt
which fails the last operation). The only option I see is to iterate the resultingbson_t
and either re-index it (essentially re-writing abson_t
array) or to throw if one of the keys isn't what we expect. Since we have the mantra of "be lenient in what you accept, but strict in what you produce" I'm inclined to go with the first option, despite this potentially causing some overhead. WDYT @jmikola?