fix: support byte values in Bundles #1395

wu-hui · 2021-01-24T20:29:07Z

No description provided.

…d json output.

codecov · 2021-01-24T22:24:58Z

Codecov Report

Merging #1395 (187650b) into master (fa6721f) will decrease coverage by 0.00%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
- Coverage   98.51%   98.50%   -0.01%     
==========================================
  Files          32       32              
  Lines       19392    19397       +5     
  Branches     1273     1362      +89     
==========================================
+ Hits        19104    19107       +3     
- Misses        284      286       +2     
  Partials        4        4

Impacted Files	Coverage Δ
dev/src/document.ts	`98.50% <0.00%> (-0.19%)`	⬇️
dev/src/bundle.ts	`94.44% <100.00%> (+0.12%)`	⬆️
dev/src/timestamp.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa6721f...676be69. Read the comment docs.

schmidt-sebastian · 2021-01-25T16:22:43Z

dev/src/bundle.ts

+    const message = BundleElement.fromObject(bundleElement).toJSON();
+    const buffer = Buffer.from(JSON.stringify(message), 'utf-8');


Let's talk about this offline later, but this is quite an expensive operation. The steps are:

Manually serialize documents in ProtobufJS format

Read elements back Protobuf JS and convert to Proto3 JSON

Parse Proto3 JSON into string

This seems like an okay "quick fix", but not the most elegant solution. We should figure out whether we are ok with this quick fix or whether we should directly produce Proto3 JSON. We could use a similar approach as the Web SDK and use different serialization options in our serializer depending on the target. While this might end up being a fair amount of work, it also helps us with the timestamp conversion.

Furthermore, can you verify whether the current conversion correctly translates numbers greater than 2^53 bits? You would have to turn on useBigInt and then write a large integer value like such: 2147483647n. I am asking because the JSON roundtrip might convert this type to a lossy floating point representation.

Added a test for BigInt.

schmidt-sebastian · 2021-01-27T18:36:45Z

dev/src/bundle.ts

@@ -41,7 +42,6 @@ export class BundleBuilder {
  private latestReadTime = new Timestamp(0, 0);

  constructor(readonly bundleId: string) {}
-


I think you should add this back.

schmidt-sebastian · 2021-01-27T18:39:26Z

dev/system-test/firestore.ts

+import BundledDocumentMetadata = firestore.BundledDocumentMetadata;
 import IBundleElement = firestore.IBundleElement;
+import NamedQuery = firestore.NamedQuery;
+import Document = google.firestore.v1.Document;


Can you move these imports to a different "section" (separated by a blank space below)?

schmidt-sebastian · 2021-01-27T19:25:16Z

dev/system-test/firestore.ts

+            structuredQuery: query.toProto().structuredQuery,
+          }
+        ),
+      }).toJSON()


Do we have to do this round-trip here (and below)? Can we not just compare against the resulting JSON directly?

The trick here is the expected object we use here will have int64 encoded as number, while the actual object we get from bundles have them encoded as strings, for example.

I have created some utility functions to help with the boilerplate. Unfortunately i cannot figure out how to make them generic, because fromObject is static, and returns different types.

Is it not possible to manually create the desired Proto output? Would it be possible to write expect(...).to.deep.equal({integerValue:'1337'})) instead of expect(...).to.deep.equal(fromJson({integerValue:1337}).toJSON()))?

This works for the example you give. But the tests deal a lot with timestamps, and expect(...).to.deep.equal(snap.readTime.toProto.timestampValue) will compare encoded string seconds to the actual integer value. So you get tests failures like:

- "seconds": "1577836805" + "seconds": 1577836805

schmidt-sebastian · 2021-02-02T19:42:50Z

dev/test/bundle.ts

  }
  expect(meta.id).to.equal(TEST_BUNDLE_ID);
  expect(meta.version).to.equal(TEST_BUNDLE_VERSION);
  expect(meta.totalDocuments).to.equal(totalDocuments);
