Skip to content

Support Params as inputs in eventTrigger #168

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

Merged
merged 4 commits into from
Sep 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 7 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"@types/mocha": "^5.2.7",
"chai": "^4.2.0",
"firebase-admin": "^10.1.0",
"firebase-functions": "^3.22.0",
"firebase-functions": "^3.23.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the peer-depdency? It looks like our package won't work unless you are using firebase-functions 3.23 or above now since we explicitly try to import the Expression type in v2/params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're extremely right!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That probably means we need to do a minor or major bump then... WDYT?

"firebase-tools": "^8.9.2",
"mocha": "^6.2.2",
"prettier": "^1.19.1",
Expand All @@ -57,7 +57,7 @@
},
"peerDependencies": {
"firebase-admin": ">=6.0.0",
"firebase-functions": ">=3.22.0",
"firebase-functions": ">=3.23.0",
"jest": ">=28.0.0"
},
"engines": {
Expand Down
34 changes: 34 additions & 0 deletions spec/v2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
eventarc,
https,
} from 'firebase-functions/v2';
import { defineString } from 'firebase-functions/v2/params';
import { makeDataSnapshot } from '../src/providers/database';

describe('v2', () => {
Expand Down Expand Up @@ -372,6 +373,39 @@ describe('v2', () => {
expect(cloudEvent.ref).equal('foo/bar/baz');
});

it('should resolve default ref given StringParam', () => {
process.env.rtdb_ref = 'foo/StringParam/baz';
const referenceOptions = {
ref: '',
instance: 'instance-1',
};
const cloudFn = database.onValueCreated(referenceOptions, handler);
cloudFn.__endpoint.eventTrigger.eventFilterPathPatterns.ref = defineString(
'rtdb_ref'
);
const cloudFnWrap = wrapV2(cloudFn);
const cloudEvent = cloudFnWrap().cloudEvent;
expect(cloudEvent.ref).equal('foo/StringParam/baz');
});

it.skip('should resolve default ref given TernaryExpression', () => {
const ref1 = defineString('rtdb_ref_1');
process.env.rtdb_ref_1 = 'foo/StringParam/1';
const ref2 = defineString('rtdb_ref_2');
process.env.rtdb_ref_2 = 'foo/StringParam/2';
const referenceOptions = {
ref: '',
instance: 'instance-1',
};
const cloudFn = database.onValueCreated(referenceOptions, handler);
cloudFn.__endpoint.eventTrigger.eventFilterPathPatterns.ref = ref1
.equals('aa')
.then('rtdb_ref_1', 'rtdb_ref_2');
const cloudFnWrap = wrapV2(cloudFn);
const cloudEvent = cloudFnWrap().cloudEvent;
expect(cloudEvent.ref).equal('rtdb_ref_2');
});

it('should resolve using params', () => {
const referenceOptions = {
ref: 'users/{user}',
Expand Down
9 changes: 6 additions & 3 deletions src/cloudevent/mocks/database/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { CloudFunction, database } from 'firebase-functions/v2';
import { Expression } from 'firebase-functions/v2/params';
import { DeepPartial } from '../../types';
import {
exampleDataSnapshot,
exampleDataSnapshotChange,
} from '../../../providers/database';
import { getBaseCloudEvent } from '../helpers';
import { resolveStringExpression, getBaseCloudEvent } from '../helpers';
import { Change } from 'firebase-functions';
import { makeDataSnapshot } from '../../../providers/database';

Expand Down Expand Up @@ -117,18 +118,20 @@ export function getCommonDatabaseFields(
>
>
) {
const instance =
const instanceOrExpression =
(cloudEventPartial?.instance as string) ||
cloudFunction.__endpoint?.eventTrigger?.eventFilterPathPatterns?.instance ||
cloudFunction.__endpoint?.eventTrigger?.eventFilters?.instance ||
'instance-1';
const instance = resolveStringExpression(instanceOrExpression);
const firebaseDatabaseHost =
(cloudEventPartial?.firebaseDatabaseHost as string) ||
'firebaseDatabaseHost';
const rawRef =
const rawRefOrExpression =
(cloudEventPartial?.ref as string) ||
cloudFunction?.__endpoint?.eventTrigger?.eventFilterPathPatterns?.ref ||
'/foo/bar';
const rawRef = resolveStringExpression(rawRefOrExpression);
const location = (cloudEventPartial?.location as string) || 'us-central1';
const params: Record<string, string> = cloudEventPartial?.params || {};
const ref = extractRef(rawRef, params);
Expand Down
12 changes: 11 additions & 1 deletion src/cloudevent/mocks/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { CloudEvent, CloudFunction } from 'firebase-functions/v2';
import { Expression } from 'firebase-functions/v2/params';
Copy link
Contributor

@taeold taeold Sep 6, 2022

Choose a reason for hiding this comment

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

Aside - we are planning to change the import path of Expression from v2/params to just /params.

I wonder if there are more "elegant' way to import the type to avoid having this problem 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When is the change going to occur? 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

v4 sdk!


export const APP_ID = '__APP_ID__';
export const PROJECT_ID = '42';
Expand All @@ -10,7 +11,7 @@ export function getEventType(cloudFunction: CloudFunction<any>): string {

export function getEventFilters(
cloudFunction: CloudFunction<any>
): Record<string, string> {
): Record<string, string | Expression<string>> {
return cloudFunction?.__endpoint?.eventTrigger?.eventFilters || {};
}

Expand All @@ -27,6 +28,15 @@ export function getBaseCloudEvent<EventType extends CloudEvent<unknown>>(
} as EventType;
}

export function resolveStringExpression(
stringOrExpression: string | Expression<string>
) {
if (typeof stringOrExpression === 'string') {
return stringOrExpression;
}
return stringOrExpression?.value();
Copy link
Contributor

@taeold taeold Sep 6, 2022

Choose a reason for hiding this comment

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

Probably double check with @Berlioz - but I thought it is very possible that this to return empty string.

I'm wondering if we should think more about supporting Params firebase-functions.

Params are a way to specify "value of this is going to be determined at deploy time" - so it is probably going to be undefined at test time unless there is some special override mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I was working against was eventTrigger.eventFilters and eventTrigger.eventFilterPathPatterns were converted to type Record<string, string | Expression<string>> :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests show a scenario where its populated at runtime too

}

function makeEventId(): string {
return (
Math.random()
Expand Down
4 changes: 3 additions & 1 deletion src/cloudevent/mocks/storage/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { CloudFunction, CloudEvent } from 'firebase-functions/v2';
import { StorageEvent } from 'firebase-functions/v2/storage';
import {
FILENAME,
resolveStringExpression,
getBaseCloudEvent,
getEventFilters,
getEventType,
Expand All @@ -14,10 +15,11 @@ export const storageV1: MockCloudEventAbstractFactory<StorageEvent> = {
cloudFunction: CloudFunction<StorageEvent>,
cloudEventPartial?: DeepPartial<StorageEvent>
): StorageEvent {
const bucket =
const bucketOrExpression =
cloudEventPartial?.bucket ||
getEventFilters(cloudFunction)?.bucket ||
'bucket_name';
const bucket = resolveStringExpression(bucketOrExpression);
const source =
cloudEventPartial?.source ||
`//storage.googleapis.com/projects/_/buckets/${bucket}`;
Expand Down