Overhaul Documents #1886

wilhuff · 2019-06-15T20:59:21Z

Remove different internal types for MaybeDocument, Document, NoDocument,
and UnknownDocument. These are all now just Document.

Also make Documents non-nullable nearly everywhere.

The change introduces a DocumentType enum and maps the old classes like
so:

Before	After
`null`	`DocumentType.UNKNOWN`
`UnknownDocument`	`DocumentType.CONTENTS_UNKNOWN`
`NoDocument`	`DocumentType.MISSING`
`Document`	`DocumentType.EXISTING`

Part of why this change works is that most of the system only cares
whether or not a document exists. We have several refinements of
non-existing documents for the purposes of caching but everywhere else
we can ignore it and just check document.exists. The instanceof checks
were usually over-constraining things.

The motivation for this change is in two parts:

It's always bugged me that we had so many instanceof checks around
In C++ we don't have instanceof anyway, so we have to do something
else.

Also, this definitely addresses b/32275378.

wilhuff · 2019-06-15T21:01:43Z

Note that this PR is requesting feedback on the idea in here as well. I think this makes things considerably easier to reason about internally and it will be definitely make the C++ implementation easier (no subclasses make things so much easier), but I wanted to vet this outside the noise of actually porting iOS Objective-C code to C++.

Remove different internal types for MaybeDocument, Document, NoDocument, and UnknownDocument. These are all now just Documents. Also make Documents non-nullable nearly everywhere. The change introduces a DocumentType enum and maps the old classes like so: null => DocumentType.UNKNOWN UnknownDocument => DocumentType.CONTENTS_UNKNOWN NoDocument => DocumentType.MISSING Document => DocumentType.EXISTING Part of why this change works is that most of the system only cares whether or not a document exists. We have several refinements of non-existing documents for the purposes of caching but everywhere else we can ignore it and just check Document.exists. The instanceof checks were usually overconstraining things. The motivation for this change is in two parts: * It's always bugged me that we had so many instanceof checks around * In C++ we don't have instanceof anyway, so we have to do something else. Also, this definitely addresses b/32275378.

mikelehen

[only partially reviewed...]

Hrmm..... I'm not sure if I'm excited about this. My big reservation is that we've significantly expanded the state-space of Document. it's state now consists of {1 of 4 DocumentTypes, 1 of 3 DocumentStates, real snapshot version or not, real value or not} so there are 4 * 3 * 2 *2 = 48 possible combinations, most of which are probably not valid / useful. Before I think we only had 14 combinations (8 for Document, 4 for NoDocument, 2 for UnknownDocument) and (unfortunately) some of those were already invalid...

This makes it harder to reason about code correctness. When writing code that deals with Documents, we theoretically have to make sure it handle all possible state combinations. But that's not feasible or desirable, so instead all of the code around Document will end up with implicit assumptions about the possible states document could actually be in... which can be fragile and hard to read.

We could mitigate to a degree by adding strict assertions (and supporting comments) to Document to clearly establish the expected states a document could actually be in... but runtime validation is not as good as compile-time validation and in general it just seems like we're not doing a good job of mapping our needs onto the right data type if we have so many invalid / non-useful states our data type could be in...

That said, this is mostly an abstract concern and I haven't looked at all the calling code. Maybe it's not that big of a deal. I'll take another look Monday and see how I feel about it.

mikelehen · 2019-06-15T21:40:45Z

packages/firestore/src/model/document.ts

+ * denotes time we know it didn't exist at.
+ *
+ * A class representing an existing document whose data is unknown (e.g. a
+ * document that was updated without a known base document).


I think this comment needs cleanup.

mikelehen · 2019-06-15T21:48:50Z

packages/firestore/src/model/document.ts

+   * Memoized serialized form of the document for optimization purposes (avoids
+   * repeated serialization). Might be undefined.
+   */
+  memoizedProto?: api.Document;


This isn't actually used, right? I feel like it should probably not be in this PR. :-)

It is. It was previously accepted as an optional last argument to the Document constructor. This piggybacks on the DocumentOptions now instead.

