nodejs-datastore
nodejs-datastore copied to clipboard
googleapis
Saving an empty Buffer throws an error.
Environment details
- OS: MacOs
- Node.js version: v14.4.0
- npm version: 6.14.8
@google-cloud/datastoreversion: 6.3.0
Steps to reproduce
Execute the following code:
async function saveBlob() {
const key = datastore.key(['TEST']);
await datastore.save({
key: key,
data: {
name: 'test',
blob: Buffer.from([])
},
});
}
It will trigger an error: Error: 3 INVALID_ARGUMENT: The value "blob" does not contain a value.
I expect to be able to save an empty Buffer without error.
If you change the blob value to Buffer.from([1, 2, 3]) the error disappears.
My use case is that I have a protocol buffer composed of repeated fields only:
message LiveTrack {
optional int64 id = 1 [jstype = JS_NUMBER];
optional string name = 2;
repeated float lat = 3;
repeated float lon = 4;
repeated int32 alt = 5;
repeated int32 time_sec = 6;
repeated uint32 flags = 7;
map<uint32, LiveExtra> extra = 8;
}
It could happen that all repeated fields (including the map) are empty. Because the optional field are not populated, the proto would serialize to an empty Buffer.
Workaround
The workaround would be to set the value to null when the Buffer size is 0.
This requires extra code both while saving (to replace the Buffer with null) and while loading (to replace null with the default message).
Other
The wording of the message could be improved: The value "blob" does not contain a value., i.e. use field or key instead of the first "value".
EDIT: The actual issue in protobuf-js, while this should also resolve the issue it probably better to fix protobufjs (see below for details)
I have update encodeValue to read like:
if (value instanceof Buffer) {
// was valueProto.blobValue = buffer;
valueProto.blobValue = value.length == 0 ? '' : value;
return valueProto;
}
And it fixes the issue:
- The column is save as a Blob without vale (verified in the Cloud Console),
- Reading the column returns an empty buffer.
Anyway this code looks like it fixes the issue.
An other way to fix would be valueProto.blobValue = buffer.toString('base64');.
I don't know the code well enough to determine if this is the best possible fix. What do you think ?
I have done some digging into this bug:
Calling datastore.save(...) ends up @grpc/proto-loader createSerializer
function createSerializer(cls: Protobuf.Type): Serialize<object> {
return function serialize(arg: object): Buffer {
const message = cls.fromObject(arg);
return cls.encode(message).finish() as Buffer;
};
}
The problems seems to be in const message = cls.fromObject(arg);
The arg is:
mode:'NON_TRANSACTIONAL'
mutations:(1) [
0: {
upsert:{
key:{path: Array(1)},
properties:{
name: {stringValue: 'test-null'},
track: {blobValue: Buffer(0), excludeFromIndexes: true}
}
}
]
But the message variable has lost the value:
upsert: Entity {
key:Key {path: Array(1)}
properties: {
name:Value {stringValue: 'test-null'}
track:Value {excludeFromIndexes: true}
}
}
There should be a blobValue: Buffer(0) for the track.
When the Buffer is not empty the message variable would look like:
upsert:Entity {
key:Key {path: Array(1)}
properties:{
name:Value {stringValue: 'test-null'}
track:Value {blobValue: Buffer(1), excludeFromIndexes: true}
}
}
Here there is a blobValue for the track which is the expected result.
It seems like this might be related to:
- https://github.com/protobufjs/protobuf.js/pull/1500
- https://github.com/protobufjs/protobuf.js/issues/885
Patching protobufjs/protobuf.js#1500 did resolve the issue.
I'll try to move that PR forward and then propagate the fix through the dependencies.
See https://github.com/protobufjs/protobuf.js/pull/1514#pullrequestreview-545366600, the proper fix in protobuf.js should be in v7
The current nodejs-datastore code uses a workaround that could be removed once protobuf.js is fixed - that v7 is released and nodejs-datastore is updated to use this dep.
/cc @alexander-fenster
Update v7.0 of protobufjs is staged but not relased.
https://github.com/protobufjs/protobuf.js/pull/1519
https://github.com/protobufjs/protobuf.js/releases shows releases for 7.x+ — checking to see if we've picked these up yet.
The current minimum version of google-gax required is 3.3.0, and it looks like protobufjs v7 was required in v3.1.4, so we should be good to go to unwind this workaround. I've removed the "external" label accordingly.
