-
Notifications
You must be signed in to change notification settings - Fork 219
Implement V1 compat API for PubSub #1782
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| * @packageDocumentation | ||
| */ | ||
|
|
||
| import type { EventContext } from "../v1/cloud-functions"; | ||
| import { Change } from "../common/change"; | ||
| import { ManifestEndpoint } from "../runtime/manifest"; | ||
|
|
||
|
|
@@ -91,6 +92,13 @@ | |
|
|
||
| /** Information about this specific event. */ | ||
| data: T; | ||
|
|
||
| /** V1- compatible context of this event. | ||
| * | ||
| * This getter is added at runtime for V1 compatibility. | ||
| * May be undefined it not set by a provider | ||
| */ | ||
|
Comment on lines
+96
to
+100
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @egilmorez heads up, copy here will go into the reference docs on this page: https://firebase.google.com/docs/reference/functions/2nd-gen/node/firebase-functions.cloudevent
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you like me to make the comments more informative and developer friendly. Considering it might be put into the reference docs directly from here? |
||
| readonly context?: EventContext; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
| import * as options from "../options"; | ||
| import { SecretParam } from "../../params/types"; | ||
| import { withInit } from "../../common/onInit"; | ||
| import type { EventContext, Resource } from "../../v1/cloud-functions"; | ||
|
|
||
| /** | ||
| * Google Cloud Pub/Sub is a globally distributed message bus that automatically scales as you need it. | ||
|
|
@@ -304,7 +305,9 @@ | |
| subscription: string; | ||
| }; | ||
| messagePublishedData.message = new Message(messagePublishedData.message); | ||
| return wrapTraceContext(withInit(handler))(raw as CloudEvent<MessagePublishedData<T>>); | ||
| const event = raw as CloudEvent<MessagePublishedData<T>>; | ||
| attachPubSubContext(event, topic); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ptal look at v1 pubsub signature: (message: Message, context: EventContext) => PromiseLike<any> We also need a getter for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Made the needed changes. Added the message getter. This would allow us to access event.messag directly now. |
||
| return wrapTraceContext(withInit(handler))(event); | ||
| }; | ||
|
|
||
| func.run = handler; | ||
|
|
@@ -353,3 +356,70 @@ | |
|
|
||
| return func; | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| * | ||
| * Adds a v1-style context to the event. | ||
| * | ||
| * @param event - The event to add the context to. | ||
| * @param topic - The topic the event is for. | ||
| */ | ||
| function attachPubSubContext<T>(event: CloudEvent<MessagePublishedData<T>>, topic: string) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't think we strictly need
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the change and got rid of the topic. |
||
| if ("context" in event && event.context) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when would context already be in the event? should this throw if that happens?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Changed the code to throw an error in this case. The context existing would be unlikely. Would you prefer I get rid of the check altogether? |
||
| throw new Error("Unexpected context in event."); | ||
| } | ||
|
|
||
| const resourceName = getResourceName(event, topic); | ||
| const resource: Resource = { | ||
|
|
||
| service: "pubsub.googleapis.com", | ||
| name: resourceName ?? "", | ||
|
|
||
| }; | ||
|
|
||
| const context: EventContext = { | ||
| eventId: event.id, | ||
| timestamp: event.time, | ||
| resource, | ||
| eventType: "google.cloud.pubsub.topic.v1.messagePublished", | ||
| params: {} | ||
|
|
||
| }; | ||
|
|
||
| Object.defineProperty(event, "context", { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for my own learning, is
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. This is to ensure event.context a read-only property so as to not accidentally end up changing it inside the function. |
||
|
|
||
| get: () => context, | ||
| enumerable: false, | ||
| configurable: false, | ||
|
|
||
| }); | ||
|
|
||
| Object.defineProperty(event, "message", { | ||
| get: () => (event.data as MessagePublishedData<T>).message, | ||
| enumerable: false, | ||
| configurable: false, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| * | ||
| * Extracts the resource name from the event source. | ||
| * | ||
| * @param event - The event to extract the resource name from. | ||
| * @param topic - The topic the event is for. | ||
| * @returns The resource name. | ||
| */ | ||
| function getResourceName(event: CloudEvent<MessagePublishedData<any>>, topic: string) { | ||
| const match = event.source?.match(/projects\/([^/]+)\/topics\/([^/]+)/); | ||
| const project = match?.[1]; | ||
| const topicName = match?.[2] ?? topic; | ||
|
|
||
| if (!project) { | ||
| return `projects/unknown-project/topics/${topicName}`; | ||
| } | ||
|
|
||
| return `projects/${project}/topics/${topicName}`; | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth redefining this as
1stGenEventContextorLegacyEventContextso that it's clear it is not really meant for people using 2nd gen?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does v1EventContext sound more clear and concise?