Skip to content

Update CloudEvent generation to replace data instead of deep merge #144

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

Closed
wants to merge 5 commits into from

Conversation

TheIronDev
Copy link
Contributor

@TheIronDev TheIronDev commented May 10, 2022

Description

CloudEvent.data is defined as an arbitrary payload. For some
CloudEvents it may be a string, for others it may be an object.

Theres a few special cases that need to be handled with data:

  • eventarc - initially returns data as JSON, it can be returned as a primitive
  • pubsub - data.message.json should be an opaque value

Code sample

const data = {
  message: {
    json: { firebase: 'test' },
    data: 'eyJmaXJlYmFzZSI6InRlc3QifQ==',
  },
  subscription: 'subscription',
};
const cloudFn = pubsub.onMessagePublished('topic', handler);
const cloudFnWrap = wrapV2(cloudFn);
const cloudEventPartial = { data };

expect(cloudFnWrap(cloudEventPartial).cloudEvent.data.message).to.not.include({
  data: 'eyJoZWxsbyI6IndvcmxkIn0=', // Note: would be a mismatch from the json
});
expect(cloudFnWrap(cloudEventPartial).cloudEvent.data.message.json)
  .to.equal(data.message.json);

`CloudEvent.data` is defined as an arbitrary payload. For some
CloudEvents it may be a string, for others it may be an object.

Theres a few special cases that need to be handled with data:

* eventarc - initially returns `data` as JSON, it can be returned as a primitive
* pubsub - `data.message.json` should be an opaque value
@TheIronDev TheIronDev requested a review from inlined May 10, 2022 21:02
@TheIronDev TheIronDev force-pushed the tystark.deep_merge_no_data branch from be7bbfb to 4c72b0f Compare May 10, 2022 21:05
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the behavior as is except for when we know the schema is a map. In cases where something is logically a map of string to string we can say that overriding anything in that map overrides the whole map. So for example, setting content type on a storage object still leaves content size present, but overriding a pubsub message's json attribute overrides the whole json.

@TheIronDev
Copy link
Contributor Author

Let's keep the behavior as is except for when we know the schema is a map. In cases where something is logically a map of string to string we can say that overriding anything in that map overrides the whole map. So for example, setting content type on a storage object still leaves content size present, but overriding a pubsub message's json attribute overrides the whole json.

Would that be something as simple as:

const combined = merge(generated, partial);

if (combined?.data?.message?.json && partial?.data?.message?.json) {
  combined.data.message.json = partial.data.message.json;
}

Or is there something bigger I am overlooking?

@TheIronDev TheIronDev requested a review from inlined May 11, 2022 19:54
@inlined
Copy link
Member

inlined commented May 12, 2022

/**
  * Create a new T given an initial set of values and a value to be merged in deeply.
  * any field mask in opaqueKeys mean that any value in delta would overwrite base
  * starting at that field.
  * @example
  * const original = {
  *   nested: { key1: "val1", key2: "val2"},
  *   data: { json: { key: "value" } }
  * };
  * const delta = {
  *   nested: { key1: "otherval1" },
  *   data: { json: { ohterKey: "otherValue" } },
  * };
  * const result = deepMerge(original, delta, "data.json");
  * // result is {
  * //   nested: { key1: "otherval1", key2: "val2" },
  * //   data: { json: { otherKey: "otherValue" }},
  * // }
  */
deepMerge<T>(base: T, delta: RecursivePartial<T>, opaqueKeys... RecursiveKeysOf<T>[]): T;

At least that's how I'd do it if I wanted to solve it generically. Otherwise you can do piecemeal overrides everywhere. This would solve json + data for Pub/Sub:

const combined = merge(generated, partial);

if (partial.data?.message?.json) {
  combined.data.message.json = partial.data!.message!.json;
  if (!patial.data?.message?.data) {
    combined.data.message.data = Buffer.from(JSON.stringify(partial.data!.message!.data)).toString("base64");
  }
} else if (partial.data?.message?.data) {
  Object.addProperty(combined.data.message, "json", {
    get: () => JSON.parse(Buffer.from(partial.data!.message!.data, "base64").toString())
  });
}

* Update to README.md to include more samples to get started
Storage CloudEvents contain a "bucket" field. The mock data should
return this "bucket" field to accurately reflect live data.

[More info here](https://github.com/firebase/firebase-functions/blob/27a49837ea7b74d9d2926e74c387f45704d57889/src/v2/providers/storage.ts#L185)
... But I'm not convinced this is the best approach.
@TheIronDev
Copy link
Contributor Author

Closing this PR in favor of #146

@TheIronDev TheIronDev closed this May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants