Skip to content

Update Rtdb test sdk to allow json for CloudEvent #159

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Update Rtdb test sdk to allow json for CloudEvent #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcfeea5
Update Rtdb test sdk to allow json for CloudEvent
TheIronDev Aug 3, 2022
1481939
Updates per PR feedback
TheIronDev Aug 4, 2022
eb3d89b
Update src/cloudevent/mocks/database/helpers.ts
TheIronDev Aug 4, 2022
77b831b
Added ChangeLike type, and reverted an Object to object (lint errror)
TheIronDev Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions spec/v2.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,19 @@ describe('v2', () => {

expect(cloudEvent.data.val()).deep.equal(dataVal);
});

it('should accept json data', () => {
const referenceOptions = {
ref: 'foo/bar',
instance: 'instance-1',
};
const cloudFn = database.onValueCreated(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const dataVal = { snapshot: 'override' };
const cloudEvent = cloudFnWrap({ data: dataVal }).cloudEvent;

expect(cloudEvent.data.val()).deep.equal(dataVal);
});
});

describe('database.onValueDeleted()', () => {
Expand Down Expand Up @@ -563,6 +576,19 @@ describe('v2', () => {

expect(cloudEvent.data.val()).deep.equal(dataVal);
});

it('should accept json data', () => {
const referenceOptions = {
ref: 'foo/bar',
instance: 'instance-1',
};
const cloudFn = database.onValueDeleted(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const dataVal = { snapshot: 'override' };
const cloudEvent = cloudFnWrap({ data: dataVal }).cloudEvent;

expect(cloudEvent.data.val()).deep.equal(dataVal);
});
});

describe('database.onValueUpdated()', () => {
Expand Down Expand Up @@ -610,6 +636,23 @@ describe('v2', () => {
expect(cloudEvent.data.before.val()).deep.equal(beforeDataVal);
expect(cloudEvent.data.after.val()).deep.equal(afterDataVal);
});

it('should accept json data', () => {
const referenceOptions = {
ref: 'foo/bar',
instance: 'instance-1',
};
const cloudFn = database.onValueUpdated(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const afterDataVal = { snapshot: 'after' };
const beforeDataVal = { snapshot: 'before' };
const data = { before: beforeDataVal, after: afterDataVal };

const cloudEvent = cloudFnWrap({ data }).cloudEvent;

expect(cloudEvent.data.before.val()).deep.equal(beforeDataVal);
expect(cloudEvent.data.after.val()).deep.equal(afterDataVal);
});
});

describe('database.onValueWritten()', () => {
Expand Down Expand Up @@ -657,6 +700,24 @@ describe('v2', () => {
expect(cloudEvent.data.before.val()).deep.equal(beforeDataVal);
expect(cloudEvent.data.after.val()).deep.equal(afterDataVal);
});

it('should accept json data', () => {
const referenceOptions = {
ref: 'foo/bar',
instance: 'instance-1',
};
const cloudFn = database.onValueWritten(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const afterDataVal = { snapshot: 'after' };

const beforeDataVal = { snapshot: 'before' };

const data = { before: beforeDataVal, after: afterDataVal };
const cloudEvent = cloudFnWrap({ data }).cloudEvent;

expect(cloudEvent.data.before.val()).deep.equal(beforeDataVal);
expect(cloudEvent.data.after.val()).deep.equal(afterDataVal);
});
});
});

Expand Down
71 changes: 65 additions & 6 deletions src/cloudevent/generate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { CloudEvent } from 'firebase-functions/v2';
import { CloudFunction } from 'firebase-functions/v2';
import {
CloudEvent,
CloudFunction,
database,
pubsub,
} from 'firebase-functions/v2';
import { LIST_OF_MOCK_CLOUD_EVENT_PARTIALS } from './mocks/partials';
import { DeepPartial, MockCloudEventAbstractFactory } from './types';
import { DeepPartial } from './types';
import { Change } from 'firebase-functions';
import merge from 'ts-deepmerge';

/**
Expand All @@ -17,9 +22,7 @@ export function generateCombinedCloudEvent<
cloudFunction,
cloudEventPartial
);
return cloudEventPartial
? (merge(generatedCloudEvent, cloudEventPartial) as EventType)
: generatedCloudEvent;
return mergeCloudEvents(generatedCloudEvent, cloudEventPartial);
}

export function generateMockCloudEvent<EventType extends CloudEvent<unknown>>(
Expand All @@ -37,3 +40,59 @@ export function generateMockCloudEvent<EventType extends CloudEvent<unknown>>(
// No matches were found
return null;
}

const IMMUTABLE_DATA_TYPES = [database.DataSnapshot, Change, pubsub.Message];

function mergeCloudEvents<EventType extends CloudEvent<unknown>>(
generatedCloudEvent: EventType,
cloudEventPartial: DeepPartial<EventType>
) {
/**
* There are several CloudEvent.data types that can not be overridden with json.
* In these circumstances, we generate the CloudEvent.data given the user supplies
* in the DeepPartial<CloudEvent>.
*
* Because we have already extracted the user supplied data, we don't want to overwrite
* the CloudEvent.data with an incompatible type.
*
* An example of this is a user supplying JSON for the data of the DatabaseEvents.
* The returned CloudEvent should be returning DataSnapshot that uses the supplied json,
* NOT the supplied JSON.
*/
if (shouldDeleteUserSuppliedData(generatedCloudEvent, cloudEventPartial)) {
delete cloudEventPartial.data;
}
return cloudEventPartial
? (merge(generatedCloudEvent, cloudEventPartial) as EventType)
: generatedCloudEvent;
}

