Improvement for DataSnapshot paths (#10, #26)

[abeisgoat]

Description

Fixes common UX issue where context.params are not populated from wildcards in path. (See #10 and #26)

Code sample

New Behavior

const func = functions.database
  .ref("/different/{wildcard}/nested/{anotherWildcard}")
  .onCreate((snap, context) => {
    console.log(context.params.wildcard == "at") // true
    console.log(context.params.anotherWildcard == "bat") // true
    console.log(snap.key == "bat") // true
  }

const snap = makeDataSnapshot(
    'hello world',
    '/different/at/nested/bat',
);
const params = {};
wrap(func)(snap, {params});

New Behavior

const func = functions.database
  .ref("/different/{wildcard}/nested/{anotherWildcard}")
  .onCreate((snap, context) => {
    console.log(context.params.wildcard == "at") // true
    console.log(context.params.anotherWildcard == "bat") // true
    console.log(snap.key == "bat") // true
  }

const snap = makeDataSnapshot(
    'hello world',
    '/different/at/nested/bat',
);
const params = {wildcard: "that"};
wrap(func)(snap, {params}); // Throws error due to overlapping "wildcard" definition

I've also added some better logic for ensuring a DataSnapshot's path fits the trigger it is passed to.

Previous / Current Behavior (DataSnapshot Path Checking)

const func = functions.database
  .ref("/good_path/{id}")
  .onCreate(snap => {
    console.log(snap.ref.path == "/bad_path/test") // true, previously silently wrong
  }

const snap = makeDataSnapshot(
    'hello world',
    '/bad_path/{id}', // Note mismatch with trigger ref
);
const params = {
    id: "test"
};
wrap(func)(snap, {params}); // New functionality throws an error on invocation due to mismatch between snap and trigger.

Tests

Added various tests for new functionality which changes DataSnapshot path, _isValidWildcardMatch, and _extractParamsFromPath.

All test pass, canonical uppercase sample tests also pass without change.

[laurenzlong]

I still need to take a more detailed look, but my high level feedback is that path supplied to makeDataSnapshot should populate context.params, not the other way around. Snapshots represent a particular database write that happened, so wildcards should not be supported in their construction.

[abeisgoat]

That seems reasonable. Let me know what you think once you take a good look and we'll make it work one way or the other!

[abeisgoat]

Alright, dropped out snapshot path population from params, so now params is populated from snapshot path and not the other way around.

Also added some better logging for when a dev accidentally passes URL with wildcards.

[laurenzlong]

You haven't fully removed the functionality for populating the snapshot's path from params. See inline comments.

As well, I would like to avoid casting the data as any, since bugs are harder to catch and the code is harder to maintain down the road. Your code currently also does not handle the case when the function is onWrite, therefore data is actually a Change object as opposed to a snapshot. It also doesn't seem to handle Firestore, which faces the same usability challenge.

Some suggestions:

• Modify database.makeSnapshot and firestore.makeDocumentSnapshot to throw if there's a wildcard in the path provided
• Within wrap, check __trigger.eventTrigger.eventType to see if the function being wrapped is RTDB or Firestore, and whether it's onWrite or onCreate/onDelete
• Depending on the check above, check data to ensure it is an instance of DataSnapshot, DocumentSnapshot, Change<DataSnapshot>, or Change<DocumentSnapshot>, throw error otherwise.
• Once you are assured the data is the right type, you can cast it as such. Proceed to get its path, and construct the params from it.

[laurenzlong]

Can you make makeDataSnapshot throw, instead of only catching it later when wrapped is called.

[laurenzlong]

this test is no longer relevant

[abeisgoat]

This test checks the snapshot's provided path against the route in the function definition, I think that's still relevant because a dev could still path a snapshot which has a completely wrong path for a trigger, right?

[abeisgoat]

Ah, I see now, it is a poorly written test, but the intent is still useful, I think. I've changed it to..

    it('should error when DataSnapshot path does not match trigger template', () => {
      const fft = new FirebaseFunctionsTest();
      fft.init();

      const snap = makeDataSnapshot(
          'hello world',
          '/different/at/nested/bat',
      );
      const wrapped = wrap(constructCF('google.firebase.database.ref.write'));
      expect(() => wrapped(snap, { params })).to.throw();
    });
  });

Does that seem more right?

[laurenzlong]

companies/firebase/users/user (second parameter to _extractParamsFromPath should not have wildcard, under the normal use of this function)

[laurenzlong]

companies/firebase/users/user for same reason as above

[laurenzlong]

Remaking the data snapshot isn't necessary if you are not populating the snapshot's path from params.

[abeisgoat]

That's what I thought, It is necessary for standardizing the path to remove extra /, however I'll move this logic to makeSnapshot as you're right that it's a better place for it.

[laurenzlong]

this is also not necessary if you do a check within makeSnapshot to ensure the snapshot path doesn't have wildcards.

[laurenzlong]

@abeisgoat Do you have bandwidth to pick this up again? The test is broken and there's been some merge conflicts, but I think we're close!

[rhodgkins]

I think this has been superseded by #114

[TheIronDev]

I think this has been superseded by #114

I agree with this. Marking this as closed, but happy to reevaluate too.