Skip to content

Improvement for DataSnapshot paths (#10, #26) #32

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 9 commits into from
Closed
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@
"@types/chai": "^4.1.2",
"@types/mocha": "^5.0.0",
"chai": "^4.1.2",
"firebase-admin": "~6.1.0",
"firebase-admin": "^6.2.0",
"firebase-functions": "^2.1.0",
"mocha": "^5.0.5",
"sinon": "^4.4.9",
"tslint": "^5.8.0",
"typescript": "^2.4.1"
"typescript": "^2.9.2"
},
"peerDependencies": {
"firebase-functions": ">1.0.1",
Expand Down
130 changes: 126 additions & 4 deletions spec/main.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,16 @@ import { expect } from 'chai';
import * as functions from 'firebase-functions';
import { set } from 'lodash';

import { mockConfig, makeChange, _makeResourceName, wrap } from '../src/main';
import {
mockConfig,
makeChange,
_compiledWildcardPath,
wrap,
_isValidWildcardMatch,
_extractParamsFromPath,
} from '../src/main';
import { makeDataSnapshot } from '../src/providers/database';
import { FirebaseFunctionsTest } from '../src/lifecycle';

describe('main', () => {
describe('#wrap', () => {
Expand Down Expand Up @@ -54,7 +63,7 @@ describe('main', () => {
expect(typeof context.eventId).to.equal('string');
expect(context.resource.service).to.equal('service');
expect(/ref\/wildcard[1-9]\/nested\/anotherWildcard[1-9]/
.test(context.resource.name)).to.be.true;
.test(context.resource.name)).to.be.true;
expect(context.eventType).to.equal('event');
expect(Date.parse(context.timestamp)).to.be.greaterThan(0);
expect(context.params).to.deep.equal({});
Expand Down Expand Up @@ -106,11 +115,124 @@ describe('main', () => {
expect(context.params).to.deep.equal(params);
expect(context.resource.name).to.equal('ref/a/nested/b');
});

it('should throw when snapshot path includes wildcards', () => {
const fft = new FirebaseFunctionsTest();
fft.init();

const snap = makeDataSnapshot(
'hello world',
'/ref/{wildcard}/nested/{anotherWildcard}',
);
const params = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make makeDataSnapshot throw, instead of only catching it later when wrapped is called.

wildcard: 'at',
anotherWildcard: 'bat',
};
const wrapped = wrap(constructCF('google.firebase.database.ref.write'));
expect(() => wrapped(snap, { params })).to.throw();
});

it('should set auto-fill params based on DataSnapshot path', () => {
const fft = new FirebaseFunctionsTest();
fft.init();

const snap = makeDataSnapshot(
'hello world',
'/ref/cat/nested/that',
);

const wrapped = wrap(constructCF('google.firebase.database.ref.write'));
const result = wrapped(snap, {});

expect(result.data._path).to.equal('ref/cat/nested/that');
expect(result.data.key).to.equal('that');
expect(result.context.params.wildcard).to.equal('cat');
expect(result.context.resource.name).to.equal('ref/cat/nested/that');
});

it('should error when DataSnapshot path does not match trigger template', () => {
const fft = new FirebaseFunctionsTest();
fft.init();

const snap = makeDataSnapshot(
'hello world',
'/different/at/nested/bat',
);
const wrapped = wrap(constructCF('google.firebase.database.ref.write'));
expect(() => wrapped(snap, { params })).to.throw();
});
});

describe('#_extractParamsFromPath', () => {
it('should match a path which fits a wildcard template', () => {
const params = _extractParamsFromPath(
'companies/{company}/users/{user}',
'/companies/firebase/users/abe');
expect(params).to.deep.equal({company: 'firebase', user: 'abe'});
});

it('should not match unfilled wildcards', () => {
const params = _extractParamsFromPath(
'companies/{company}/users/{user}',
'companies/{still_wild}/users/abe');
expect(params).to.deep.equal({user: 'abe'});
});

it('should not match a path which is too long', () => {
const params = _extractParamsFromPath(
'companies/{company}/users/{user}',
'companies/firebase/users/abe/boots');
expect(params).to.deep.equal({});
});

it('should not match a path which is too short', () => {
const params = _extractParamsFromPath(
'companies/{company}/users/{user}',
'companies/firebase/users/');
expect(params).to.deep.equal({});
});

it('should not match a path which has different chunks', () => {
const params = _extractParamsFromPath(
'locations/{company}/users/{user}',
'companies/firebase/users/{user}');
Copy link
Contributor

Choose a reason for hiding this comment

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

companies/firebase/users/user (second parameter to _extractParamsFromPath should not have wildcard, under the normal use of this function)

expect(params).to.deep.equal({});
});
});

describe('#_isValidWildcardMatch', () => {
it('should match a path which fits a wildcard template', () => {
const valid = _isValidWildcardMatch(
'companies/{company}/users/{user}',
'/companies/firebase/users/abe');
expect(valid).to.equal(true);
});

it('should not match a path which is too long', () => {
const tooLong = _isValidWildcardMatch(
'companies/{company}/users/{user}',
'companies/firebase/users/abe/boots');
expect(tooLong).to.equal(false);
});

it('should not match a path which is too short', () => {
const tooShort = _isValidWildcardMatch(
'companies/{company}/users/{user}',
'companies/firebase/users/');
expect(tooShort).to.equal(false);
});

it('should not match a path which has different chunks', () => {
const differentChunk = _isValidWildcardMatch(
'locations/{company}/users/{user}',
'companies/firebase/users/{user}');
Copy link
Contributor

Choose a reason for hiding this comment

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

companies/firebase/users/user for same reason as above

expect(differentChunk).to.equal(false);
});
});

describe('#_makeResourceName', () => {
describe('#_compiledWildcardPath', () => {
it('constructs the right resource name from params', () => {
const resource = _makeResourceName('companies/{company}/users/{user}', {
const resource = _compiledWildcardPath('companies/{company}/users/{user}', {
company: 'Google',
user: 'Lauren',
});
Expand Down
104 changes: 93 additions & 11 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

import { has, merge, random, get } from 'lodash';
import { has, merge, random, get, differenceWith } from 'lodash';

import { CloudFunction, EventContext, Change } from 'firebase-functions';
import {makeDataSnapshot} from './providers/database';
const wildcardRegex = new RegExp('{[^/{}]*}', 'g');

/** Fields of the event context that can be overridden/customized. */
export type EventContextOptions = {
Expand Down Expand Up @@ -97,14 +99,47 @@ export function wrap<T>(cloudFunction: CloudFunction<T>): WrappedFunction {
} else {
_checkOptionValidity(['eventId', 'timestamp', 'params', 'auth', 'authType'], options);
let eventContextOptions = options as EventContextOptions;
const unsafeData = data as any;

if (typeof unsafeData === 'object' && unsafeData._path) {
// Remake the snapshot to standardize the path and remove extra slashes
const formattedSnapshotPath = _compiledWildcardPath(
unsafeData._path,
eventContextOptions ? eventContextOptions.params : null);
data = makeDataSnapshot(unsafeData._data, formattedSnapshotPath, unsafeData.app) as any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remaking the data snapshot isn't necessary if you are not populating the snapshot's path from 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.

That's what I thought, It is necessary for standardizing the path to remove extra /, however I'll move this logic to makeSnapshot as you're right that it's a better place for it.


// Ensure the path has no wildcards
if (unsafeData._path.match(wildcardRegex)) {
throw new Error(`Data path ("${unsafeData._path}") should not contain wildcards.`);
}

// Ensure that the same param wasn't specified by the path and the options.params bundle
const pathParams = _extractParamsFromPath(cloudFunction.__trigger.eventTrigger.resource, unsafeData._path);
const overlappingWildcards = differenceWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also not necessary if you do a check within makeSnapshot to ensure the snapshot path doesn't have wildcards.

Object.keys(eventContextOptions.params),
Object.keys(pathParams),
);

if (overlappingWildcards.length) {
throw new Error(`The same context parameter (${overlappingWildcards.join(', ')}) \
was supplied by the data path "${unsafeData._path}" and the options bundle "${JSON.stringify(options)}".`);
}

// Build final params bundle
eventContextOptions.params = {
...eventContextOptions.params,
...pathParams,
};
}

const defaultContext: EventContext = {
eventId: _makeEventId(),
resource: cloudFunction.__trigger.eventTrigger && {
service: cloudFunction.__trigger.eventTrigger.service,
name: _makeResourceName(
cloudFunction.__trigger.eventTrigger.resource,
has(eventContextOptions, 'params') && eventContextOptions.params,
),
eventId: _makeEventId(),
resource: cloudFunction.__trigger.eventTrigger && {
service: cloudFunction.__trigger.eventTrigger.service,
name: _compiledWildcardPath(
cloudFunction.__trigger.eventTrigger.resource,
has(eventContextOptions, 'params') && eventContextOptions.params,
),
},
eventType: get(cloudFunction, '__trigger.eventTrigger.eventType'),
timestamp: (new Date()).toISOString(),
Expand All @@ -116,6 +151,13 @@ export function wrap<T>(cloudFunction: CloudFunction<T>): WrappedFunction {
defaultContext.authType = 'UNAUTHENTICATED';
defaultContext.auth = null;
}

if (unsafeData._path &&
!_isValidWildcardMatch(cloudFunction.__trigger.eventTrigger.resource, unsafeData._path)) {
throw new Error(`Provided path ${_trimSlashes(unsafeData._path)} \
does not match trigger template ${_trimSlashes(cloudFunction.__trigger.eventTrigger.resource)}`);
}

context = merge({}, defaultContext, eventContextOptions);
}

Expand All @@ -129,14 +171,54 @@ export function wrap<T>(cloudFunction: CloudFunction<T>): WrappedFunction {
}

/** @internal */
export function _makeResourceName(triggerResource: string, params = {}): string {
const wildcardRegex = new RegExp('{[^/{}]*}', 'g');
export function _compiledWildcardPath(triggerResource: string, params = {}): string {
let resourceName = triggerResource.replace(wildcardRegex, (wildcard) => {
let wildcardNoBraces = wildcard.slice(1, -1); // .slice removes '{' and '}' from wildcard
let sub = get(params, wildcardNoBraces);
return sub || wildcardNoBraces + random(1, 9);
});
return resourceName;

return _trimSlashes(resourceName);
}

export function _extractParamsFromPath(wildcardPath, snapshotPath) {
if (!_isValidWildcardMatch(wildcardPath, snapshotPath)) {
return {};
}

const wildcardKeyRegex = /{(.+)}/;
const wildcardChunks = _trimSlashes(wildcardPath).split('/');
const snapshotChucks = _trimSlashes(snapshotPath).split('/');
return wildcardChunks.slice(-snapshotChucks.length).reduce((params, chunk, index) => {
let match = wildcardKeyRegex.exec(chunk);
if (match) {
const wildcardKey = match[1];
const potentialWildcardValue = snapshotChucks[index];
if (!wildcardKeyRegex.exec(potentialWildcardValue)) {
params[wildcardKey] = potentialWildcardValue;
}
}
return params;
}, {});
}

export function _isValidWildcardMatch(wildcardPath, snapshotPath) {
const wildcardChunks = _trimSlashes(wildcardPath).split('/');
const snapshotChucks = _trimSlashes(snapshotPath).split('/');

if (snapshotChucks.length > wildcardChunks.length) {
return false;
}

const mismatchedChunks = wildcardChunks.slice(-snapshotChucks.length).filter((chunk, index) => {
return !(wildcardRegex.exec(chunk) || chunk === snapshotChucks[index]);
});

return !mismatchedChunks.length;
}

export function _trimSlashes(path: string) {
return path.split('/').filter((c) => c).join('/');
}

function _makeEventId(): string {
Expand Down
2 changes: 1 addition & 1 deletion src/providers/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function makeDataSnapshot(
/** Value of data for the snapshot. */
val: string | number | boolean | any[] | object,
/** Full path of the reference (e.g. 'users/alovelace'). */
refPath: string,
refPath?: string,
/** The Firebase app that the database belongs to.
* The databaseURL supplied when initializing the app will be used for creating this snapshot.
* You do not need to supply this parameter if you supplied Firebase config values when initializing
Expand Down