function shouldDeleteUserSuppliedData<EventType extends CloudEvent<unknown>>(
Copy link
Member

Choose a reason for hiding this comment

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

This is such a cool problem you had to solve. Good find!

generatedCloudEvent: EventType,
cloudEventPartial: DeepPartial<EventType>
) {
// Don't attempt to delete the data if there is no data.
if (cloudEventPartial?.data === undefined) {
return false;
}
// If the user intentionally provides one of the IMMUTABLE DataTypes, DON'T delete it!
if (
IMMUTABLE_DATA_TYPES.some((type) => cloudEventPartial?.data instanceof type)
) {
return false;
}

/** If the generated CloudEvent.data is an IMMUTABLE DataTypes, then use the generated data and
* delete the user supplied CloudEvent.data.
*/
if (
IMMUTABLE_DATA_TYPES.some(
(type) => generatedCloudEvent?.data instanceof type
)
) {
return true;
}

// Otherwise, don't delete the data and allow ts-merge to handle merging the data.
return false;
}
48 changes: 40 additions & 8 deletions src/cloudevent/mocks/database/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,46 @@ import {
} from '../../../providers/database';
import { getBaseCloudEvent } from '../helpers';
import { Change } from 'firebase-functions';
import { makeDataSnapshot } from '../../../providers/database';

type ChangeLike = {
Copy link
Member

Choose a reason for hiding this comment

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

nit: This reads a little weird. Why did you create a type alias to an anonymous interface type rather than just creating a named interface (i.e. interface ChangeLike {)

before: database.DataSnapshot | object;
after: database.DataSnapshot | object;
};

function getOrCreateDataSnapshot(
data: database.DataSnapshot | object,
ref: string
) {
if (data instanceof database.DataSnapshot) {
return data;
}
if (data instanceof Object) {
return makeDataSnapshot(data, ref);
}
return exampleDataSnapshot(ref);
}

function getOrCreateDataSnapshotChange(
data: DeepPartial<Change<database.DataSnapshot> | ChangeLike>,
ref: string
) {
if (data instanceof Change) {
return data;
}
if (data instanceof Object && data?.before && data?.after) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want && (data?.before || data?.after) and make an undefined before/after be a DataSnapshot to a nonexistent value. WDYT?

Functionally it's the difference between one LOC in:

wrappedFn({
  ref: "users/inlined"
  data: {
    // is this necessary?
    before: null,
    after: { name: "Thomas" },
  }
});

I could see arguments in either direction. Either we're helping them write simpler code or we could require them to be explicit to avoid accidental misuse (e.g. typo in "before" or "after"). Thoughts?

const beforeDataSnapshot = getOrCreateDataSnapshot(data!.before, ref);
const afterDataSnapshot = getOrCreateDataSnapshot(data!.after, ref);
return new Change(beforeDataSnapshot, afterDataSnapshot);
}
return exampleDataSnapshotChange(ref);
}

