Skip to content
This repository

return BLOB columns as Buffers instead of strings #124

Closed
wants to merge 1 commit into from

5 participants

Dusty Leary Felix Geisendörfer Ordr.in Mitchell Simoens Brian White
Dusty Leary

You rejected my previous request because I didn't add a test.

So now, I'm back, with a test :)

You were quite right to reject my previous patch, and I would have added a test previously if I had realized there was a test suite. Mea culpa.

My previous patch failed some of the already existing tests, even.

This new patch is a little more 'intrusive' to the code. I reshaped the way the ROW_PACKET_DATA buffers are handled. But, everything works for me locally, the tests now pass, and I added a new test to cover the BLOB functionality.

Felix Geisendörfer
Owner

Thanks, this looks good! I'll need a few days to review / land this so, things are crazy busy right now.

Ordr.in

Running into a problem with blobs and buffers and strings, oh my. Very happy to see that the wheels are already in motion!

Mitchell Simoens

What's the status of this? I dropped the query.js file directly into my app and it works great!

Brian White

Recreating the buffer on every chunk is inefficient. The total size is already known when the first chunk comes in (buffer.length + remaining). See this pull request: #52

Dusty Leary

My request doesn't recreate the buffer on every chunk... It stores the chunk buffers in a list and then coalesces them at the end.

I agree that the way #52 grabs the data looks nicer, I didn't know about the (buffer.length + remaining) invariant.

But, I doubt the commit as a whole can possibly be correct. I didn't try it out, but it looks like it's not checking for BINARY_FLAG (which I had to add a constant for), which would mean that it would return Buffers for TEXT fields as well as BLOB fields.

Brian White

You're right, it doesn't check for BINARY_FLAG, but it does restrict the field type to blobs only and should not affect any other fields (including TEXT).

Dusty Leary

I'm not a mysql protocol expert, so I could be wrong, but I think that there is no difference between BLOB and TEXT types except for the BINARY_FLAG.

For example, once you've identified field.type === FIELD_TYPE_MEDIUM_BLOB you must then check for the field.flags & BINARY_FLAG to see if it's "really" a BLOB or if it's actually "MEDIUM_TEXT" (which doesn't have a constant of its own).

I reached this conclusion when writing tests for my commit (I notice #52 has no added tests).

I used http://forge.mysql.com/wiki/MySQL_Internals_ClientServer_Protocol for reference... If this reference is incomplete and there are actual constants for MEDIUM_TEXT etc, then I concluded wrongly.

The test I wrote, "BLOB fields returned as Buffers" also exercises a text field. If you would run that against pull52, I suspect it would fail. If it doesn't, then I'll be quiet :)

Felix Geisendörfer
Owner
felixge commented May 15, 2012
Felix Geisendörfer felixge closed this May 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Sep 22, 2011
Dusty Leary return BLOB data as a Buffer instead of a string. test added 16dd2ed
This page is out of date. Refresh to see the latest.
133  lib/query.js
@@ -16,6 +16,48 @@ function Query(properties) {
16 16
 util.inherits(Query, EventEmitter);
17 17
 module.exports = Query;
18 18
 
  19
+function doTypeCast(text, fieldType) {
  20
+  switch (fieldType) {
  21
+    case Query.FIELD_TYPE_TIMESTAMP:
  22
+    case Query.FIELD_TYPE_DATE:
  23
+    case Query.FIELD_TYPE_DATETIME:
  24
+    case Query.FIELD_TYPE_NEWDATE:
  25
+      return new Date(text);
  26
+    case Query.FIELD_TYPE_TINY:
  27
+    case Query.FIELD_TYPE_SHORT:
  28
+    case Query.FIELD_TYPE_LONG:
  29
+    case Query.FIELD_TYPE_LONGLONG:
  30
+    case Query.FIELD_TYPE_INT24:
  31
+    case Query.FIELD_TYPE_YEAR:
  32
+      return parseInt(text, 10);
  33
+    case Query.FIELD_TYPE_FLOAT:
  34
+    case Query.FIELD_TYPE_DOUBLE:
  35
+      // decimal types cannot be parsed as floats because
  36
+      // V8 Numbers have less precision than some MySQL Decimals
  37
+      return parseFloat(text);
  38
+  }
  39
+  return text;
  40
+}
  41
+
  42
+function coalesceBuffers(buffers) {
  43
+  var totalSize = 0;
  44
+  for(var i=0, c=buffers.length; i<c; i++) {
  45
+    totalSize += buffers[i].length;
  46
+  }
  47
+  var bigBuffer = new Buffer(totalSize);
  48
+  for(var i=0, c=buffers.length, curOffset=0; i<c; i++) {
  49
+    buffers[i].copy(bigBuffer, curOffset);
  50
+    curOffset += buffers[i].length;
  51
+  }
  52
+  return bigBuffer;
  53
+}
  54
+
  55
+function fieldShouldBeReturnedAsBuffer(field) {
  56
+  var blob_or_text = 0 != (field.flags & Query.BLOB_FLAG);
  57
+  var binary = 0 != (field.flags & Query.BINARY_FLAG);
  58
+  return blob_or_text && binary;
  59
+}
  60
+
19 61
 Query.prototype._handlePacket = function(packet) {
20 62
   var self = this;
21 63
 
@@ -52,60 +94,49 @@ Query.prototype._handlePacket = function(packet) {
52 94
       }
53 95
       break;
54 96
     case Parser.ROW_DATA_PACKET:
55  
-      var row = this._row = {}, field;
  97
+      var row = {};
56 98
 
57  
-      this._rowIndex = 0;
  99
+      var rowIndex = 0;
  100
+      var cellBuffers = null;
58 101
 
59  
-      packet.on('data', function(buffer, remaining) {
60  
-        if (!field) {
61  
-          field = self._fields[self._rowIndex];
62  
-          row[field.name] = '';
63  
-        }
64  
-
65  
-        if (buffer) {
66  
-          row[field.name] += buffer.toString('utf-8');
67  
-        } else {
  102
+      function finishCell() {
  103
+        var field = self._fields[rowIndex];
  104
+        if(!cellBuffers) {
68 105
           row[field.name] = null;
  106
+        } else {
  107
+          //coalesce collected buffers
  108
+          var cell = coalesceBuffers(cellBuffers);
  109
+
  110
+          if(!fieldShouldBeReturnedAsBuffer(field)) {
  111
+            //if it's not a BLOB, convert to string and perhaps typecast
  112
+            cell = cell.toString('utf-8');
  113
+            if(self.typeCast) {
  114
+              cell = doTypeCast(cell, field.fieldType);
  115
+            }
  116
+          }
  117
+          row[field.name] = cell;
69 118
         }
70 119
 
71  
-        if (remaining !== 0) {
72  
-          return;
  120
+        //prepare for next field
  121
+        ++rowIndex;
  122
+        cellBuffers = null;
  123
+        if (rowIndex == self._fields.length) {
  124
+           self.emit('row', row);
73 125
         }
  126
+      }
74 127
 
75  
-        self._rowIndex++;
76  
-        if (self.typeCast && buffer !== null) {
77  
-          switch (field.fieldType) {
78  
-            case Query.FIELD_TYPE_TIMESTAMP:
79  
-            case Query.FIELD_TYPE_DATE:
80  
-            case Query.FIELD_TYPE_DATETIME:
81  
-            case Query.FIELD_TYPE_NEWDATE:
82  
-              row[field.name] = new Date(row[field.name]);
83  
-              break;
84  
-            case Query.FIELD_TYPE_TINY:
85  
-            case Query.FIELD_TYPE_SHORT:
86  
-            case Query.FIELD_TYPE_LONG:
87  
-            case Query.FIELD_TYPE_LONGLONG:
88  
-            case Query.FIELD_TYPE_INT24:
89  
-            case Query.FIELD_TYPE_YEAR:
90  
-              row[field.name] = parseInt(row[field.name], 10);
91  
-              break;
92  
-            case Query.FIELD_TYPE_FLOAT:
93  
-            case Query.FIELD_TYPE_DOUBLE:
94  
-              // decimal types cannot be parsed as floats because
95  
-              // V8 Numbers have less precision than some MySQL Decimals
96  
-              row[field.name] = parseFloat(row[field.name]);
97  
-              break;
  128
+      packet.on('data', function(buffer, remaining) {
  129
+        if (buffer) {
  130
+          cellBuffers = cellBuffers || [];
  131
+          cellBuffers.push(buffer);
  132
+          if(remaining === 0) {
  133
+            finishCell();
98 134
           }
  135
+        } else {
  136
+          cellBuffers = null;
  137
+          finishCell();
99 138
         }
100 139
 
101  
-        if (self._rowIndex == self._fields.length) {
102  
-           delete self._row;
103  
-           delete self._rowIndex;
104  
-           self.emit('row', row);
105  
-           return;
106  
-        }
107  
-
108  
-        field = null;
109 140
       });
110 141
       break;
111 142
   }
@@ -138,3 +169,17 @@ Query.FIELD_TYPE_BLOB        = 0xfc;
138 169
 Query.FIELD_TYPE_VAR_STRING  = 0xfd;
139 170
 Query.FIELD_TYPE_STRING      = 0xfe;
140 171
 Query.FIELD_TYPE_GEOMETRY    = 0xff;
  172
+
  173
+Query.NOT_NULL_FLAG          = 0x0001;
  174
+Query.PRI_KEY_FLAG           = 0x0002;
  175
+Query.UNIQUE_KEY_FLAG        = 0x0004;
  176
+Query.MULTIPLE_KEY_FLAG      = 0x0008;
  177
+Query.BLOB_FLAG              = 0x0010;
  178
+Query.UNSIGNED_FLAG          = 0x0020;
  179
+Query.ZEROFILL_FLAG          = 0x0040;
  180
+Query.BINARY_FLAG            = 0x0080;
  181
+Query.ENUM_FLAG              = 0x0100;
  182
+Query.AUTO_INCREMENT_FLAG    = 0x0200;
  183
+Query.TIMESTAMP_FLAG         = 0x0400;
  184
+Query.SET_FLAG               = 0x0800;
  185
+
27  test/slow/test-client-query.js
@@ -65,6 +65,33 @@ test('Long fields', function(done) {
65 65
   test(true);
66 66
 });
67 67
 
  68
+test('BLOB fields returned as Buffers', function(done) {
  69
+  this.client.query('USE '+common.TEST_DB, function useDbCb(err) {
  70
+    if (err) done(err);
  71
+  });
  72
+  this.client.query(
  73
+    'CREATE TEMPORARY TABLE '+common.TEST_TABLE+
  74
+    '(mytext TEXT, myblob BLOB)',
  75
+    function createTableCb(err) {
  76
+      if (err) done (err);
  77
+    }
  78
+  );
  79
+  this.client.query(
  80
+    'INSERT INTO '+common.TEST_TABLE+' '+
  81
+    'SET mytext = ?, myblob = ?',
  82
+    ['asdf', 'asdf']
  83
+  );
  84
+  this.client.query(
  85
+    'SELECT * FROM '+common.TEST_TABLE,
  86
+    function selectCb(err, results, fields) {
  87
+      if (err) done (err);
  88
+      assert.strictEqual(results[0].mytext, 'asdf');
  89
+      assert.strictEqual(typeof results[0].myblob, 'object');
  90
+      assert.strictEqual(results[0].myblob.toString('utf-8'), 'asdf');
  91
+    }
  92
+  );
  93
+});
  94
+
68 95
 test('Query a NULL value', function(done) {
69 96
   this.client.query('SELECT NULL as field_a, NULL as field_b', function(err, results) {
70 97
     assert.strictEqual(results[0].field_a, null);
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.