feat(data-connect): Add Message Preparation & Headers#9704
feat(data-connect): Add Message Preparation & Headers#9704stephenarosaj merged 10 commits intopasta/mainfrom
Conversation
|
Summary of ChangesHello, 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 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* **packages/data-connect/src/network/stream/streamTransport.ts** * Implemented the `_prepareMessage` method to manage stream request headers, including `authToken`, `appCheckToken`, `X-Goog-Api-Client`, and `x-firebase-gmpid`. * Added state tracking for `_isFirstStreamMessage` and `_lastSentAuthToken`. * Integrated `onConnectionReady` to reset stream state flags. * Removed the `onAuthTokenChanged` method. * **packages/data-connect/test/unit/streamTransport.test.ts** * Added new unit tests for the `AbstractDataConnectStreamTransport` to verify the correct behavior of `_prepareMessage` regarding header and name attachment under various conditions.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
|
There was a problem hiding this comment.
Code Review
This pull request introduces stateful message preparation for stream transports, adding logic to handle headers for authentication, telemetry, and connection handshakes. While the changes are generally well-tested, a critical security vulnerability was identified where the authentication state is not correctly cleared on the server upon client sign-out, potentially leading to session persistence.
dconeybe
left a comment
There was a problem hiding this comment.
LGTM with some minor suggestions.
| * the stream. | ||
| */ | ||
| get connectorResourcePath(): string { | ||
| return `projects/${this._project}/locations/${this._location}/services/${this._serviceName}/connectors/${this._connectorName}`; |
There was a problem hiding this comment.
Optional: Consider generating this string in the constructor and saving it to a private variable. IIUC, its value will never change during the lifetime of the object, and saving its value to an instance variable communicates this clearly.
Overview
✨ This PR focuses on the stateful construction of outgoing messages, ensuring that authentication, telemetry, and connection handshake metadata are correctly applied according to the stream's state.
Key Changes
src/network/stream/streamTransport.ts:_prepareMessageprivate method.name) to the first message of every stream.authToken(sent on the first message or when the token changes).appCheckToken(sent on the first message).X-Goog-Api-Client(SDK telemetry).x-firebase-gmpid(App ID)._isFirstStreamMessageand_lastSentAuthTokentracking properties.onConnectionReadyto reset state flags for new physical connections.connectorResourcePathgetter: Encapsulates resource path construction.Testing
New unit tests were added in
test/unit/streamTransport.test.tsto verify the stateful header logic:nameand all headers are present on the initial request.name/headers meant only for the first message are not included in future messages.authTokenis present when it changes.