-  expect(meta.createTime).to.deep.equal(createTime);
+  expect(google.protobuf.Timestamp.fromObject(meta.createTime!)).to.deep.equal(


Fix for this: https://gist.github.com/schmidt-sebastian/3f53054d1e1c2d8238e473bfc133a1de

schmidt-sebastian

Please update the tests to not use the JSON helpers as it hides bugs in the JSON creation.

dev/src/bundle.ts

schmidt-sebastian · 2021-02-03T18:46:27Z

dev/system-test/firestore.ts

@@ -43,12 +43,14 @@ import {
  postConverterMerge,
  verifyInstance,
 } from '../test/util/helpers';
-import IBundleElement = firestore.IBundleElement;


Thanks for re-ordering. Can you remove the empty line?

schmidt-sebastian · 2021-02-03T18:50:11Z

dev/test/bundle.ts

@@ -37,17 +38,17 @@ export function verifyMetadata(
  expectEmptyContent = false
 ): void {
  if (!expectEmptyContent) {
-    expect(meta.totalBytes).greaterThan(0);
+    expect(parseInt(meta.totalBytes!.toString())).greaterThan(0);


I think you can probably emit toString() here (totalBytes should be a string).

It's there to silent the type error, it can be number or string.

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

JustinBeckwith

Rubber stamp because I broke CODEOWNERs

Fix: Use protobuf.js toJson before calling JSON.stringify to get vali…

d0ad604

…d json output.

wu-hui requested review from a team as code owners January 24, 2021 20:29

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 24, 2021

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jan 24, 2021

wu-hui added 3 commits January 24, 2021 15:30

Fix wording

4bdf6bc

Fix other tests by comparing to json representations.

f03ad44

Fix more tests by comparing to json representations

058f6c9

wu-hui requested a review from schmidt-sebastian January 25, 2021 14:09

wu-hui assigned schmidt-sebastian Jan 25, 2021

schmidt-sebastian reviewed Jan 25, 2021

View reviewed changes

wu-hui added 2 commits January 25, 2021 17:12

Add test to verify BigInt gets encoded correctly.

100b897

make the number a bit larger.

528debd

schmidt-sebastian reviewed Jan 27, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jan 27, 2021

Nicer tests

4f1886f

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jan 28, 2021

Feedback

4f2444d

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jan 29, 2021

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 1, 2021

schmidt-sebastian reviewed Feb 2, 2021

View reviewed changes

schmidt-sebastian suggested changes Feb 2, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Feb 2, 2021

Fix toDocumentProto not using ITimestamp.

f1c546b

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 3, 2021

schmidt-sebastian changed the title ~~Fix: Use protobuf.js toJson to get valid bundle output~~ fix: use protobuf.js toJson to get valid bundle output Feb 3, 2021

schmidt-sebastian changed the title ~~fix: use protobuf.js toJson to get valid bundle output~~ fix: support byte values in Bundles Feb 3, 2021

schmidt-sebastian approved these changes Feb 3, 2021

View reviewed changes

schmidt-sebastian removed their assignment Feb 3, 2021

wu-hui and others added 4 commits February 4, 2021 15:55

Update dev/src/bundle.ts

31ee347

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Update dev/src/bundle.ts

62ff367

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Minor fix.

7a6ff29

Merge branch 'master' into wuandy/BundlesFixJsonOutput

676be69

JustinBeckwith approved these changes Feb 4, 2021

View reviewed changes

wu-hui merged commit 8cf53a9 into master Feb 4, 2021

wu-hui deleted the wuandy/BundlesFixJsonOutput branch February 4, 2021 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support byte values in Bundles #1395

fix: support byte values in Bundles #1395

wu-hui commented Jan 24, 2021

codecov bot commented Jan 24, 2021 •

edited

Loading

schmidt-sebastian Jan 25, 2021

wu-hui Jan 25, 2021

schmidt-sebastian Jan 27, 2021

wu-hui Jan 29, 2021

schmidt-sebastian Jan 27, 2021

wu-hui Jan 28, 2021

schmidt-sebastian Jan 27, 2021

wu-hui Jan 28, 2021

schmidt-sebastian Jan 29, 2021

wu-hui Feb 1, 2021

schmidt-sebastian Feb 2, 2021

wu-hui Feb 3, 2021

schmidt-sebastian left a comment

schmidt-sebastian Feb 3, 2021

wu-hui Feb 4, 2021

schmidt-sebastian Feb 3, 2021

wu-hui Feb 4, 2021

JustinBeckwith left a comment

		const message = BundleElement.fromObject(bundleElement).toJSON();
		const buffer = Buffer.from(JSON.stringify(message), 'utf-8');

		@@ -41,7 +42,6 @@ export class BundleBuilder {
		private latestReadTime = new Timestamp(0, 0);

		constructor(readonly bundleId: string) {}

fix: support byte values in Bundles #1395

fix: support byte values in Bundles #1395

Conversation

wu-hui commented Jan 24, 2021

codecov bot commented Jan 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinBeckwith left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2021 •

edited

Loading