Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert access on a Firestore document path #52

Open
AverageHelper opened this issue Nov 12, 2020 · 7 comments
Open

Assert access on a Firestore document path #52

AverageHelper opened this issue Nov 12, 2020 · 7 comments

Comments

@AverageHelper
Copy link
Contributor

AverageHelper commented Nov 12, 2020

Summary

I propose we add some way for users to assert access to specific document or collection paths.

Basic example

We should be able to assert transaction or batched writes like this:

// The update call:
expect(mockUpdateTransaction).toHaveBeenCalledWith(
  // The document ref:
  expect.toHaveFirestorePath(`accounts/${accountId}/users/${userId}`),
  // The data:
  { ... }
);

Here we assert that the Firestore API was called with the expected parameters while asserting that one of those parameters defined a specific Firestore document path.

Standalone reads and writes are a bit trickier, since calling ref.update({ ... }) doesn't presently inform mockUpdate about the path on which the access was made. I don't have any good ideas for that yet, but at the very least we should have some matchers for our mock DocumentReference or CollectionReference types.

Motivation

Since we now have subcollection support as of #35, users now have a problem when trying to assert access to a specific document path. I do not mean access permissions, those are handled by Firestore rules and beyond the scope of firestore-jest-mock. Presently, the normal way to assert correct document access is by using the variations of the following logical argument:

  1. mockDoc was called after mockCollection, using either expect(...).toHaveBeenCalledAfter or expect(...).not.toHaveBeenCalledBefore, preferably both.
  2. mockCollection was called with the correct collection ID c, using expect(...).toHaveBeenCalledWith, or more preferably expect(...).toHaveBeenNthCalledWith.
  3. mockDoc was called with the correct document ID d, using expect(...).toHaveBeenCalledWith, or more preferably expect(...).toHaveBeenNthCalledWith.
  4. Therefore, we can be certain that the referenced Firestore document path was c/d.

This works well enough when the unit under test only deals with one Firestore document, and calls these methods exactly once each. But that is often not the case. Nested subcollections break the assumption that mockCollection was called only before mockDoc, and therefore our conclusion no longer holds true in every case. We now need to factor in which call to mockDoc or mockCollection we're asserting against, and then any reordering of accesses breaks unit tests, even when such access has only to do with the construction of DocumentReferences.

Consider the following operation on two account-scoped documents that each represent an account member:

const accountA = ...;
const accountB = ...;
const userId = ...;

const userRef = db
  .collection("accounts")
  .doc(accountA)
  .collection("users")
  .doc(userId);
const otherUserRef = db
  .collection("accounts")
  .doc(accountB)
  .collection("users")
  .doc(userId);

A common reason for preparing DocumentReferences like these is to use them in a transaction or a batch write operation, sometimes both:

await db.runTransaction(async transaction => {
  const user = await transaction.get(userRef);
  ...
  transaction.update(otherUserRef, { ... });
});

As of today, we have no way to assert directly that "the document at path `accounts/${accountId}/users/${userId}` was updated with { this data }". Today's methods, which can easily confuse assertions on userRef with assertions on otherUserRef, rely on the following assertions in Jest:

  • mockCollection was called four times, twice with the argument "accounts" and twice with the argument "users". We assert this using four toHaveBeenNthCalledWith assertions, and one toHaveBeenCalledTimes assertion for good measure.
  • mockDoc was called four times, once with the argument accountA, once with the argument accountB, and twice with the argument userId. We assert this in the same verbose manner as we did mockCollection.
  • mockUpdateTransaction was called once with the first argument being an instance of FakeFirestore.DocumentReference and the second being { the data }. To assert more than this requires reconstructing the same document reference using the mocked Firestore database (which may involve an import or a require). This is a nontrivial operation.

We cannot clearly assert the order of calls to mockCollection and mockDoc without doing a fancy dance about which call with which arguments came before which other call with which arguments, which will always be complicated by the fact that we call collection("accounts") twice. We may simplify this call, but the issue remains about the ordering of the other calls. Test cases quickly become very bulky and difficult to maintain.

Proposed Solution

Asserting specific document paths can be done with a simple Jest matcher!

FakeFirestore.DocumentReference has a path property which contains a string similar to the one canonical to Firestore.DocumentReference. In theory, we all we need to do to assert that a transaction.update call occurred on the correct path is to assert that the call's first argument has a path property and that its value is the expected document path.

We may define the matcher with something like the following TypeScript code:

