Skip to content
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

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

Closed
wants to merge 9 commits into from
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
135 changes: 131 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,129 @@ 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 unspecified wildcards', () => {
abeisgoat marked this conversation as resolved.
Show resolved Hide resolved
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 params = {};
const wrapped = wrap(constructCF('google.firebase.database.ref.write'));
const result = wrapped(snap, { params });
abeisgoat marked this conversation as resolved.
Show resolved Hide resolved

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 wildcard path does not match resource path', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this test is no longer relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test checks the snapshot's provided path against the route in the function definition, I think that's still relevant because a dev could still path a snapshot which has a completely wrong path for a trigger, 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.

Ah, I see now, it is a poorly written test, but the intent is still useful, I think. I've changed it to..

    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();
    });
  });

Does that seem more right?

const fft = new FirebaseFunctionsTest();
fft.init();

const snap = makeDataSnapshot(
'hello world',
'/different/{wildcard}/nested/{anotherWildcard}',
);
const params = {
wildcard: 'at',
anotherWildcard: '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 which is too long', () => {
abeisgoat marked this conversation as resolved.
Show resolved Hide resolved
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