-
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?
Implement V1 compat API for PubSub #1782
Conversation
Summary of ChangesHello @shettyvarun268, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the v2 Pub/Sub CloudEvent handling by integrating v1-compatible argument accessors. The primary goal is to facilitate a seamless migration for developers transitioning their existing v1 Cloud Functions to the v2 signature. By providing direct property access to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
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.
Code Review
This pull request introduces a v1-compatibility layer for v2 Pub/Sub functions by adding a lazily-computed context property to the CloudEvent object. The implementation is well-structured, using Object.defineProperty for lazy evaluation and providing fallbacks for constructing the resource name. I've identified a bug in the resource name construction for a fallback case and a couple of areas for improvement in the tests to make them more robust and accurate. My feedback includes a high-severity fix for the resource name format and medium-severity suggestions to improve test coverage and correctness.
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.
Code Review
The pull request successfully implements V1-compatible API for PubSub events in V2 Cloud Functions, which will greatly assist in migrating existing business logic. The changes introduce "context" and "message" getters to the CloudEvent<MessagePublishedData> class, along with robust helper functions attachPubSubContext and getResourceName to construct the V1 EventContext. The new test cases thoroughly cover the functionality, including handling existing contexts and fallback mechanisms for resource name extraction.
| * @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) { |
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.
i don't think we strictly need topic. all information should be derivable from event obj.
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.
Made the change and got rid of the topic.
src/v2/providers/pubsub.ts
Outdated
| */ | ||
| function getResourceName(event: CloudEvent<MessagePublishedData<any>>, topic: string) { | ||
| const match = event.source?.match(/projects\/([^/]+)\/topics\/([^/]+)/); | ||
| const project = |
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.
i think this code is overly defensive.
cloudevent is a standard format, and it's unlikely you won't be able to retrieve project/topic information from the event payload. defaulitng to "unknown" or "" is probably fine in the unlikely case we can't derive the info.
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.
Good point. Removed the condition. Also removed the corresponding test case because it won't be needed.
| 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); |
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.
ptal look at v1 pubsub signature:
(message: Message, context: EventContext) => PromiseLike<any> We also need a getter for message as well as context
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.
Got it. Made the needed changes. Added the message getter. This would allow us to access event.messag directly now.
spec/v2/providers/pubsub.spec.ts
Outdated
| ); | ||
| }); | ||
|
|
||
| //Test case to ensure Idempotency. makes things dont break if there is already context present |
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.
this comment seems redudant
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.
Noted. Addressed this by removing the test case below.
spec/v2/providers/pubsub.spec.ts
Outdated
|
|
||
| let receivedEvent: CloudEvent<pubsub.MessagePublishedData<any>>; | ||
| const func = pubsub.onMessagePublished("topic", (e) => { | ||
| receivedEvent = e; |
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.
i'd prefer we try to use the context here directly:
| receivedEvent = e; | |
| { message, context } = e; |
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.
Made the changes. Used deconstructedMessage and context instead
spec/v2/providers/pubsub.spec.ts
Outdated
| }); | ||
|
|
||
| //Test case to ensure Idempotency. makes things dont break if there is already context present | ||
| it("should not modify a CloudEvent that already has a context", async () => { |
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.
not sure why this exist? why would there be existing context?
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.
Understood. The real cases won't have an existing case. Removed this test case.
| /** V1- compatible context of this event. | ||
| * | ||
| * This getter is added at runtime for V1 compatibility. | ||
| * May be undefined it not set by a provider | ||
| */ |
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.
@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
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 you like me to make the comments more informative and developer friendly. Considering it might be put into the reference docs directly from here?
|
|
||
| }; | ||
|
|
||
| Object.defineProperty(event, "context", { |
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.
for my own learning, is defineProperty used here so that event.context isn't settable?
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.
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.
| * @packageDocumentation | ||
| */ | ||
|
|
||
| import type { EventContext } from "../v1/cloud-functions"; |
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 1stGenEventContext or LegacyEventContext so 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?
| * @param topic - The topic the event is for. | ||
| */ | ||
| function attachPubSubContext<T>(event: CloudEvent<MessagePublishedData<T>>, topic: string) { | ||
| if ("context" in event && event.context) { |
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.
when would context already be in the event? should this throw if that happens?
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.
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?
This PR adds the v1-compatible argument getters (context and message) directly to the v2 Pub/Sub CloudEvent class.
This allows developers to access v1-style arguments via property access, e.g., const { message, context } = event;, for a smoother migration of existing business logic to the v2 function signature.Implementation Details
The v1-compatible context getter is implemented as a lazily-computed property on the CloudEvent object. To correctly construct the v1 EventContext from the v2 event's properties, the following two internal helper functions are introduced:
attachPubSubContext<T>(event: CloudEvent<MessagePublishedData<T>>, topic: string):Adds the v1-style context property to the v2 event object using Object.defineProperty.
The getter computes the EventContext by mapping v2 properties (event.id, event.time) and by calling getResourceName.
This ensures the context is only computed once upon first access.
getResourceName(event: CloudEvent<MessagePublishedData<any>>, topic: string):Extracts the full resource name for the v1 EventContext resource object.
It attempts to parse the project ID and topic name from event.source or falls back to environment variables and the function's topic name parameter if necessary.
This is the first of a series of PRs aimed at simplifying the migration from v1 to v2. By introducing this new
onMessagePublished function, we are providing a more robust and flexible way to handle Pub/Sub events in Cloud Functions for Firebase v2.