Ahh. I think you accidentally dropped storing it then (makeDocumentState() ignores it).

mikelehen · 2019-06-15T21:54:35Z

packages/firestore/src/model/document.ts

@@ -24,55 +24,162 @@ import { FieldPath } from './path';

 import * as api from '../protos/firestore_proto_api';

+export interface MissingDocumentOptions {


I guess {Missing}DocumentOptions exist to supply named parameters to the static constructor methods? They could probably use comments explaining that (I was confused why they exist in addition to DocumentState at first). But I also wonder how we'll want to port this to the other platforms... may make us reconsider the approach for TypeScript.

This is pre-existing surface area. I'm definitely in favor of removing it and just passing DocumentState instead.

I think I'm in favor of merging DocumentState into DocumentKind and so we probably need to keep these around...

mikelehen · 2019-06-15T22:00:15Z

packages/firestore/src/model/document.ts

  }

-  toString(): string {
-    return `NoDocument(${this.key}, ${this.version})`;
+  get missing(): boolean {


mikelehen · 2019-06-15T22:03:50Z

packages/firestore/src/model/document.ts

 */
-export abstract class MaybeDocument {
-  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+export enum DocumentType {


I would put MISSING / EXISTS at the top of this enum, since they are the simple cases...

Per our chat conversation, I think I am a fan of combining the DocumentType and DocumentState. Also (see below), I am interested in trying to merge UNKNOWN and CONTENTS_UNKNOWN. I think this would result in the following 6 document types:

DOCUMENT
DOCUMENT_WITH_LOCAL_MUTATIONS
DOCUMENT_WITH_COMMITTED_MUTATIONS
MISSING
MISSING_WITH_LOCAL_MUTATIONS
NO_ENTRY

This takes us from 4*3 = 12 potential kind/state combinations to 6.

mikelehen · 2019-06-15T22:05:11Z

packages/firestore/src/model/document.ts

-    super(key, version);
-    this.hasCommittedMutations = !!(options && options.hasCommittedMutations);
+  get exists(): boolean {
+    return this.type === DocumentType.EXISTS;


Naively I would think that CONTENTS_UNKNOWN should return true. If it shouldn't, then perhaps .exists is misnamed and really means something else?

See below. exists here exactly mirrors the DocumentType so we should rename that to make it clearer (and then update this to match if we're keeping these properties).

Per chat conversation I think these helpers should become:

.isDocument
.isMissingDocument
.noEntry ( or .isNoEntry? )

And I'd like to provide the guarantee that exactly 1 is true.

mikelehen · 2019-06-15T22:07:08Z

packages/firestore/src/model/document.ts

  }

-  get hasPendingWrites(): boolean {
-    return this.hasCommittedMutations;
+  get unknown(): boolean {


isUnknown? Though I'm not sure that's descriptive enough... I'm not immediately thinking of a better name though.

mikelehen · 2019-06-15T22:10:38Z

packages/firestore/src/model/document.ts

+   * Returns true if the document is of some definite type: either existing
+   * or definitely missing.
+   */
+  get definite(): boolean {


I have a really hard time guessing what if (fooDoc.definite) { ... } means.

In general, I'm nervous about all of these properties (exists, missing, unknown, definite) because I'm worried they could be confusing / misleading. We're mapping 4 DocumentTypes (actually 3 since CONTENTS_UNKNOWN doesn't show up) onto 4 helper properties and the mapping isn't immediately obvious to me from the names. You might expect that !exists implies missing but that would be wrong.

There may be better terminology that makes everything clearer, but failing that I would be inclined to just drop them all and move the explicit DocumentType checks into the calling code... if that's too onerous though, we can work harder on the names / comments.

Most of the properties mirror the document types. They just abbreviate doc.type === DocumentType.MISSING to doc.isMissing. If this is confusing I can drop it.

Would renaming EXISTS to DEFINITELY_EXISTS and MISSING to DEFINITELY_MISSING help (and corresponding accessors too if still doing those)?

Could get rid of definite and just inline it into the callers. Alternatively with the rename above it might mean something clearer.

Per previous comment I'd like to remove this and be able to use !noEntry instead... failing that, perhaps inline it if there aren't too many uses...

wilhuff

Hrmm..... I'm not sure if I'm excited about this. My big reservation is that we've significantly expanded the state-space of Document. it's state now consists of {1 of 4 DocumentTypes, 1 of 3 DocumentStates, real snapshot version or not, real value or not} so there are 4 * 3 * 2 *2 = 48 possible combinations, most of which are probably not valid / useful. Before I think we only had 14 combinations (8 for Document, 4 for NoDocument, 2 for UnknownDocument) and (unfortunately) some of those were already invalid...

So I started down this path because the C++ version of this with inheritance is nasty. This started out as an experiment to see if I could make this consolidation work at all. By the time I had proven to myself it could work it was very nearly in the state I sent.

As I worked through it I found that in basically every calling case the code got simpler. The realization I had was that for the most part we don't care about the permutations you're describing.

For example:

real version or not doesn't matter: we either write the versions out as they are or coerce non-existing document versions to MIN anyway.
real value or not doesn't matter: when we want to read a field ObjectValue.EMPTY does the right thing.
document state doesn't matter except to the local serializer

Also, there were a bunch of implicit assumptions/special cases that could actually be removed because of this. For example:

Callers don't have to know that null MaybeDocuments should have SnapshotVersion.MIN versions
Callers don't have to know that only existing documents can have local mutations

One thing I considered doing but didn't end up doing is explicitly encoding the allowed DocumentType x DocumentState values in just a single enum with 7 states:

exists (sync, local, committed)
missing (sync, committed)
content unknown (committed)
unknown (sync)

I could be convinced to make this happen (though to do so I do think we need the simplifying accessors the removal of which we're discussing elsewhere).

I think in practice the state explosion you're describing doesn't happen. The factories constrain what can be created and downstream code gets a value that just works. It's only at the very edges (serialization, etc) where we care most about these differences and I don't think this change made that code any worse.

I've included some feedback below to engage in the discussion but haven't made any further changes.

wilhuff · 2019-06-16T00:45:37Z

packages/firestore/src/model/document.ts

+   * Returns true if the document is of some definite type: either existing
+   * or definitely missing.
+   */
+  get definite(): boolean {


Most of the properties mirror the document types. They just abbreviate doc.type === DocumentType.MISSING to doc.isMissing. If this is confusing I can drop it.

Would renaming EXISTS to DEFINITELY_EXISTS and MISSING to DEFINITELY_MISSING help (and corresponding accessors too if still doing those)?

Could get rid of definite and just inline it into the callers. Alternatively with the rename above it might mean something clearer.

wilhuff · 2019-06-16T00:47:56Z

packages/firestore/src/model/document.ts

-    super(key, version);
-    this.hasCommittedMutations = !!(options && options.hasCommittedMutations);
+  get exists(): boolean {
+    return this.type === DocumentType.EXISTS;


See below. exists here exactly mirrors the DocumentType so we should rename that to make it clearer (and then update this to match if we're keeping these properties).

wilhuff · 2019-06-16T00:49:22Z

packages/firestore/src/model/document.ts

+   * Memoized serialized form of the document for optimization purposes (avoids
+   * repeated serialization). Might be undefined.
+   */
+  memoizedProto?: api.Document;


It is. It was previously accepted as an optional last argument to the Document constructor. This piggybacks on the DocumentOptions now instead.

wilhuff · 2019-06-16T00:50:00Z

packages/firestore/src/model/document.ts

@@ -24,55 +24,162 @@ import { FieldPath } from './path';

 import * as api from '../protos/firestore_proto_api';

+export interface MissingDocumentOptions {


This is pre-existing surface area. I'm definitely in favor of removing it and just passing DocumentState instead.

mikelehen

Having finished reviewing this, I have pretty mixed feelings. I think:

Introducing a DocumentEntry type that can represent the lack of a cache entry (including the document key and perhaps a version) is helpful and it's really pleasing to see all the places where MaybeDocument | null turn into DocumentEntry. This is a nice clean-up.
But I am nervous everywhere we have Document or MaybeDocument turn into a DocumentEntry since now our type is too broad and so calling code could pass invalid document entries or get back document entries that it fails to handle properly (since it may have invalid assumptions about the actual types of document entries expected).

Because of 2, this is pretty difficult to review for absolute correctness. And I think it leaves our code more fragile going forward. We can mitigate to some degree by introducing assertions and more explicit comments documenting the contract of each method as far as expected document types go, but ultimately the code is less type-safe and less self-documenting. :-/

Honestly, ignoring C++, my preferred path forward would be that we keep the benefits of #1 by introducing a DocumentEntry class that is a super class of MaybeDocument containing just key and version... but we keep the separate types so we can narrow as appropriate for parts of the system that don't need to deal with all possible document types.

FWIW if we wanted, in TypeScript we could reduce instanceof checks with user-defined type guards: https://basarat.gitbooks.io/typescript/docs/types/typeGuard.html#user-defined-type-guards Doesn't help Android or C++ though. :-/

I'm open to being won over by this change, but right now I'm on-the-fence as-is and the burden of porting and code reviewing this twice more and the risk of bugs makes me suspicious it's not a net-win.

mikelehen · 2019-06-17T23:59:02Z

packages/firestore/src/api/database.ts

@@ -1130,12 +1112,11 @@ export class DocumentReference implements firestore.DocumentReference {
            snapshot.docs.size <= 1,
            'Too many documents returned on a document query'
          );
-          const doc = snapshot.docs.get(this._key);
+          const doc = getDocument(this._key, snapshot.docs);


I'm not sure if this helper pays for itself (I couldn't guess what it did, so I had to look it up only to find out it's a pretty trivial 2-liner). What about this?

const doc = Document.fromNullable(snapshot.docs.get(this._key));

mikelehen · 2019-06-18T03:02:45Z

packages/firestore/src/api/database.ts

@@ -1290,33 +1269,31 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
  ): unknown {
    validateBetweenNumberOfArgs('DocumentSnapshot.get', arguments, 1, 2);
    options = validateSnapshotOptions('DocumentSnapshot.get', options);
-    if (this._document) {


I'm not super psyched about relying on document.data for nonexistent documents returning an empty ObjectValue instance. While convenient, it seems a bit non-obvious and so this code becomes less obviously-correct. I don't feel super strongly, but my preference would be to not rely on this and in fact to make .data throw for nonexistent documents.

mikelehen · 2019-06-18T03:06:33Z

packages/firestore/src/core/firestore_client.ts

    this.verifyNotShutdown();
    return this.asyncQueue
      .enqueue(() => {
        return this.localStore.readDocument(docKey);
      })
-      .then((maybeDoc: MaybeDocument | null) => {
-        if (maybeDoc instanceof Document) {
+      .then((maybeDoc: Document) => {


FWIW in the fullness of time we should probably get rid of maybeDoc references, and (pending final rename decisions) perhaps move doc references toward docEntry etc... but I'm probably in favor of doing this cleanup in separate PRs. :-)

mikelehen · 2019-06-18T03:33:11Z

packages/firestore/src/core/firestore_client.ts

-      .then((maybeDoc: MaybeDocument | null) => {
-        if (maybeDoc instanceof Document) {
+      .then((maybeDoc: Document) => {
+        if (maybeDoc.definite) {


I don't like definite (both the name itself and the mere fact that we're inventing an extra term as an alias for isDocument || isMissingDocument increasing the possibility of terminology confusion).

We could just inline doc.isDocument || doc.isMissingDocument everywhere... but that does seem a bit ugly, since it shows up a lot.

I'm not 100% sure this is a good idea, but I'm wondering if we could get rid of CONTENTS_UNKNOWN and instead just use UNKNOWN but with a real version number (instead of SnapshotVersion.MIN). I think that mostly "just works" and then this code can safely become if (!maybeDoc.noEntry) { ... } and all of our code can count on exactly one of isDocument, isMissingDocument, or noEntry being true.

mikelehen · 2019-06-18T03:44:42Z

packages/firestore/src/core/transaction.ts

    } else {
-      throw fail('Document in a transaction was a ' + doc.constructor.name);


Hrm... while this case is no longer possible via the type system, we did introduce a new possibility which is that doc.noEntry could now be true. Should we throw an assert in to make sure (either for !doc.noEntry or for doc.isMissingDocument)?

mikelehen · 2019-06-18T15:39:54Z

packages/firestore/src/model/document.ts

+  /**
+   * Whether this document had a local mutation applied that has not yet been
+   * acknowledged by Watch.
+   */
  get hasPendingWrites(): boolean {


FWIW this is used in exactly one place and so I'm somewhat inclined to inline it (or move the helper closer to where it's used) in order to avoid an extra term exposed here ("local mutations", "committed mutations", "pending writes").

mikelehen · 2019-06-18T15:49:27Z

packages/firestore/src/model/mutation.ts

    } else if (this.exists !== undefined) {
-      return this.exists === maybeDoc instanceof Document;
+      return this.exists === maybeDoc.exists;


pre-existing nit: I had to question whether or not we are intentionally treating unknown the same as missing, and I think we are... but we should probably either do it explicitly or add a comment.

mikelehen · 2019-06-18T15:57:18Z

packages/firestore/src/model/mutation_batch.ts

-      if (mutatedDocument) {
+      const baseDocument = getDocument(m.key, maybeDocs);
+      const mutatedDocument = this.applyToLocalView(m.key, baseDocument);
+      if (!mutatedDocument.unknown) {


This might need to change if we merge UNKNOWN / CONTENTS_UNKNOWN. I wonder if we could just drop the if check entirely...

mikelehen · 2019-06-18T16:17:56Z

packages/firestore/src/model/document.ts

+      DocumentType.UNKNOWN,
+      key,
+      SnapshotVersion.MIN,
+      ObjectValue.EMPTY,


Per my comments elsewhere I think I would prefer if we made .data throw for missing / unknown documents (and optionally added .dataOrEmptyObject or something so you can explicitly opt into ObjectValue.EMPTY

mikelehen · 2019-06-18T16:20:58Z

packages/firestore/src/model/document.ts

-export class UnknownDocument extends MaybeDocument {
-  constructor(key: DocumentKey, version: SnapshotVersion) {
-    super(key, version);
+  get hasLocalMutations(): boolean {


If possible I think I'd like hasLocalMutations to throw for unknown / missing documents and hasCommittedMutations to throw for unknown documents... We can loosen if/when we need to, but ensuring code isn't checking for impossible things would be nice.

wilhuff · 2019-06-18T17:23:00Z

As I'm reviewing your feedback in occurs to me that an assumption underlying this change was that in practice, most of our code doesn't care about whether or not a document actually exists so long as non-existing document and existing documents behave uniformly. The uniformity would be a feature of the change, not a bug.

Building from there, whether or not a document actually exists or not is just additional metadata that we happen to attach to the document.

For example, this change makes most mutations simpler because no matter the actual state of the document you can apply the mutation and any tests around it.

Of course, there are exceptions:

Patch mutations have different behavior for documents that exist vs not.
Queries only want to return known existing documents
Document gets want known existing or known not existing documents
etc

However, naming concerns aside, most of your feedback is essentially pushing back toward non-uniformity or overspecificity. This comment encapsulates this disconnect.

My contention is that this code has two behaviors based on whether or not the document exists or not. Which form of non-existence doesn't matter here so there's no reason to apply a constraint.

If we can't agree that this basic premise is worthwhile then this change isn't really worth fixing. It's gone way too far toward that goal to bring back to true.

mikelehen · 2019-06-18T17:50:33Z

Hrm. I'm not 100% sure if I'm interpreting your comment correctly. When you say "so long as non-existing document and existing documents behavior uniformly" I'm not sure if you mean "non-existing documents behave uniformly and existing documents behave uniformly" or "(non-existing documents and existing documents) behave uniformly" and in either case I'm not sure if you are considering unknown documents to be included in "non-existing documents"...

FWIW- In the example you linked:

I think treating unknown the same as missing would be incorrect. We should only use a baseVersion of 0 if the document really is a missing document.
The calling code already protects us from .unknown documents, but because that's no longer enforced by the type system, it's no longer obvious / self-documenting.
If part of our goal is to get rid of instanceof checks, we could add .version and .exists (or .isDocument or whatever) to MaybeDocument.

Anyway, let's chat in person after sprint planning.

mikelehen

My main concern continues to be that we have broadened the types of "document entry" accepted and returned by components throughout the system. I've attempted to audit the public methods on our major components to determine if and to-what-degree this broadening could be harmful. See results here: #1900

I happened to find two concrete "bugs waiting to happen", though that wasn't my primary goal. And I think many more potential bugs would be found by looking at the code calling the methods I audited, since it's usually the return type that was broadened.

So I think:

Introducing a DocumentEntry class to represent MaybeDocument|null is useful to simplify and better document MaybeDocument|null types and allow consuming code to use .key and .version without instanceof checks.
Reworking the existing UnknownDocument (aka CONTENTS_UNKNOWN post-refactor) to not be a subclass of MaybeDocument makes sense. I think treating it more like a special case of UNKNOWN (where we happen to have a version) would be better. I experimentally merged UNKNOWN and CONTENTS_UNKNOWN in your PR and it was straightforward to get all the tests to pass.
I'm generally in favor of reworking the naming of our document-related types / concepts (MaybeDocument, NoDocument, etc.)
I think broadening the Document type throughout our codebase is overall harmful. Specifically I think it is occasionally helpful (we did have some over-specialized cases), sometimes benign (we can make code more generic than it "needs" to be with no harm to the surrounding code), but often mildly harmful (weakening our contracts and self-documentation), and occasionally visibly harmful (introducing potential bugs).

So setting C++ aside, I'm not in favor of dropping our existing type hierarchy.

I don't have a solution for C++. If there's really no good way to implement something akin to this, then maybe we should go ahead and implement the collapsed type hierarchy on C++ (like what's in this PR right now) but use comments or some other convention (macros?) to try to document expectations and make it clear how to port code back/forth to the other platforms.

wilhuff · 2019-07-11T14:57:19Z

As discussed, this approach isn't going to work.

wilhuff added the api: firestore label Jun 15, 2019

wilhuff requested a review from mikelehen June 15, 2019 20:59

wilhuff assigned mikelehen Jun 15, 2019

wilhuff force-pushed the wilhuff/consolidate-document branch from ee06751 to 622d9d5 Compare June 15, 2019 21:08

mikelehen reviewed Jun 15, 2019

View reviewed changes

wilhuff commented Jun 16, 2019

View reviewed changes

mikelehen suggested changes Jun 18, 2019

View reviewed changes

mikelehen assigned wilhuff and unassigned mikelehen Jun 18, 2019

wilhuff assigned mikelehen and unassigned wilhuff Jun 18, 2019

mikelehen suggested changes Jun 21, 2019

View reviewed changes

mikelehen assigned wilhuff and unassigned mikelehen Jun 21, 2019

wilhuff closed this Jul 11, 2019

firebase locked and limited conversation to collaborators Oct 10, 2019

wilhuff deleted the wilhuff/consolidate-document branch March 13, 2020 16:25

		@@ -24,55 +24,162 @@ import { FieldPath } from './path';

		import * as api from '../protos/firestore_proto_api';

		export interface MissingDocumentOptions {

		} else {
		throw fail('Document in a transaction was a ' + doc.constructor.name);

Overhaul Documents #1886

Overhaul Documents #1886

Uh oh!

Conversation

wilhuff commented Jun 15, 2019

Uh oh!

wilhuff commented Jun 15, 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!

mikelehen Jun 15, 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!

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!

wilhuff left a comment • 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!

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!

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 Jun 15, 2019 •

edited

Loading

wilhuff left a comment •

edited

Loading

wilhuff commented Jun 18, 2019 •

edited

Loading