Try to extract context.params from triggered data #114
Conversation
| if (eventType.startsWith('google.firebase.database.ref.')) { | ||
| let data: database.DataSnapshot; | ||
| if (eventType.endsWith('.write')) { | ||
| // Triggered with change | ||
| if (!(triggerData instanceof Change)) { | ||
| throw new Error('Must be triggered by database change'); | ||
| } | ||
| data = triggerData.before; | ||
| } else { | ||
| data = triggerData as any; | ||
| } | ||
| triggerParams = _extractDatabaseParams(eventResource, data); | ||
| } else if (eventType.startsWith('google.firestore.document.')) { | ||
| let data: firestore.DocumentSnapshot; | ||
| if (eventType.endsWith('.write')) { | ||
| // Triggered with change | ||
| if (!(triggerData instanceof Change)) { | ||
| throw new Error('Must be triggered by firestore document change'); | ||
| } | ||
| data = triggerData.before; | ||
| } else { | ||
| data = triggerData as any; | ||
| } | ||
| triggerParams = _extractFirestoreDocumentParams(eventResource, data); |
There was a problem hiding this comment.
I don't really like these bits but not really sure there's a better way?
I was thinking to check data instanceof database.DataSnapshot (or firestore.DocumentSnapshot) and throwing an Error but then if you call the wrapped function with an actual fetched database.DataSnapshot (using the admin SDK) it would fail that check which would be annoying!
|
Can anyone review and, if happy, merge this in please? |
spec/main.spec.ts
Outdated
| expect(context.auth).to.equal(null); | ||
| expect(context.authType).to.equal('UNAUTHENTICATED'); | ||
| }); | ||
| context('database functions', () => { |
There was a problem hiding this comment.
This seems to be a refactor without a benefit, unless I"m missing something. We also just use describe in our codebases instead of context. We also generally use beforeEach over before because the nanosecond we save by not calling a setup function repeatedly is less important than hairpulling if a test changes some of the state between runs.
There was a problem hiding this comment.
Cool, will update context / before usage 👍 (and context usage below)
The refactor is so change can be defined for the RTDB function tests, the current tests just used some junk data ('data') so wasn't actually a Change object with a before and after. Because we're now using the incoming paths to populate the params the input in to the RTDB functions is now validated and has to be a Change or a DataSnapshot (depending on the function type).
rhodgkins
force-pushed
the
params
branch
2 times, most recently
from
a01ce37 to
78a2bc8
Compare
|
@inlined done and rebased from |
Description
When running the function, the wrapper tries to extract the
EventContextparamsfrom the triggered data based on the wildcard path.E.g. A wildcard path of
users/{userId}run against withusers/FOOwould result inparams: { userId: 'FOO' }.Provided
paramsto the wrapped function are preferred over the extracted ones - I've only done this to prevent breaking usage for anyone whose not matching the values in the path up to the providedparams.Also added tests (and updated the
auth and authTypetests as they were just being run with junk data).Code sample
See the tests:
Firebase RTDB - https://github.com/bookcreator/firebase-functions-test/blob/params/spec/main.spec.ts#L150-L165
Firestore - https://github.com/bookcreator/firebase-functions-test/blob/params/spec/main.spec.ts#L167-L185