feat: Add totalDocuments and totalBytes to bundle metadata. #1085

wu-hui · 2020-05-19T15:55:37Z

No description provided.

codecov · 2020-05-19T15:59:38Z

Codecov Report

Merging #1085 into node10 will increase coverage by 0.00%.
The diff coverage is 95.32%.

@@           Coverage Diff           @@
##           node10    #1085   +/-   ##
=======================================
  Coverage   98.62%   98.63%           
=======================================
  Files          28       28           
  Lines       17944    18009   +65     
  Branches     1394     1399    +5     
=======================================
+ Hits        17698    17763   +65     
  Misses        243      243           
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/index.ts	`98.18% <93.75%> (ø)`
dev/src/bundle.ts	`93.92% <93.92%> (+0.11%)`	⬆️
dev/src/reference.ts	`99.88% <98.61%> (+<0.01%)`	⬆️
dev/src/document.ts	`98.76% <100.00%> (ø)`
dev/src/write-batch.ts	`100.00% <100.00%> (ø)`
dev/src/types.ts	`99.72% <0.00%> (+0.01%)`	⬆️
dev/src/transaction.ts	`97.02% <0.00%> (+0.02%)`	⬆️
dev/src/bulk-writer.ts	`98.82% <0.00%> (+0.03%)`	⬆️
... and 2 more

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 19b8922...6905f59. Read the comment docs.

schmidt-sebastian · 2020-05-19T16:17:59Z

dev/src/bundle.ts

+    bundleBuffer = Buffer.concat([
+      this.elementToLengthPrefixedBuffer({metadata}),
+      bundleBuffer,
+    ]);


I am not sure I understand how this will work. I assume this was added to allow the progress handler to display their status. For that to work, would this not need to be the first message?

Note the order of the arguments to Buffer.concat is different here. Also added a comment.

schmidt-sebastian · 2020-05-21T16:25:57Z

dev/src/bundle.ts

+      totalDocuments: this.documents.size,
+      totalBytes: bundleBuffer.length,
+    };
+    // Prepends the metadata element to the bundleBuffer, note `bundleBuffer` is the second argument to `Buffer.concat`.


s/, note/:/

// Prepends the metadata element to the bundleBuffer: `bundleBuffer` is the second argument to `Buffer.concat`.

schmidt-sebastian · 2020-05-21T16:27:03Z

dev/test/bundle.ts

