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.

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

Jump to bottom

Add pipes & hooks wildcard event #1305

Merged
merged 7 commits into from
May 27, 2019
Merged

Add pipes & hooks wildcard event #1305

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
59f13ad
hooks & pipes: use wildcard events
thomasarbona May 22, 2019
bf7c5bd
replace regex with vanilla js
thomasarbona May 22, 2019
3f9331f
replace EventEmitter2 with EventEmitter3
thomasarbona May 23, 2019
9cb5393
Merge branch '1-dev' into KZL-291-wildcarded-event
scottinet May 23, 2019
488a774
suggestions & fix tests
thomasarbona May 24, 2019
0ab6275
move the declaration of getWildcardEvents
thomasarbona May 24, 2019
97b50d0
add 'error' to templated wildcards
thomasarbona May 27, 2019
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
50 changes: 36 additions & 14 deletions lib/api/core/plugins/pluginsManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,8 +831,15 @@ class PluginsManager {
* @returns {Promise}
*/
function triggerHooks(emitter, event, data) {
const wildcardEvents = getWildcardEvents(event)
// filter events like 'foo:*' because EventEmitter2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be simpler (and better) to completely get rid of EventEmitter2, since we're only using it for wildcards (this should be a light change since EventEmitter2 has the same interface than the native EventEmitter).

And instead manage wildcards ourselves entirely, because that kind of exception handling is a bit costly performances wide, and events triggering is a critical part of Kuzzle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this code is unsafe because getWildcardEvents can return null.

// already have this wildcard feature
.filter(e => e[0] !== '*' && e.split(':')[1] !== '*');

emitter.emit(event, data);

wildcardEvents.forEach(wildcardEvent => emitter.emit(wildcardEvent, data));

return Bluebird.resolve(data);
}

Expand All @@ -845,15 +852,19 @@ function triggerHooks(emitter, event, data) {
* @returns {Promise}
*/
function triggerPipes(pipes, event, data) {
let preparedPipes = [];
const wildcardEvent = getWildcardEvent(event);
const preparedPipes = [];
const wildcardEvents = getWildcardEvents(event);

if (pipes && pipes[event] && pipes[event].length) {
preparedPipes = pipes[event];
preparedPipes.push(...pipes[event]);
scottinet marked this conversation as resolved.
Show resolved Hide resolved
}

if (wildcardEvent && pipes && pipes[wildcardEvent] && pipes[wildcardEvent].length) {
preparedPipes = preparedPipes.concat(pipes[wildcardEvent]);
if (wildcardEvents && pipes) {
wildcardEvents.forEach(wildcardEvent => {
if (pipes[wildcardEvent] && pipes[wildcardEvent].length) {
preparedPipes.push(...pipes[wildcardEvent]);
}
});
}

if (preparedPipes.length === 0) {
Expand All @@ -876,19 +887,30 @@ function triggerPipes(pipes, event, data) {
}

/**
* For a specific event, return the corresponding wildcard
* For a specific event, return the correspondings wildcards
* @example
* getWildcardEvent('data:create') // return 'data:*'
* @param {string} event
* @returns {String} wildcard event
* getWildcardEvents('data:create') // return ['data:*']
* getWildcardEvents('data:beforeCreate') // return ['data:*', 'data:before*']
* @param {String} event
* @returns {Array<String>} wildcard events
*/
function getWildcardEvent (event) {
const indexDelimiter = event.indexOf(':');
if (indexDelimiter !== 1) {
return event.substring(0, indexDelimiter+1) + '*';
function getWildcardEvents (event) {
const [scope, name] = event.split(':');
Copy link
Contributor

Choose a reason for hiding this comment

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

This leaves events with 3 or more separator out of the picture.

Copy link
Member

@alexandrebouthinon alexandrebouthinon May 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ugly but I think it can help you to figure out how to handle more than 3 separators:

Suggested change
const [scope, name] = event.split(':');
const rawEvents = event.split(':');
const [name] = rawEvents.pop();
const [scope] = rawEvents;


if (!name) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

to prevent breaking code using the return value of this function without first checking that it didn't return null:

Suggested change
return null;
return [];

}

return null;
const wildcardEvents = ['*:*', `${scope}:*`];
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with the event *:* because:

  • we can have an arbitrary number of separators (for instance we have a core:auth:strategyAdded event)
  • I think that * would be much simpler
Suggested change
const wildcardEvents = ['*:*', `${scope}:*`];
const wildcardEvents = ['*', `${scope}:*`];

Copy link
Member

Choose a reason for hiding this comment

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

As we discuss with @thomasarbona, it could be nice to handle wildcard pattern directly as regex. So we do not need to define rules for patterns and use regex syntax.

Copy link
Contributor

@scottinet scottinet May 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Veto: regexp are slow, and the number and frequency of events triggered make them a critical part of the code.

Why do we need regular expressions?

Copy link
Member

Choose a reason for hiding this comment

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

You're right I didn't get that this action appends at funnel level. Ignore my previous comment 😅


['before', 'after'].forEach(prefix => {
if (name.startsWith(prefix)) {
wildcardEvents.push(`${scope}:${prefix}*`);
wildcardEvents.push(`*:${prefix}*`);
}
});

return wildcardEvents;
}

/**
Expand Down
117 changes: 116 additions & 1 deletion test/api/core/pluginsManager/trigger.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
const
mockrequire = require('mock-require'),
should = require('should'),
sinon = require('sinon'),
ElasticsearchClientMock = require('../../../mocks/services/elasticsearchClient.mock'),
KuzzleMock = require('../../../mocks/kuzzle.mock'),
{
errors: {
PluginImplementationError
}
},
Request: KuzzleRequest
} = require('kuzzle-common-objects');

describe('Test plugins manager trigger', () => {
Expand Down Expand Up @@ -44,6 +46,119 @@ describe('Test plugins manager trigger', () => {
});
});

it('should trigger hooks with before wildcard event', done => {
pluginsManager.plugins = [{
object: {
init: () => {},
hooks: {
'foo:before*': 'myFunc'
},
myFunc: done
},
config: {},
activated: true,
manifest: {
name: 'foo'
}
}];

pluginsManager.run()
.then(() => {
pluginsManager.trigger('foo:beforeBar');
});
});

it('should trigger hooks with after wildcard event', done => {
pluginsManager.plugins = [{
object: {
init: () => {},
hooks: {
'foo:after*': 'myFunc'
},
myFunc: done
},
config: {},
activated: true,
manifest: {
name: 'foo'
}
}];

pluginsManager.run()
.then(() => {
pluginsManager.trigger('foo:afterBar');
});
});

it('should trigger pipes with wildcard event', () => {
pluginsManager.plugins = [{
object: {
init: () => {},
pipes: {
'foo:*': 'myFunc'
},
myFunc: sinon.stub().callsArgWith(1, null, new KuzzleRequest({}))
},
config: {},
activated: true,
manifest: {
name: 'foo'
}
}];

pluginsManager.run()
Copy link
Contributor

Choose a reason for hiding this comment

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

that promise must be returned, otherwise the test isn't awaited and it always passes

.then(() => pluginsManager.trigger('foo:bar'))
.then(() => {
should(pluginsManager.plugins[0].object.myFunc).be.calledOnce();
});
});

it('should trigger pipes with before wildcard event', () => {
pluginsManager.plugins = [{
object: {
init: () => {},
pipes: {
'foo:before*': 'myFunc'
},
myFunc: sinon.stub().callsArgWith(1, null, new KuzzleRequest({}))
},
config: {},
activated: true,
manifest: {
name: 'foo'
}
}];

pluginsManager.run()
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

.then(() => pluginsManager.trigger('foo:beforeBar'))
.then(() => {
should(pluginsManager.plugins[0].object.myFunc).be.calledOnce();
});
});

it('should trigger pipes with after wildcard event', () => {
pluginsManager.plugins = [{
object: {
init: () => {},
pipes: {
'foo:after*': 'myFunc'
},
myFunc: sinon.stub().callsArgWith(1, null, new KuzzleRequest({}))
},
config: {},
activated: true,
manifest: {
name: 'foo'
}
}];

pluginsManager.run()
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

.then(() => pluginsManager.trigger('foo:afterBar'))
.then(() => {
should(pluginsManager.plugins[0].object.myFunc).be.calledOnce();
});
});

it('should reject a pipe returned value if it is not a Request instance', () => {
const pluginMock = {
object: {
Expand Down