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

Add pipes & hooks wildcard event #1305

Merged
merged 7 commits into from May 27, 2019

Conversation

Projects
None yet
5 participants
@thomasarbona
Copy link
Contributor

commented May 22, 2019

What does this PR do ?

  • pipes & hooks can now trigger wildcarded events like this:
    'foo:*'
    'foo:after*'
    'foo:before*'

How should this be manually tested?

  • Step 1 : create a plugin and declare some wildcarded events handler
  • Step 2 : trigger events
@codecov

This comment has been minimized.

Copy link

commented May 22, 2019

Codecov Report

Merging #1305 into 1-dev will increase coverage by <.01%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1-dev    #1305      +/-   ##
==========================================
+ Coverage   93.84%   93.85%   +<.01%     
==========================================
  Files          98       98              
  Lines        6884     6895      +11     
==========================================
+ Hits         6460     6471      +11     
  Misses        424      424
Impacted Files Coverage Δ
lib/api/kuzzle.js 83.78% <100%> (ø) ⬆️
lib/api/core/plugins/pluginsManager.js 87.76% <95%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db14d78...97b50d0. Read the comment docs.

@@ -831,8 +831,15 @@ class PluginsManager {
* @returns {Promise}
*/
function triggerHooks(emitter, event, data) {
const wildcardEvents = getWildcardEvents(event)
// filter events like 'foo:*' because EventEmitter2

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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.

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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

Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js
}

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

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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}:*`];

This comment has been minimized.

Copy link
@alexandrebouthinon

alexandrebouthinon May 23, 2019

Member

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.

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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?

This comment has been minimized.

Copy link
@alexandrebouthinon

alexandrebouthinon May 23, 2019

Member

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

if (indexDelimiter !== 1) {
return event.substring(0, indexDelimiter+1) + '*';
function getWildcardEvents (event) {
const [scope, name] = event.split(':');

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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

This comment has been minimized.

Copy link
@alexandrebouthinon

alexandrebouthinon May 23, 2019

Member

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 (preparedHooks) {
preparedHooks.forEach(hook => {
hook(data);

This comment has been minimized.

Copy link
@Aschen

Aschen May 23, 2019

Contributor

I'm not sure but what if the hook function provided by the developer throw an error ? Can't we have a unhandled rejection here ?

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

more importantly: this bypasses Kuzzle as an event emitter, which is not the purpose of hooks

@thomasarbona thomasarbona requested a review from scottinet May 23, 2019

if (preparedHooks) {
preparedHooks.forEach(hook => {
try {
hook(data);

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

this is still broken: plugins aren't the only objects using events

why don't you let the kuzzle object, which inherits from eventemitter, emit that event?

@thomasarbona thomasarbona force-pushed the KZL-291-wildcarded-event branch from 1234067 to 3f9331f May 23, 2019

@thomasarbona thomasarbona requested a review from scottinet May 23, 2019

const delimIndex = event.lastIndexOf(':');

if (delimIndex === -1) {
return null;

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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 [];
}
}];

pluginsManager.run()

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

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

}
}];

pluginsManager.run()

This comment has been minimized.

Copy link
@scottinet

scottinet May 23, 2019

Member

same here

}
}];

pluginsManager.run()

This comment has been minimized.

Copy link
@scottinet

scottinet added a commit that referenced this pull request May 24, 2019

@kuzzle

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. INFO pluginsManager.js#L726: Complete the task associated to this TODO comment. rule
@Aschen

Aschen approved these changes May 27, 2019

@Aschen Aschen merged commit 3d06476 into 1-dev May 27, 2019

4 checks passed

LGTM analysis: JavaScript No new or fixed alerts
Details
codecov/project 93.85% (+<.01%) compared to db14d78
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sonarqube SonarQube reported 1 issue, no criticals or blockers

@Aschen Aschen deleted the KZL-291-wildcarded-event branch May 27, 2019

@Aschen Aschen changed the title [KZL-291] pipes & hooks: wildcarded event Add pipes & hooks wildcarded event May 28, 2019

@Aschen Aschen changed the title Add pipes & hooks wildcarded event Add pipes & hooks wildcard event May 28, 2019

scottinet added a commit that referenced this pull request May 28, 2019

@scottinet scottinet restored the KZL-291-wildcarded-event branch May 28, 2019

@scottinet scottinet deleted the KZL-291-wildcarded-event branch May 28, 2019

xbill82 added a commit to kuzzleio/documentation that referenced this pull request Jun 12, 2019

Documentation for wildcard events (#299)
- add documentation for wildcard events (kuzzleio/kuzzle#1305)

### How should this be manually tested?

- `npm install && npm run dev`
- visit http://localhost:8080/core/1/plugins/plugins/events/wildcard-events/

### Boyscout

- [ @xbill82  ] Disabled C++ and Java tests from local execution
- fix pages sorting (page with an order set to 0 isn't classified first)

@Aschen Aschen referenced this pull request Jun 14, 2019

Merged

Release 1.8.0 #1322

Aschen added a commit that referenced this pull request Jun 14, 2019

Merge pull request #1322 from kuzzleio/1.8.0-proposal
Release 1.8.0

Bug fixes

    [ #1311 ] Fix promise leaks (scottinet)
    [ #1298 ] Fix disabled protocol initialization (Aschen)
    [ #1297 ] Fix timeouts on plugin action returing the request (benoitvidis)
    [ #1288 ] Fix an error when trying a non-partial bulk update (scottinet)
    [ #1286 ] Allows bulk inserts on aliases (benoitvidis)
    [ #1282 ] Scan keys on redis cluster (benoitvidis)
    [ #1279 ] Users must be authenticated to use auth:logout (scottinet)

New features

    [ #1315 ] Add the new Vault module to handle encrypted application secrets (Aschen)
    [ #1302 ] Add write and mWrite (Aschen)
    [ #1305 ] Add pipes & hooks wildcard event (thomasarbona)

Enhancements

    [ #1318 ] Add a maximum ttl to auth:login (benoitvidis)
    [ #1301 ] Upgrade the WebSocket libraries (scottinet)
    [ #1308 ] Events triggering refactor (scottinet)
    [ #1300 ] Collection specifications methods cloisoned to a collection (thomasarbona)
    [ #1295 ] Improve validation error messages (benoitvidis)
    [ #1292 ] Throw an error when the realtime controller is invoked by plugin developers (benoitvidis)
    [ #1257 ] Add ability to define mapping policy for new fields (Aschen)
    [ #1291 ] Fix --help on subcommands (Yoann-Abbes)
    [ #1289 ] Handle ping/pong packets (scottinet)
    [ #1273 ] Fix incomplete access logs (scottinet)

Others

    [ #1317 ] Add ps dependency to plugin-dev Docker image for pm2 (benoitvidis)
    [ #1312 ] Check that .kuzzlerc.sample is well-formed (scottinet)
    [ #1299 ] Add Kuzzle Nightly & Redis 3 and 4 test (alexandrebouthinon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.