Adds emulator support for v2 rtdb triggers #5045
Conversation
Codecov ReportBase: 55.88% // Head: 55.47% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #5045 +/- ##
==========================================
- Coverage 55.88% 55.47% -0.42%
==========================================
Files 305 307 +2
Lines 20324 20986 +662
Branches 4094 4327 +233
==========================================
+ Hits 11358 11641 +283
- Misses 8011 8385 +374
- Partials 955 960 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
colerogers
force-pushed
the
colerogers.rtdb-emulator
branch
from
eb0f441
to
41f0bc3
Compare
| @@ -819,7 +862,7 @@ export class FunctionsEmulator implements EmulatorInstance { | |||
| logger.debug(`addPubsubTrigger`, JSON.stringify({ eventTrigger })); | |||
|
|
|||
| // "resource":\"projects/{PROJECT_ID}/topics/{TOPIC_ID}"; | |||
| const resource = eventTrigger.resource; | |||
| const resource = eventTrigger.resource!; | |||
There was a problem hiding this comment.
is this ! necessary here?
There was a problem hiding this comment.
We make def.resource string | undefined now
|
|
||
| const ref = eventTrigger.eventFilterPathPatterns?.ref; | ||
| if (!ref) { | ||
| throw new FirebaseError("A database reference must be supplied."); |
There was a problem hiding this comment.
q: What does it mean for these values to not exist (in other words - can these conditions be triggered if users are using the function sdk correctly?)
There was a problem hiding this comment.
No, but I appreciate treating this as untrusted input. We could have new SDK authors pop these assertions because they have bugs in their implementation.
There was a problem hiding this comment.
Correct, using the Functions SDK as it currently is will never achieve this state
| namespacePattern: instance, | ||
| }); | ||
|
|
||
| const apiPath = "/.settings/functionTriggers.json"; |
There was a problem hiding this comment.
does difference in how we construct the apiPath for v1 and v2 mean that v2 rtdb emulation only works on the default RTDB instance? or existence of namespacePattern imply that it's a v2 trigger?
(whatever ends up being true - can we add a comment explaining this?)
There was a problem hiding this comment.
namespacePattern implies that we are using the v2 interface. Under the hood, the ?ns=... query param will be ignored for v2 triggers, but I figured it would be better to show that explicitly here.
There was a problem hiding this comment.
Added a comment explaining this
|
|
||
| const ref = eventTrigger.eventFilterPathPatterns?.ref; | ||
| if (!ref) { | ||
| throw new FirebaseError("A database reference must be supplied."); |
There was a problem hiding this comment.
No, but I appreciate treating this as untrusted input. We could have new SDK authors pop these assertions because they have bugs in their implementation.
| } | ||
| if (EventUtils.isBinaryCloudEvent(req)) { | ||
| reqBody = EventUtils.extractBinaryCloudEventContext(req); | ||
| reqBody.data = req.body; |
There was a problem hiding this comment.
I wonder if we actually wanted a third option: check for contenttype to include cloudevent but then throw that structured encoding is not supported if isBinaryCloudEvent is false.
There was a problem hiding this comment.
We are actually removing the check for content type to include cloudevent since it's not in the spec - https://cloud.google.com/eventarc/docs/workflows/cloudevents#payload-format
| @@ -50,10 +58,11 @@ export interface EventSchedule { | |||
| } | |||
|
|
|||
| export interface EventTrigger { | |||
| resource: string; | |||
| resource: string | undefined; | |||
| @@ -1,4 +1,4 @@ | |||
| export const PUBSUB_PUBLISH_EVENT = "google.cloud.pubsub.topic.v1.messagePublished" as const; | |||
There was a problem hiding this comment.
Do you know why as const was there in the first place? Was it used as a string literal type somewhere & isn't now?
There was a problem hiding this comment.
I'm not sure why we had it like this, maybe leftover from a different implementation?
…igger register by platform
colerogers
force-pushed
the
colerogers.rtdb-emulator
branch
from
03556f2
to
49caa0c
Compare
This change tries to make the minimum amount of changes to the functions emulator to support v2 rtdb triggers. Once we get some breathing room in the future, I'd like to refactor how triggers are added and possibly merge some of the deploy/emulator code so we aren't re-building similar logic.