export function getDatabaseSnapshotCloudEvent(
cloudFunction: CloudFunction<database.DatabaseEvent<database.DataSnapshot>>,
cloudEventPartial?: DeepPartial<database.DatabaseEvent<database.DataSnapshot>>
cloudEventPartial?: DeepPartial<
database.DatabaseEvent<database.DataSnapshot | object>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I personally would hoist | object to the top of the type definition because a DeepPartial<object> is meaningless. So DeepPartial<database.DatabaseEvent<database.DataSnapshot>> | object

>
) {
const {
instance,
Expand All @@ -19,9 +55,7 @@ export function getDatabaseSnapshotCloudEvent(
params,
} = getCommonDatabaseFields(cloudFunction, cloudEventPartial);

const data =
(cloudEventPartial?.data as database.DataSnapshot) ||
exampleDataSnapshot(ref);
const data = getOrCreateDataSnapshot(cloudEventPartial?.data, ref);

return {
// Spread common fields
Expand All @@ -43,7 +77,7 @@ export function getDatabaseChangeSnapshotCloudEvent(
database.DatabaseEvent<Change<database.DataSnapshot>>
>,
cloudEventPartial?: DeepPartial<
database.DatabaseEvent<Change<database.DataSnapshot>>
database.DatabaseEvent<Change<database.DataSnapshot> | ChangeLike>
>
): database.DatabaseEvent<Change<database.DataSnapshot>> {
const {
Expand All @@ -54,9 +88,7 @@ export function getDatabaseChangeSnapshotCloudEvent(
params,
} = getCommonDatabaseFields(cloudFunction, cloudEventPartial);

const data =
(cloudEventPartial?.data as Change<database.DataSnapshot>) ||
exampleDataSnapshotChange(ref);
const data = getOrCreateDataSnapshotChange(cloudEventPartial?.data, ref);

return {
// Spread common fields
Expand Down
2 changes: 1 addition & 1 deletion src/v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { DeepPartial } from './cloudevent/types';
* It will subsequently invoke the cloud function it wraps with the provided {@link CloudEvent}
*/
export type WrappedV2Function<T extends CloudEvent<unknown>> = (
cloudEventPartial?: DeepPartial<T>
cloudEventPartial?: DeepPartial<T | object>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think typing of this should be DeepPartial<T> | Object instead.

  2. Since Object is roughly similar to any, I'm pretty sad to see this function loosing quite a bit of type info. Is there way to make the typing a little more stricter? E.g. Object can only have keys that are part of CloudEvent type (I'm not sure if my suggestion makes complete sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. But Object throws a lint error.
  2. Thats a rough one. If we want to support raw JSON being passed in for the onValueCreated and onValueDeleted, I'm not sure of any good alternatives.. :\

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. TIL.

  2. Understood. I wish there were a way to make this type-safe somehow (e.g. after/before example we discussed offline) but I don't see a way.

Copy link
Member

Choose a reason for hiding this comment

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

There's a big difference between Object (JS) and object (TS). For one, I don't know if DeepPartial<T> | object would be the same (IDK if you can even use "object" outside of "extends object"). If you really want to say "Anything that looks like a map" you can use Record<string, unknown>. If you want to come up with some really clever metaprogramming, I can try too.

) => any | Promise<any>;

/**
Expand Down