function toHaveFirestorePath(
  this: jest.MatcherUtils,
  ref: { path: string },
  path: string
): jest.CustomMatcherResult {
  if (typeof path !== "string") {
    throw new Error(`Expected 'path' to be a string. Got ${typeof path}: ${path}`);
  }
  const pass =
    ref &&                           // truthy
    typeof ref === "object" &&       // is an object
    "path" in ref &&                 // has a "path" property
    typeof ref.path === "string" &&  // ref.path is a string
    ref.path.startsWith(path);       // ref.path is at least a child of the expected path

  // The message only appears when the test fails (whether by passing when it was expected not to
  // or by not passing when it was expected to)
  return {
    message: () => `Expected '${path}' ${pass ? "NOT" : ""}to be '${ref.path}'`,
    pass
  };
}

This code uses the path property of FakeFirestore.DocumentReference or FakeFirestore.CollectionReference to check that the value under test is a child of the provided path.

We use the matcher in test cases like so:

// the unit under test
async function doTheThing() {
  const ref = db.collection("accounts").doc("accountA").collection("users").doc("userA");
  await db.runTransaction(async transaction => {
    ...
    await transaction.get(ref);
    ...
  });
}

// the test
beforeEach(async () => {
  await doTheThing();
});

test("reads from account A", () => {
  expect(mockGetTransaction).toHaveBeenCalledWith(
    expect.toHaveFirestorePath(`accounts/accountA`)
  );
});

With one simple assertion, we can prove that the unit accessed some document under accounts/accountA. It is now trivial to assert other important parts of the Firestore call, such as:

  • ensuring that the document was only read once.
  • the read occurred before any write call to the same path.
  • all calls were made against a document at a specific path.

Other matchers may be written to handle more granular or more specific cases as needed.

Further Work

FakeFirestore.DocumentReference#path

The canonical Firestore.DocumentReference object has a path property, but in designing FakeFirestore.DocumentReference I did not consider the structure of the canon version. This may break some code that relies on the value of that path property. We should update our implementation to match Firestore's.

#102 makes document paths more closely match Firestore's implementation to the best of my knowledge.

What about single I/O calls?

The matcher described in this proposal only extend Jest's present ability to assert properties of arguments to mock function calls. As far as I am aware, Jest does not have an easy way to assert properties of objects on which mock methods were called. Some restructuring may be necessary to permit that capability in a similar fashion. Suggestions would be appreciated.

EDIT: IDEA!! IIRC, Firestore canonically returns a document ref or document snapshot as the result of a write operation. We might be able to assert the path of that return value in a Jest matcher.

@AverageHelper
Copy link
Contributor Author

We've been using this matcher in our own production code for some time, and have found it extremely handy. I don't know how to export Jest matchers from a package, but it must be possible with minimal work on users' end, considering jest-extended is a thing.

@sbatson5
Copy link
Owner

sbatson5 commented Mar 5, 2021

Sorry it's taken me so long to read through this but I like the concept a lot. And it doesn't make our API more complicated.

I'm not finding an easy way for us to have this automatically added to jest, but it looks like we can follow what they do on this library:
https://github.com/tanem/jest-prettyhtml-matchers#basic-usage

The end-user can just call expect.extend(toHaveFirestorePath) if they want to use the helper. That doesn't feel like a big ask, given the value it adds. So we would just export this as a helper from our package. What do you think?

@AverageHelper
Copy link
Contributor Author

Absolutely! It would be simple to write up a function to export. Perhaps we would even export a function that would build toHaveFirestorePath and whatever matchers we want to add in the future. expect.extend takes an object of functions keyed by the name that Jest should expose on the expect helper, so having some function that returns { toHaveFirestorePath } would be perfect.

@AverageHelper
Copy link
Contributor Author

AverageHelper commented Mar 6, 2021

I'm working on a branch that will make the path property of firestore-jest-mock's document and collection references match more closely Firestore's analagous path properties. Ours are close (except in the case of collectionGroup queries), but not quite ready to assert document paths correctly. It may be prudent to wait on exporting this function until our paths are more consistent, so as to avoid breaking things in the future.

@AverageHelper
Copy link
Contributor Author

I'm working on a branch ...

Update on that branch. It's very nearly ready. For the sake of my own dependent projects, I'm waiting on firestore-jest-mock's TypeScript definitions before I get that work merged in.

@sbatson5
Copy link
Owner

Looking at that today!

@AverageHelper
Copy link
Contributor Author

@sbatson5 Document and Collection path fixes are on #102.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants