Add new 2nd gen Firestore auth context triggers #1519
Conversation
|
|
||
| /** | ||
| * Event handler which triggers when a document is updated in Firestore. | ||
| * |
There was a problem hiding this comment.
The following doc was not added here:
This trigger will also provide the authentication context of the principal who triggered the event.
src/v2/providers/firestore.ts
Outdated
| @@ -153,6 +177,50 @@ export function onDocumentWritten<Document extends string>( | |||
| return onChangedOperation(writtenEventType, documentOrOpts, handler); | |||
| } | |||
|
|
|||
| /** | |||
| * Event handler which triggers when a document is created, updated, or deleted in Firestore. | |||
There was a problem hiding this comment.
This and most other instances of "which" in these comments is better as "that" (more restrictive, rather than additive).
https://prowritingaid.com/art/438/-Which--or--That-%3A-Know-When-to-Use-Each.aspx
src/v2/providers/firestore.ts
Outdated
| @@ -153,6 +177,50 @@ export function onDocumentWritten<Document extends string>( | |||
| return onChangedOperation(writtenEventType, documentOrOpts, handler); | |||
| } | |||
|
|
|||
| /** | |||
| * Event handler which triggers when a document is created, updated, or deleted in Firestore. | |||
| * This trigger will also provide the authentication context of the principal who triggered the event. | |||
There was a problem hiding this comment.
Suggest "also provides"
Avoid "will" wherever you can. Please check this globally in this PR, thanks!
src/v2/providers/firestore.ts
Outdated
| @@ -95,6 +115,10 @@ export interface FirestoreEvent<T, Params = Record<string, string>> extends Clou | |||
| namespace: string; | |||
| /** The document path */ | |||
| document: string; | |||
| /** The type of principal that triggered the event */ | |||
| authType?: AuthType; | |||
There was a problem hiding this comment.
These will always be populated in the new event type, right? If so, we should create a separate event type where the fields are omitted in the previous event handlers and guaranteed to exist in the new ones.
There was a problem hiding this comment.
@exaby73 actually made the same suggestion when implementing the triggers in the Python SDK, but my reasoning for using the same event type is that we had discussed plans for aliasing onDocumentX to onDocumentXWithAuthContext in the next major release of the SDK, which may pollute the namespace a bit and cause confusion; a user writing a onDocumentX function will need to use the FirestoreEventWithAuthContext interface as opposed to just a single FirestoreEvent interface. The additional authId field is also optional and not guaranteed to exist in the delivered event.
But I also see the argument for having a separate event type. WDYT? Happy to make the changes and coordinate with @exaby73 to restore his old changes if we think the correct approach.
There was a problem hiding this comment.
I'm pretty strongly in favor of fields being non-optional when they will always be provided and omitted when they will never be provided (in TypeScript at least. I'm not the expert for Python)
src/v2/providers/firestore.ts
Outdated
| @@ -83,6 +101,8 @@ export type DocumentSnapshot = firestore.DocumentSnapshot; | |||
| /** A Firestore QueryDocumentSnapshot */ | |||
| export type QueryDocumentSnapshot = firestore.QueryDocumentSnapshot; | |||
|
|
|||
| type AuthType = "service_account" | "api_key" | "system" | "unauthenticated" | "unknown"; | |||
There was a problem hiding this comment.
Can we document when "unknown" will happen and if there are any assumptions the dev can make (e.g. that it's from an admin context)?
There was a problem hiding this comment.
Yep the CL is a few months old, quite a few things have changed in the meantime!
blidd-google
force-pushed
the
blidd.firestore-v2-auth-context
branch
from
393c5e1
to
0c51861
Compare
This reverts commit 0c51861.
This reverts commit aa8df0f.
* Ugly typesafety * Formatter * consolidate make firestore event fns and simplify typings --------- Co-authored-by: Brian Li <[email protected]>
src/v2/providers/firestore.ts
Outdated
| // const fn = ( | ||
| // event: FirestoreEvent<Change<QueryDocumentSnapshot> | undefined, ParamsOf<Document>> & { | ||
| // foo: string; | ||
| // } |
|
migration guide link in the original post is 404. |
Auth context support has only been available for RTDB 1st gen functions — until now! The Firestore and Eventarc teams have recently added support for Firestore Cloud Events to include event actor information, adding four new event types. These changes enable us to offer four new 2nd gen Firestore trigger types whose events will additionally contain the auth context along with the existing event data:
onDocumentWrittenWithAuthContextonDocumentUpdatedWithAuthContextonDocumentCreatedWithAuthContextonDocumentDeletedWithAuthContextThe API is very similar to the existing 2nd gen Firestore triggers API; however, users should take special care to avoid event loss when migrating from the original 2nd gen Firestore trigger to the auth-context trigger. See the migration guide for more details about best practices.