Removing @google-cloud/projectify by schmidt-sebastian · Pull Request #564 · googleapis/nodejs-firestore

schmidt-sebastian · 2019-03-05T00:23:07Z

This PR removes the project ID replacement that relies on @google-cloud/projectify to replace the string {{projectId}} in all outgoing Proto messages. This is an improved version of the original PR that uses less async/await after some much appreciated pushback from @mikelehen (yay!).

I kept some of the non-essential formatting changes from the first version of this PR in the first commit. There are no functional changes here.

The second commit (e55174c) still has a bunch of changes:

It moves all Proto conversion to callsites that get executed after initializedIsNeeded() is called. This means that there is now an operation queue in WriteBatch, and that Query cursors are no longer serialized to Proto at the point of query construction.
It adds RelativePath as a new top-level (internal) class. This class is used in DocumentReferences and CollectionReferences when we do not yet know the project ID. RelativePaths get converted to ResourcePaths during serialization.
It removes the Project ID replacement from the Firestore class. While there is a lot of churn, the resulting code is much simpler:
- Most of the initialization now happens at startup. ClientPool is initialized right away, which removes some state keeping from the client.
- We no longer have to "decorate" requests
- Watch no longer has to go through two transform streams

codecov · 2019-03-05T05:19:59Z

Codecov Report

Merging #564 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
+ Coverage   95.85%   95.88%   +0.02%     
==========================================
  Files          24       24              
  Lines        1931     1969      +38     
  Branches      168      168              
==========================================
+ Hits         1851     1888      +37     
- Misses         57       58       +1     
  Partials       23       23

Impacted Files	Coverage Δ
dev/src/timestamp.ts	`100% <ø> (ø)`	⬆️
dev/src/geo-point.ts	`91.66% <ø> (ø)`	⬆️
dev/src/watch.ts	`99.1% <100%> (+0.06%)`	⬆️
dev/src/transaction.ts	`98.41% <100%> (+0.07%)`	⬆️
dev/src/field-value.ts	`93.75% <100%> (ø)`	⬆️
dev/src/document.ts	`97.02% <100%> (+0.03%)`	⬆️
dev/src/validate.ts	`90% <100%> (ø)`	⬆️
dev/src/write-batch.ts	`100% <100%> (ø)`	⬆️
dev/src/serializer.ts	`96.51% <100%> (ø)`	⬆️
dev/src/index.ts	`97.67% <100%> (-0.51%)`	⬇️
... and 3 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 c92e1cf...0bd9f93. Read the comment docs.

codecov · 2019-03-05T05:19:59Z

Codecov Report

Merging #564 into master will decrease coverage by 0.37%.
The diff coverage is 97.68%.

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
- Coverage   96.01%   95.63%   -0.38%     
==========================================
  Files          24       24              
  Lines        1955     1969      +14     
  Branches      168      167       -1     
==========================================
+ Hits         1877     1883       +6     
- Misses         57       65       +8     
  Partials       21       21

Impacted Files	Coverage Δ
dev/src/timestamp.ts	`100% <ø> (ø)`	⬆️
dev/src/document.ts	`96.96% <100%> (-0.02%)`	⬇️
dev/src/validate.ts	`94% <100%> (ø)`	⬆️
dev/src/write-batch.ts	`100% <100%> (ø)`	⬆️
dev/src/serializer.ts	`96.51% <100%> (ø)`	⬆️
dev/src/path.ts	`97.82% <100%> (+0.2%)`	⬆️
dev/src/watch.ts	`98.21% <100%> (-0.89%)`	⬇️
dev/src/reference.ts	`99.69% <100%> (+0.01%)`	⬆️
dev/src/order.ts	`95.4% <100%> (ø)`	⬆️
dev/src/index.ts	`96.07% <89.47%> (-2.1%)`	⬇️
... and 3 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 44ad277...b11bc01. Read the comment docs.

mikelehen

Nice! Mostly LGTM other than a few nits and a gripe about RelativePath naming / inheritance.

mikelehen · 2019-03-06T23:56:44Z

+   * @private
+   */
+  get projectId(): string {
+    this.ensureClientInitialized();


FWIW, this naming tripped me up. I was confused about ensureClientInitialized() vs initializeIfNeeded().

Looking at them, I would be tempted to rename ensureClientInitialized() to assertClientInitialized().

But looking closer, ensureClientInitialized() is only called in one other place (directly below) and it's actually redundant. So why not drop the method and just put the projectId_ !== undefined check here?

I wanted to point out that this is an important step, but I also did not mean to confuse anyone. To err on the side of caution, I inlined it here and removed the redundant call in formattedName.

mikelehen · 2019-03-07T00:01:39Z

    }