@@ -80,11 +80,14 @@ describe('Bundle Buidler', () => {
    expect(elements.length).to.equal(3);

    const meta = (elements[0] as IBundleElement).metadata;
+    expect(meta!.totalBytes).greaterThan(0);


Is the size not fixed here in the unit tests? Or will that make the tests too fragile?

Yes, I started with checking for exact size, but it became too fragile.

schmidt-sebastian · 2020-05-21T16:27:50Z

dev/test/bundle.ts

@@ -128,11 +131,14 @@ describe('Bundle Buidler', () => {
    expect(elements.length).to.equal(4);

    const meta = (elements[0] as IBundleElement).metadata;


Do you mind adding a helper for the metadata comparison?

schmidt-sebastian · 2020-05-21T17:21:45Z

dev/test/bundle.ts

+  meta: IBundleMetadata,
+  expected: IBundleMetadata
+): void {
+  expect(meta!.totalBytes).greaterThan(0);


schmidt-sebastian · 2020-05-21T17:22:31Z

dev/test/bundle.ts

+
+export function verifyMetadata(
+  meta: IBundleMetadata,
+  expected: IBundleMetadata


I was hoping you would pass in the expected state one at a time:

export function verifyMetadata( meta: IBundleMetadata, id: string, createTime: ReadTime, version: number, totalDocuments: 0 )

From that, you can simplify further. Extract a constant for the bundle ID, and use it everywhere. The version is also always a constant. So in the end you have:

export function verifyMetadata( meta: IBundleMetadata, createTime: ReadTime, totalDocuments: 0 )

You can then also compare each field directly:

expect(meta.totalBytes).greaterThan(0); expect(meta.id).to.equal(BUNDLE_ID); expect(meta.version).to.equal(1); expect(meta.createTime).to.deep.equal(readTime.toProto()); expect(meta.totalDocuments).to.equal(totalDocuments);

This removes the need to delete the field and ruins nasty surprises in the future.

schmidt-sebastian

Very nice! Thanks!

If you want to spend more time on this, I'd suggest prefixing the "totalDocuments" argument with /* totalDocuments= */ for better readability. But even without this, this is good to go.

* fix!: mark v1beta1 client as deprecated (#937) * feat!: use QueryDocumentSnapshot in FirestoreDataConverter (#965) * deps: update to gts 2.x (#1013) * chore!: update settings for Node 10 (#1019) * deps: drop through2 (#1014) * feat: support BigInt (#1016) * fix: make update.sh work on Linux (#1043) * fix: only use BigInt in BigInt system test (#1044) * fix: make pbjs compile admin proto again (#1045) * Add BulkWriter (#1055) * docs: Add documentation for FirestoreDataConverter (#1059) * chore: enforce return types (#1065) * fix: add generic to Firestore.getAll() (#1066) * chore: remove internal WriteOp (#1067) * chore: add linter checks for it|describe.only (#1068) * fix: handle terminate in BulkWriter (#1070) * chore: run template copying last in synthtool (#1071) * feat: Firestore Bundles implementation (#1078) * feat: add support for set() with SetOptions when using FirestoreDataConverter (#1087) * feat: Add totalDocuments and totalBytes to bundle metadata. (#1085) * feat: Add totalDocuments and totalBytes to bundle metadata. * fix: Better comment * fix: Better testing. * fix: Improve metadata testing. * fix: incomplete expect in rate-limiter test (#1092) * Remove BatchWrite proto, fix conformance tests * chore: use public API types internally (#1100) * feat: add Partition and BatchWrite protos (#1110) * fix: remove GCF transaction fallback (#1112) * fix: add BulkWriter integration tests, java backport changes, delete fix (#1117) * chore: merge master (#1218) * chore: add eslint check for console.log statements (#1229) * fix: another attempt at fixing the flaky BulkWriter test (#1228) * Fix comment * Renames * Test fix * Fix unit tests Co-authored-by: Brian Chen <[email protected]> Co-authored-by: wu-hui <[email protected]>

Source-Link: googleapis/synthtool@4dfd20a Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:2d850512335d7adca3a4b08e02f8e63192978aea88c042dacb3e382aa996ae7c

feat: Add totalDocuments and totalBytes to bundle metadata.

be577dd

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2020

wu-hui requested a review from schmidt-sebastian May 19, 2020 15:55

wu-hui assigned schmidt-sebastian May 19, 2020

schmidt-sebastian reviewed May 19, 2020

View reviewed changes

fix: Better comment

095e12d

schmidt-sebastian reviewed May 21, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 21, 2020

fix: Better testing.

38abf77

wu-hui assigned schmidt-sebastian and unassigned wu-hui May 21, 2020

schmidt-sebastian reviewed May 21, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui May 21, 2020

fix: Improve metadata testing.

6905f59

wu-hui assigned schmidt-sebastian and unassigned wu-hui and schmidt-sebastian May 25, 2020

schmidt-sebastian approved these changes May 26, 2020

View reviewed changes

schmidt-sebastian assigned laurenzlong and wu-hui and unassigned schmidt-sebastian and laurenzlong May 26, 2020

wu-hui merged commit 01dcab7 into node10 May 26, 2020

gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 13, 2022

chore: update github issue templates (#1085) (#1658)

1518ec7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

wu-hui commented May 19, 2020

codecov bot commented May 19, 2020 •

edited

Loading

schmidt-sebastian May 19, 2020

wu-hui May 19, 2020

schmidt-sebastian May 21, 2020

wu-hui May 21, 2020

schmidt-sebastian May 21, 2020

wu-hui May 21, 2020

schmidt-sebastian May 21, 2020

wu-hui May 21, 2020

schmidt-sebastian May 21, 2020

wu-hui May 25, 2020

schmidt-sebastian May 21, 2020

wu-hui May 25, 2020

schmidt-sebastian left a comment

		@@ -128,11 +131,14 @@ describe('Bundle Buidler', () => {
		expect(elements.length).to.equal(4);

		const meta = (elements[0] as IBundleElement).metadata;

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

Conversation

wu-hui commented May 19, 2020

codecov bot commented May 19, 2020 • 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

codecov bot commented May 19, 2020 •

edited

Loading