Clean up some serializer stuff. #1732

mikelehen · 2019-04-29T23:20:47Z

Sorry, I decided to review #1669 in the form of a PR. 😄 Thanks for doing the hard work to get protobufjs upgraded and the tests working again. This is basically a cleanup pass to convince myself that everything was working right and simplify the tests. In particular:

The switch to protobufjs 6.x seems to have removed the need for hasTag() (and actually it had broken our usage of it since the tags ended up changing from snake_case to camelCase so they never matched). So I've removed hasTag() and set oneofs: false in protoLoaderOptions so the virtual "fooType" fields are no longer added at all.
'seconds' in timestamps can and should always be serialized as a string, since it's an int64 value but we weren't doing that. I fixed it, which removed some of the special casing in your roundtrip testing.
I reworked the expect...RoundTrip methods into a single unified one. I think this is simpler (and reduced some repetition) but if you have concerns, let me know.
I re-added the disabled useProto3Json assert that you had removed from serializer.ts and instead tweaked the offending test so it doesn't try to round-trip through protobufjs.

Add app to master list of components in `package.json`

…1204)" (#1723) This reverts commit e50b070.

v5.10.1

1. The switch to protobufjs 6.x seems to have removed the need for hasTag() (and actually it had broken our usage of it since the tags ended up changing from snake_case to camelCase). So I've removed hasTag() and set `oneofs: false` in protoLoaderOptions so the virtual "fooType" fields are no longer added. 2. 'seconds' in timestamps should always be serialized as a string, since it's an int64 but we weren't doing that. I fixed it, which removed some of the special casing from rsgowman/grpc_load. 3. I reworked the expect...RoundTrip methods into a single unified one which saves some repetition and I think is easier to make sense of. 4. I re-added the disabled useProto3Json assert and instead tweaked the offending test not to try to round-trip through protobufjs.

rsgowman · 2019-04-30T15:36:01Z

packages/firestore/src/remote/serializer.ts