-    return this._runTransaction(updateFunction, transactionOptions);
+    return this.initializeIfNeeded().then(


Are you intentionally not using async/await (here and in most but not all of the other calls to initializeIfNeeded())? I think it's generally cleaner.

Yes, this is intentional. Async functions do not throw exceptions, instead they return a failed Promise. Since we want to validate our input synchronously, I did not use async/await in functions that perform validation.

mikelehen · 2019-03-07T00:06:41Z

+  private ensureClientInitialized(): void {
+    if (this._projectId === undefined) {
+      // Note that we do not mention `initializeIfNeeded()` here to not leak the
+      // private API.


I suspect this method is going away, but if not I'd drop this comment since 1) it's not really important for somebody to know why you didn't mention it, and 2) in fact I don't think it's important to not mention it. Internal Errors are necessarily going to mention internal implementation details sometimes. I wouldn't have any concerns about "leaking" stuff... we want to make the error as useful as possible for us to debug what went wrong.

I removed it. I do however thing that we should not mention initializeIfNeeded(). If we point out that we should have called initializeIfNeeded(), our users will do so instead of filing a GitHub issue.

mikelehen · 2019-03-07T00:08:07Z

+ * The default database ID for this Firestore client. We do not yet expose the
+ * ability to use different databases.
+ */
+export const DATABASE_ID = '(default)';


FWIW I had trouble guessing what this was when I saw it imported elsewhere. DEFAULT_DATABASE_ID would be much clearer.

Sure thing. DEFAULT_DATABASE_ID it is.

mikelehen · 2019-03-07T01:21:35Z

- * @class
 */
-export class ResourcePath extends Path<ResourcePath> {
+export class RelativePath extends Path<RelativePath> {


So I'm having some trouble with this RelativePath class:

It's not an arbitrary relative path (else isDocument() and isCollection() wouldn't be answerable).

"Relative" probably isn't the defining characteristic of this class. In fact, the Path base class is already a relative path. It seems like the unique methods (id(), isCollection(), isDocument()) are really characteristics of being a "resource path" rather than a "relative path."

It seems strange for ResourcePath to extend RelativePath. "A resource path is a relative path" doesn't hold. And it's confusing for a method that takes path: RelativePath that path may actually be an (absolute) ResourcePath.

I haven't analyzed the code to figure out how these classes are used and what other options might work. Naive idea fragments:

I think the mobile SDKs deal with this by having all ResourcePaths be relative and we only create an "absolute" one at the point we serialize. But that has downsides (most notably we can't handle resource-paths in documents that point to to other projects).

If there's a way to solve this via composition rather than inheritance (e.g. have a DatabasePath that contains projectId, databaseId, and resourcePath members), I'd be a fan. I haven't read it, but presumably this wikipedia article makes the argument for me about why composition should be preferred over inheritance: https://en.wikipedia.org/wiki/Composition_over_inheritance

Failing all that, if you renamed ResourcePath to AbsoluteResourcePath and RelativePath to ResourcePath that would actually solve most of the problems I listed above I think.

Perhaps any of these solutions will involve a lot of churn though, so we may want to punt to a separate PR.

I kept the class structure as is. I actually think that inheritance makes sense here - we are special casing the RelativePath class and modify the existing behavior only slightly. Furthermore, we can use both classes interchangeably, without having to to rely on TypeScrips's intersection types (or introduce another interface).

With all that being said, I renamed the classes to ResourcePath and QualifiedResourcePath. This should address most of your concerns. While there is some churn, it's 70ish lines and I pushed in a single commit together with the rest of the feedback (5f93109).

mikelehen · 2019-03-07T01:38:59Z

  private readonly _firestore: Firestore;
  private readonly _serializer: Serializer;
-  private readonly _writes: WriteOp[] = [];
+  private readonly _ops: Array<() => WriteOp> = [];


This probably deserves a comment... Part of me wants to rename it lazyOps too, though not sure that is necessary /better.

Comment added.

schmidt-sebastian · 2019-03-11T02:09:45Z

Feedback addressed. I also had to add back the @private annotation since my assumption that a @private class doesn't need @private for its members did not hold.

mikelehen

Basically LGTM. If you decide to rework ResourcePath / QualifiedResourcePath inheritance, let me know and I can take another look.

mikelehen · 2019-03-11T15:33:03Z

+
+  /**
+   * Convenience method to match the ResourcePath API. This method always
+   * returns the current instance. The arguments is ignored.


Not a big fan of ignoring arguments. Somebody could theoretically call this incorrectly and never know. Seems like we should either throw unconditionally (if we expect this to never get called), respect the projectId, or assert the projectId matches the actual projectId.

I renamed the argument to projectIdIfMissing here and in ResourcePath. When I started writing this, I originally added an assert - but that doesn't work since the projectId in the argument is always the Project ID of the SDK's Firestore project. The Project ID in the path itself may be an ID from a different project if we deserialize a DocumentReference from a document.
An alternative would be to remove this method and only call toQualifiedResourcePath if we need to. That makes the callsites more complicated, and I kinda like the simplicity of the current approach. So for now I have just updated the argument name and comment, but please do let me know if you'd rather want me to drop this method altogether.

Oh, wow. So DocumentReference may wrap either a ResourcePath or a QualifiedResourcePath and so we have to make sure we preserve the projectId if it's already a QualifiedResourcePath, but insert the current projectId if it's not? I didn't realize that issue (partially because we don't deal with it in the mobile SDK). That is a bit funky, but I guess I don't feel strongly enough to suggest you rework this anymore...

Another way to solve this would be to add Firestore as a member to ResourcePath. formattedName() could then figure out what it needs on its own. I'll leave this as an exercise for the reader for now.

mikelehen · 2019-03-11T15:45:33Z

-  append(relativePath: RelativePath|string): ResourcePath {
-    return super.append(relativePath) as ResourcePath;
+  append(relativePath: ResourcePath|string): QualifiedResourcePath {
+    return super.append(relativePath) as QualifiedResourcePath;


I guess we broke the Path<T> trick that lets us inherit these methods as-is? And this cast here is safe because super.append() ends up calling our construct() which actually does return a QualifiedResourcePath, even though we end up losing that type information? This feels a bit hacky...

I guess to make this work using the Path<T> pattern (and be able to inherit methods w/o overriding and unsafe casting, etc.), we'd need to create an abstract ResourcePathBase<T> class with the shared functionality and then have ResourcePath and QualifiedResourcePath both inherit from it with the appropriate construct() method implemented. WDYT? Worth it?

I debated doing this but I think this overcomplicates this class structure quite a bit. I added a comment to explain why this cast is safe.

mikelehen · 2019-03-11T15:46:26Z

+  }
+
+  /**
+   * Constructs a new instance of RelativePath.


RelativePath => ResourcePath here and below.

mikelehen · 2019-03-12T00:14:05Z

+
+  /**
+   * Convenience method to match the ResourcePath API. This method always
+   * returns the current instance. The arguments is ignored.


Oh, wow. So DocumentReference may wrap either a ResourcePath or a QualifiedResourcePath and so we have to make sure we preserve the projectId if it's already a QualifiedResourcePath, but insert the current projectId if it's not? I didn't realize that issue (partially because we don't deal with it in the mobile SDK). That is a bit funky, but I guess I don't feel strongly enough to suggest you rework this anymore...

mikelehen · 2019-03-12T00:14:39Z

LGTM other than CI failing. 😛

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 5, 2019

This was referenced Mar 5, 2019

Don't call stream.end() on Watch ended by server #565

Merged

Introduce async/await to test/order.ts #566

Merged

This was referenced Mar 5, 2019

Make formattedName a method #567

Closed

Serialize documents at commit time #568

Closed

Various low risk fixes

0b04319

schmidt-sebastian force-pushed the mrschmidt-projectid branch 2 times, most recently from fc040cf to 981d992 Compare March 6, 2019 02:10

Remove project ID replacement

e55174c

schmidt-sebastian force-pushed the mrschmidt-projectid branch from 981d992 to e55174c Compare March 6, 2019 02:16

schmidt-sebastian requested a review from mikelehen March 6, 2019 02:35

schmidt-sebastian assigned mikelehen Mar 6, 2019

schmidt-sebastian changed the title ~~WIP: Removing @google-cloud/projectify~~ Removing @google-cloud/projectify Mar 6, 2019

mikelehen suggested changes Mar 7, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Mar 7, 2019

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Mar 11, 2019

schmidt-sebastian force-pushed the mrschmidt-projectid branch from 0eba165 to e20227f Compare March 11, 2019 01:59

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 11, 2019

googleapis deleted a comment from googlebot Mar 11, 2019

schmidt-sebastian force-pushed the mrschmidt-projectid branch from e20227f to 89b5dbb Compare March 11, 2019 02:01

schmidt-sebastian added 2 commits March 11, 2019 10:12

Addressing Feedback

5f93109

Add @Private annotation

b49fe31

schmidt-sebastian force-pushed the mrschmidt-projectid branch from 7cf08ec to b49fe31 Compare March 11, 2019 02:12

Merge branch 'master' into mrschmidt-projectid

5484370

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Mar 11, 2019

Remove all usages of extend

a840e59

schmidt-sebastian force-pushed the mrschmidt-projectid branch from 08fedb9 to a840e59 Compare March 11, 2019 05:12

Merge branch 'master' into mrschmidt-projectid

635f3d1

mikelehen approved these changes Mar 11, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Mar 11, 2019

Last round of feedback

dd2bce6

mikelehen approved these changes Mar 12, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label Mar 12, 2019

Fix compile

b11bc01

schmidt-sebastian merged commit 114de25 into master Mar 12, 2019

schmidt-sebastian deleted the mrschmidt-projectid branch April 15, 2019 23:34

Conversation

schmidt-sebastian commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 5, 2019

Codecov Report

Uh oh!

codecov Bot commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

schmidt-sebastian Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

schmidt-sebastian commented Mar 11, 2019

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!

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 commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schmidt-sebastian commented Mar 5, 2019 •

edited

Loading

codecov Bot commented Mar 5, 2019 •

edited

Loading

schmidt-sebastian Mar 11, 2019 •

edited

Loading