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 significantly enhances the backward compatibility of V2 Cloud Functions by extending the V1 compatibility layer to include Google Cloud Storage events. This change ensures that functions migrating from V1 to V2 can seamlessly process Storage events, maintaining access to expected context and data structures. The refactoring centralizes compatibility logic and improves the overall robustness and maintainability of the event handling system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a V1 compatibility layer for Storage events by centralizing logic in a new compat.ts module, which is a solid architectural improvement. The refactoring to use constants for event types also enhances maintainability. However, I've identified a critical circular dependency between compat.ts and providers/storage.ts that leads to type-safety issues. An object that doesn't fully conform to the StorageEvent interface is passed to handlers, which could cause runtime errors. I've provided detailed comments and suggestions to resolve these issues.
shettyvarun268
force-pushed
the
feaure/Generic-v1-Compat-storage
branch
from
465b6ad to
585e081
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the V1 compatibility layer to support Google Cloud Storage events, which is a great enhancement. The implementation is solid, including the refactoring of event type strings into constants for better maintainability. I've identified a couple of areas for improvement: one to make a test less brittle and another to fix a potential bug when handling malformed storage events. My detailed comments include specific code suggestions to address these points.
| if (!v2) { | ||
| return {} as ObjectMetadata; | ||
| } |
There was a problem hiding this comment.
Returning an empty object {} cast as ObjectMetadata when v2 is falsy violates the ObjectMetadata type contract, which has several required properties (e.g., id, bucket, name). This could lead to runtime errors in downstream code that expects a valid ObjectMetadata object. A better approach for malformed events would be to throw an error, similar to how malformed Pub/Sub events are handled in this file.
| if (!v2) { | |
| return {} as ObjectMetadata; | |
| } | |
| if (!v2) { | |
| throw new Error("Malformed Storage event: missing 'data' property."); | |
| } |
spec/v2/compat.spec.ts
Outdated
| const rawEvent = { | ||
| specversion: "1.0", | ||
| source: "//storage.googleapis.com/projects/_/buckets/my-bucket/objects/my-object", | ||
| id: "event-id-stable-storage", | ||
| type: finalizedEvent, | ||
| time: new Date().toISOString(), | ||
| data: { | ||
| bucket: "my-bucket", | ||
| name: "my-object", | ||
| size: 1024, | ||
| timeCreated: new Date() | ||
| }, | ||
| } as any; | ||
|
|
||
| const patched = patchV1Compat(rawEvent) as any; |
There was a problem hiding this comment.
The mock rawEvent object is incomplete and cast to any. This makes the test brittle because it doesn't fully conform to the CloudEvent<StorageObjectData> type. If the implementation of patchV1Compat changes to access other required properties of StorageObjectData in the future, this test could fail unexpectedly.
To make the test more robust and self-documenting, it's better to provide a complete mock object that satisfies the type contract. This also has the benefit of removing the need for casting to any.
| const rawEvent = { | |
| specversion: "1.0", | |
| source: "//storage.googleapis.com/projects/_/buckets/my-bucket/objects/my-object", | |
| id: "event-id-stable-storage", | |
| type: finalizedEvent, | |
| time: new Date().toISOString(), | |
| data: { | |
| bucket: "my-bucket", | |
| name: "my-object", | |
| size: 1024, | |
| timeCreated: new Date() | |
| }, | |
| } as any; | |
| const patched = patchV1Compat(rawEvent) as any; | |
| const rawEvent: CloudEvent<import("../../src/v2/providers/storage").StorageObjectData> = { | |
| specversion: "1.0", | |
| source: "//storage.googleapis.com/projects/_/buckets/my-bucket/objects/my-object", | |
| id: "event-id-stable-storage", | |
| type: finalizedEvent, | |
| time: new Date().toISOString(), | |
| data: { | |
| bucket: "my-bucket", | |
| name: "my-object", | |
| size: 1024, | |
| timeCreated: new Date(), | |
| id: "my-bucket/my-object/1", | |
| generation: 1, | |
| metageneration: 1, | |
| storageClass: "STANDARD", | |
| updated: new Date(), | |
| }, | |
| }; | |
| const patched = patchV1Compat(rawEvent); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the V1 compatibility layer to support Google Cloud Storage events, which is a great addition. The implementation is solid, refactoring event type strings into constants for better maintainability. However, I've identified a bug in the new compatibility logic where generation and metageneration fields are not converted to strings, which could lead to issues for developers migrating V1 functions. I've also included suggestions to improve test coverage for this conversion and to enhance code quality by removing an unnecessary type casting in another test.
| if (v2.timeStorageClassUpdated instanceof Date) v1.timeStorageClassUpdated = v2.timeStorageClassUpdated.toISOString(); | ||
|
|
||
| // 'size' in V2 is number, V1 says string. | ||
| if (typeof v2.size === "number") v1.size = v2.size.toString(); |
There was a problem hiding this comment.
The V1 ObjectMetadata type expects generation and metageneration to be strings, but the V2 StorageObjectData provides them as numbers. The conversion logic is missing for these fields. This will cause a type mismatch for consumers of the V1 compatibility layer. Please add conversions for these fields from number to string, similar to how size is handled.
spec/v2/compat.spec.ts
Outdated
| }, | ||
| }; | ||
|
|
||
| const patched = patchV1Compat(rawEvent) as any; |
There was a problem hiding this comment.
The as any cast here can be removed. The function overloads for patchV1Compat should allow TypeScript to correctly infer the return type as StorageCloudEvent<StorageObjectData>, which has the getV1Compat method. Relying on type inference makes the code safer and easier to read.
| const patched = patchV1Compat(rawEvent) as any; | |
| const patched = patchV1Compat(rawEvent); |
| expect(v1Event.data).to.include({ | ||
| bucket: "my-bucket", | ||
| name: "my-object", | ||
| kind: "storage#object", | ||
| size: "1024", // V1 expects string | ||
| timeCreated: "2023-01-01T00:00:00.000Z", // V1 expects string | ||
| }); |
There was a problem hiding this comment.
This test for V1 compatibility is a good start. To make it more comprehensive, please also assert that generation and metageneration are correctly converted to strings in the V1 event data. This will help ensure the compatibility layer is working as expected for all relevant fields and would have caught the type conversion issue in compat.ts.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively extends the V1 compatibility layer to support Google Cloud Storage events. The changes are well-structured, introducing specific mapping logic for storage triggers and refactoring event type strings into constants for better maintainability. The addition of StorageCloudEvent and updates to patchV1Compat are implemented correctly. The accompanying tests are thorough, though there's a small opportunity to improve test determinism by using fixed dates instead of new Date(). Overall, this is a solid contribution that enhances backward compatibility for developers migrating storage functions.
1 participant
This PR extends the generic V1 compatibility layer to support Google Cloud Storage events. It builds upon the shared compat module introduced in the previous PR by adding specific mapping logic for generic Storage triggers.
We updated patchV1Compat to handle Storage events, correctly mapping V2 StorageObjectData to V1 ObjectMetadata (including necessary date and size type conversions). This logic is now applied to all Storage triggers (onObjectArchived, onObjectFinalized, onObjectDeleted, onObjectMetadataUpdated), ensuring that the context and data (as object) properties are available to V1-migrated functions.
This change ensures a consistent, backward-compatible experience for developers migrating Storage functions to V2. Additionally, we refactored event type strings into exported constants to improve maintainability across the provider and compliance tests. The StorageCloudEvent type is also exported for better type safety.