-    // tslint:disable-next-line:no-any
-    const type = (obj as any)['value_type'];
-    if (hasTag(obj, type, 'nullValue')) {
+    if ('nullValue' in obj) {


By just looking for the presence of fooValue in obj, rather than looking at value_type, we leave ourselves open to a proto message containing (eg) both nullValue and booleanValue (or any other combination that you like), implying that if we receive a corrupt proto, we'll end up (attempting to) parse it as whatever type comes first in this list. (Or will protobufjs notice that this is a oneof field and throw an error or otherwise do something intelligent?)

I don't have any objection to this; but thought it worth pointing out.

I'd consider it a bug in protobufjs to have more than one value for a one_of field and I don't think we can gracefully handle it. If we wanted to be paranoid we could try to detect / throw, but I'm not sure if using value_type in that case would be a particularly better behavior, so I am inclined to leave it as-is.

For a bit of added context in case it's not clear:

JSON protos received via WebChannel (i.e. useProto3Json=true) never had value_type, so they were subject to this same potential corruption.

Prior to protobufjs 6.x, one_of fields would contain all of the values and so using value_type was the only way to tell which was the actual value. That's fixed now, hence no longer a need to use value_type.

The hasTag() implementation was a pretty big wart to gracefully handle both protobufJs and proto3Json. I think getting rid of it is a big improvement. :-)

rsgowman · 2019-04-30T17:55:50Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      const roundTripProto: api.Value = ValueMessage.toObject(
-        ValueMessage.create({ [tag]: messageField }),
-        protoLoaderOptions
+    function verifyFieldValueRoundTrip(opts: {


So we talked about separating these concerns into multiple tests... which I didn't quite do. I had a single test for each type that called 3 helpers (rather than 3 completely separate tests). This is going a step further and merging all three things into a single helper. But those 3 things are obviously very closely related, and I do like the results.

However, the protobufJsValue seems a little de-emphasized. It's a bit of an odd thing to test, since it's really protobufjs's responsibility to encode the json proto into something that can be sent over the wire. Calling this out more explicitly would be helpful. The best way would probably be a separate test (or at least separate helper) but making this more explicit in the docstring might be sufficient too.

Yeah, I actually have slightly mixed feelings over whether we really are benefiting from testing the protobufJs values, so de-emphasizing it was perhaps slightly intentional. :-) But I do think a better comment is a good idea. Done.

rsgowman · 2019-04-30T18:33:46Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      );
-      expectFullRoundTrip(value, serializer);
+      const obj = serializer.toValue(new fieldValue.BlobValue(example));
+      expect(obj).to.deep.equal(expected);


Undoing the call to verifyFieldValueRoundTrip essentially means we never test the round trip of a fieldValue.BlobValue with proto3. Is this ok? (I'm not sure how/where we use proto3json, so I wasn't sure.)

Oh, that's a fair point. I thought I was reverting this to the state in master but I forgot there's a "converts BlobValue from string" test in master that was removed in your PR... That said, I think your PR wasn't testing the full round-trip either, since it included round-tripping through protobufJS which yields a UInt8Array instead of base64 string (and this is why you had to remove that assert in serializer.ts).

So I tweaked verifyFieldValueRoundTrip() to accept a useProto3Json option that uses the proto3 JSON serializer and skips the round-trip through protobufJS.

mikelehen

Thanks for the feedback! Made some tweaks. Let me know what you think.

mikelehen · 2019-04-30T19:57:26Z

packages/firestore/src/remote/serializer.ts

-    // tslint:disable-next-line:no-any
-    const type = (obj as any)['value_type'];
-    if (hasTag(obj, type, 'nullValue')) {
+    if ('nullValue' in obj) {


I'd consider it a bug in protobufjs to have more than one value for a one_of field and I don't think we can gracefully handle it. If we wanted to be paranoid we could try to detect / throw, but I'm not sure if using value_type in that case would be a particularly better behavior, so I am inclined to leave it as-is.

For a bit of added context in case it's not clear:

JSON protos received via WebChannel (i.e. useProto3Json=true) never had value_type, so they were subject to this same potential corruption.

Prior to protobufjs 6.x, one_of fields would contain all of the values and so using value_type was the only way to tell which was the actual value. That's fixed now, hence no longer a need to use value_type.

The hasTag() implementation was a pretty big wart to gracefully handle both protobufJs and proto3Json. I think getting rid of it is a big improvement. :-)

mikelehen · 2019-04-30T20:19:35Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      const roundTripProto: api.Value = ValueMessage.toObject(
-        ValueMessage.create({ [tag]: messageField }),
-        protoLoaderOptions
+    function verifyFieldValueRoundTrip(opts: {


Yeah, I actually have slightly mixed feelings over whether we really are benefiting from testing the protobufJs values, so de-emphasizing it was perhaps slightly intentional. :-) But I do think a better comment is a good idea. Done.

mikelehen · 2019-04-30T22:12:32Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      );
-      expectFullRoundTrip(value, serializer);
+      const obj = serializer.toValue(new fieldValue.BlobValue(example));
+      expect(obj).to.deep.equal(expected);


Oh, that's a fair point. I thought I was reverting this to the state in master but I forgot there's a "converts BlobValue from string" test in master that was removed in your PR... That said, I think your PR wasn't testing the full round-trip either, since it included round-tripping through protobufJS which yields a UInt8Array instead of base64 string (and this is why you had to remove that assert in serializer.ts).

So I tweaked verifyFieldValueRoundTrip() to accept a useProto3Json option that uses the proto3 JSON serializer and skips the round-trip through protobufJS.

mikelehen · 2019-05-01T18:34:47Z

Manually merged (with history) to rsgowman/grpc_load.

hsubox76 and others added 6 commits April 24, 2019 10:35

Add app to component list (#1719)

2337089

Add app to master list of components in `package.json`

Fix the type annotations in the RTDB Query.on/off/once API. (#1204)

e50b070

Revert "Fix the type annotations in the RTDB Query.on/off/once API. (#…

b6bfae0

…1204)" (#1723) This reverts commit e50b070.

Publish [email protected]

ea6bd60

Merge branch 'release'

366713a

v5.10.1

mikelehen added the api: firestore label Apr 29, 2019

mikelehen requested a review from rsgowman April 29, 2019 23:20

mikelehen assigned rsgowman Apr 29, 2019

rsgowman reviewed Apr 30, 2019

View reviewed changes

rsgowman assigned mikelehen and unassigned rsgowman Apr 30, 2019

CR Feedback.

6bb2b6d

mikelehen commented Apr 30, 2019

View reviewed changes

mikelehen assigned rsgowman and unassigned mikelehen Apr 30, 2019

Merge branch 'master' into mikelehen/grpc_load-tweaks

0ad8778

mikelehen requested review from bojeil-google, davideast, dwoffinden, Feiyang1, hiranya911, hsubox76, jamesdaniels, mmermerkaya, pinarx, ryanpbrewster and wti806 as code owners May 1, 2019 18:33

mikelehen merged commit 0ad8778 into rsgowman/grpc_load May 1, 2019

mikelehen deleted the mikelehen/grpc_load-tweaks branch May 1, 2019 18:34

firebase locked and limited conversation to collaborators Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up some serializer stuff. #1732

Clean up some serializer stuff. #1732

Uh oh!

mikelehen commented Apr 29, 2019

Uh oh!

rsgowman Apr 30, 2019

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

rsgowman Apr 30, 2019

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

rsgowman Apr 30, 2019

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

mikelehen left a comment

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

mikelehen Apr 30, 2019

Uh oh!

mikelehen commented May 1, 2019

Uh oh!

Uh oh!

Clean up some serializer stuff. #1732

Clean up some serializer stuff. #1732

Uh oh!

Conversation

mikelehen commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen commented May 1, 2019

Uh oh!

Uh oh!