Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Base64 encode binary data before logging #177

Merged
merged 3 commits into from Mar 18, 2013

Conversation

Projects
None yet
2 participants
Contributor

bfeaver commented Mar 14, 2013

Both the logger and the data collector for the profiler would output raw
binary data. The logger would fail due to UTF-8 warnings when doing
json_encode and the profiler queries would fail in Twig when displaying
them in mongodb.html.twig.

Brian Feaver Base64 encode MongoBinData objects before logging.
Both the logger and the data collector for the profiler would output raw
binary data. The logger would fail due to UTF-8 warnings when doing
json_encode and the profiler queries would fail in Twig when displaying
them in mongodb.html.twig.
8e69509

@jmikola jmikola and 1 other commented on an outdated diff Mar 15, 2013

Tests/DataCollector/PrettyDataCollectorTest.php
@@ -37,15 +37,20 @@ public function getQueries()
{
return array(
// batchInsert
- array(
+ 'batch insert' => array(
@jmikola

jmikola Mar 15, 2013

Owner

Can you remove these array keys? This isn't used by PHPUnit and is redundant given the inline comments above.

@bfeaver

bfeaver Mar 15, 2013

Contributor

The array keys are used by PHPUnit to name the data set being used. In debug output or when a test fails, instead of having "data set #1" failed, it'll put out "data set "batch insert" failed".

However, if you don't want named datasets, I can remove the keys.

@jmikola

jmikola Mar 15, 2013

Owner

Oh, I had no idea. That sounds great, then 👍. If you like, we can drop the comments instead.

@jmikola jmikola and 1 other commented on an outdated diff Mar 15, 2013

DataCollector/PrettyDataCollector.php
@@ -216,7 +216,7 @@ private function bsonEncode($query, $array = true)
} elseif ($value instanceof \MongoMaxKey) {
$formatted = 'new MaxKey()';
} elseif ($value instanceof \MongoBinData) {
- $formatted = 'new BinData("'.$value->bin.'", "'.$value->type.'")';
+ $formatted = 'new BinData("'.base64_encode($value->bin).'", "'.$value->type.'")';
@jmikola

jmikola Mar 15, 2013

Owner

For printing BinData, we should be consistent with the Mongo shell here. Could you wrap the type and value argument positions? We can continue printing the type as a decimal number, despite the docs below saying it prints it in hex (I believe they need to be updated).

See: http://docs.mongodb.org/manual/reference/mongodb-extended-json/#data_binary

@bfeaver

bfeaver Mar 15, 2013

Contributor

Started this and realized I was a little confused. What did you want to wrap it with? It'll already currently print the decimal number, but if we want to output the hex value we could wrap it in dechex().

@jmikola

jmikola Mar 15, 2013

Owner

Sorry, I meant to say "swap", not "wrap". We can leave the type printed as-is. Moving it to the first argument would make this consistent with the Mongo shell.

@jmikola jmikola commented on the diff Mar 15, 2013

Logger/Logger.php
@@ -48,6 +48,12 @@ public function logQuery(array $query)
$query['data'] = '**'.$query['num'].' item(s)**';
}
+ array_walk_recursive($query, function(&$value, $key) {
+ if ($value instanceof \MongoBinData) {
+ $value = base64_encode($value->bin);
@jmikola

jmikola Mar 15, 2013

Owner

Shouldn't this also be logged using BinData format above? Otherwise, the base64 string will be indistinguishable from a regular string.

@bfeaver

bfeaver Mar 15, 2013

Contributor

I don't disagree. I was trying to remain consistent as no other types are currently treated special in this logger. I'll change it to use the same format as above.

@bfeaver

bfeaver Mar 15, 2013

Contributor

While changing this, I realized the output will end up looking a little strange since it will be wrapped in quotes since it's just being json_encoded.

It'll end up looking like {"foo":{"bar":"new BinData(\"anVuayBkYXRh\", \"2\")"}}.

If that's fine, then I'll leave it as is. Otherwise, maybe just a tag like "BINARY: anVuayBkYXRh, TYPE: 2"

@jmikola

jmikola Mar 15, 2013

Owner

Just out of curiosity, do other objects that get logged simply get casted to strings?

It's been some time since I saw the logger output for this bundle, so your example helped jog my memory.

I suppose the most preferable option would be to serialize the driver classes in the "strict JSON" format defined in: http://docs.mongodb.org/manual/reference/mongodb-extended-json/#data_binary (the mongoexport commands use that notation), but that would be outside the scope of this PR. Let's leave it as-is for now, then.

@jmikola jmikola added a commit that referenced this pull request Mar 18, 2013

@jmikola jmikola Merge pull request #177 from sellingsource/master
Base64 encode binary data before logging
00568cc

@jmikola jmikola merged commit 00568cc into doctrine:master Mar 18, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment