Storage Emulator Enhancement #3809

colerogers · 2021-10-07T20:50:19Z

Description

Storage Emulator Enhancement

…ator

…e-tools into colerogers.gcs-emulator

…ator

inlined

LGTM but please make sure someone who is familiar with the emulator code reviews too.

taeold

lgtm

taeold · 2021-10-13T23:44:25Z

src/emulator/storage/cloudFunctions.ts


 export class StorageCloudFunctions {
  private logger = EmulatorLogger.forEmulator(Emulators.STORAGE);
  private functionsEmulatorInfo?: EmulatorInfo;
  private multicastOrigin = "";
  private multicastPath = "";
  private enabled = false;
+  private client: Client = new Client({ urlPrefix: "" });


Is there a reason we assign a new client object here? It seems like we always create one in the constructor.

Since we only initialize in the constructor when functionsEmulator is set, we need to have client be be initialized to some default value (can't be null/undefined) so this seemed like a reasonable place to set a default value

I'm fine with what we've done here, but from strictly pedantic standpoint, I think it make more sense to let client be an optional member of this class and then add a guard in the dispatch call to exit early to do nothing if we know that the functionsEmulator is not running (i.e. functionsEmulatorInfo is undefined).

src/emulator/storage/cloudFunctions.ts

taeold · 2021-10-13T23:55:57Z

src/emulator/storage/cloudFunctions.ts

+    /** Cloud Event */
+    const ceAction = STORAGE_V2_ACTION_MAP[action];
+    const data = { ...objectMetadataPayload };
+    delete (data as any).acl; // remove this field from cloud events


I think owner property needs to be removed.

In fact, maybe we can leverage the existing type definition at https://github.com/googleapis/google-cloudevents-nodejs/blob/main/cloud/storage/v1/StorageObjectData.ts to typecheck the data object, e.g.

const data: StorageObjectData = { ... }

So this is kind of funny, the emulator doesn't actually send back the full object (

firebase-tools/src/emulator/storage/metadata.ts

Line 368 in e4b9b93

export class CloudStorageObjectMetadata {

), just a partial object (both acl & owner are missing).

But you made a good point, I imported StorageObjectData, casted objectMetaDataPayload to unknown then to StorageObjectData (b/c we had some type mismatches)

src/emulator/storage/cloudFunctions.ts

taeold · 2021-10-14T00:14:41Z

src/emulator/storage/cloudFunctions.ts

+    const cloudEvent: string = JSON.stringify({
+      eventType: `google.cloud.storage.object.v1.${ceAction}`, // need eventType for multicast triggerId
+      data: ce,
+    });


Huh I'm surprised that this works since I expected the body of the cloudevent http call to look like:

{ specVersion: 1, type: "google.cloud.storage.object.v1..." source: "..." data: { ... } }

vs

{ data: { specVersion: 1, type: "google.cloud.storage.object.v1..." source: "..." data: { ... } } }

Something must be unboxing the nested data property before sending it off to the function? Can you help me step through how this works?

Looks like I had the changes stashed locally from branch switching, I ended up changing it to the comment below, having an eventFormat field and changing the proto based on that.

taeold · 2021-10-14T00:31:07Z

src/emulator/storage/cloudFunctions.ts

+      data,
+    };
+    const cloudEvent: string = JSON.stringify({
+      eventType: `google.cloud.storage.object.v1.${ceAction}`, // need eventType for multicast triggerId


Hmm this implementation feels a bit brittle since eventType isn't a thing in a cloudevent payload. It also makes an implicit assumption that the name of the event will always be different between legacy and cloudevent. That is actually, probably true, but it's a subtle assumption that's worth being explicit (either in code or in implementation).

Another way to implement this change would be to teach the multicast handler to understand the difference between cloudevent and legacy event payloads (either by having different endpoint, having a wrapper request body e.g. { eventFormat: "cloudevent", eventBody: body }, etc. Do you think that is going to work?

I kept it as eventType since I wanted to keep multicast handler almost unchanged. But since CloudEvents moved eventType to type we can rely on that to figure out our trigger key. Having a wrapper request makes sense even though it now adds CloudEvents specific logic into the functionsEmulator

Maybe instead of a wrapper, we just build our triggerKey like this:

const triggerKey = proto.type ? `${this.args.projectId}:${proto.type}` : `${this.args.projectId}:${proto.eventType}`; const triggers = this.multicastTriggers[triggerKey] || [];

Discussed offline - we will probably use the value of the Content-Type header to differentiate cloudevent vs legacy payload.

Also, I think this already works w/o any modification, but I'd make sure that the change we make here is compatible with trigger registration:

firebase-tools/src/emulator/functionsEmulator.ts

Lines 695 to 703 in e4b9b93

addStorageTrigger(projectId: string, key: string, eventTrigger: EventTrigger): boolean {

logger.debug(`addStorageTrigger`, JSON.stringify({ eventTrigger }));

const eventTriggerId = `${projectId}:${eventTrigger.eventType}`;

const triggers = this.multicastTriggers[eventTriggerId] || [];

triggers.push(key);

this.multicastTriggers[eventTriggerId] = triggers;

return true;

}

taeold

Overall LGTM. You may want to think about adding some test cases in https://github.com/firebase/firebase-tools/tree/master/scripts/triggers-end-to-end-tests for the new storage triggers (old legacy style and new cloudevent style if you are willing to contribute some time).

src/emulator/storage/cloudFunctions.ts

colerogers · 2021-10-18T14:36:17Z

Don't merge until #3829 completes

…ator

* storage trigger works * removing comment * cleaning up request body code * add types * fixing comments * removing wrapper, adding check on type field * using Content-Type header * linter * nit

storage trigger works

a7b0b2c

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Oct 7, 2021

colerogers changed the title ~~storage trigger works~~ Storage Emulator Enhancement Oct 7, 2021

colerogers and others added 7 commits October 7, 2021 13:50

Merge branch 'master' into colerogers.gcs-emulator

dda4142

removing comment

59d5688

Merge remote-tracking branch 'origin/master' into colerogers.gcs-emul…

f70c491

…ator

Merge branch 'colerogers.gcs-emulator' of github.com:firebase/firebas…

50e15e9

…e-tools into colerogers.gcs-emulator

cleaning up request body code

0071b5d

Merge remote-tracking branch 'origin/master' into colerogers.gcs-emul…

a7944df

…ator

add types

797d749

colerogers marked this pull request as ready for review October 13, 2021 19:18

colerogers requested review from inlined, taeold and abeisgoat October 13, 2021 19:18

inlined approved these changes Oct 13, 2021

View reviewed changes

taeold reviewed Oct 14, 2021

View reviewed changes

colerogers added 4 commits October 14, 2021 09:10

fixing comments

afd4be5

removing wrapper, adding check on type field

3ca97a9

using Content-Type header

0816f36

linter

b1be932

taeold approved these changes Oct 16, 2021

View reviewed changes

src/emulator/storage/cloudFunctions.ts Outdated Show resolved Hide resolved

colerogers added 2 commits October 18, 2021 11:40

nit

a47df18

Merge remote-tracking branch 'origin/master' into colerogers.gcs-emul…

d4b4208

…ator

colerogers merged commit 925bdc3 into master Oct 19, 2021

colerogers deleted the colerogers.gcs-emulator branch October 19, 2021 16:12

colerogers added the perf-h1-2022 label Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage Emulator Enhancement #3809

Storage Emulator Enhancement #3809

colerogers commented Oct 7, 2021

inlined left a comment

taeold left a comment

taeold Oct 13, 2021

colerogers Oct 14, 2021

taeold Oct 14, 2021

taeold Oct 13, 2021

colerogers Oct 14, 2021

taeold Oct 14, 2021

colerogers Oct 14, 2021

taeold Oct 14, 2021

colerogers Oct 14, 2021

colerogers Oct 14, 2021

taeold Oct 14, 2021 •

edited

taeold left a comment

colerogers commented Oct 18, 2021

	addStorageTrigger(projectId: string, key: string, eventTrigger: EventTrigger): boolean {
	logger.debug(`addStorageTrigger`, JSON.stringify({ eventTrigger }));

	const eventTriggerId = `${projectId}:${eventTrigger.eventType}`;
	const triggers = this.multicastTriggers[eventTriggerId] \|\| [];
	triggers.push(key);
	this.multicastTriggers[eventTriggerId] = triggers;
	return true;
	}

Storage Emulator Enhancement #3809

Storage Emulator Enhancement #3809

Conversation

colerogers commented Oct 7, 2021

Description

inlined left a comment

Choose a reason for hiding this comment

taeold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taeold Oct 14, 2021 • edited

Choose a reason for hiding this comment

taeold left a comment

Choose a reason for hiding this comment

colerogers commented Oct 18, 2021

taeold Oct 14, 2021 •

edited