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

Koa style middleware: Hooks for any asynchronous method #932

Open
daffl opened this issue Aug 16, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@daffl
Copy link
Member

commented Aug 16, 2018

Hooks are a powerful way to control the flow of asynchronous methods. Koa popularized a new kind of middleware that allows an even more flexible approach to control this flow. I propose to adapt this pattern as a new async hook type.

Problem

Currently, Feathers hooks are split into three types, before, after and error:

feathers-hooks-old

While this pattern works quite well, it is a little cumbersome to implement functionality that needs to have access to the before, after and/or error flow at the same time. For example functionality for a profiler that logs the method runtime or extracting and storing data (e.g. associations) that will be used after the method call returns requires a before and an after hook.

Proposal

With async/await now being fully supported by all recent versions of Node - and the ability to fall back to Promises for Node 6 - we now have a powerful new way to create middleware that can control the whole flow within a single function.

This pattern has been implemented already in KoaJS for processing HTTP requests. However, similar to what I wrote before about Express style middleware, this also doesn't have to be limited to the HTTP request/response cycle but can apply to any asynchronous function, including Feathers services. Basically this new kind of hook will wrap around the following hooks and the service method call like this:

feathers-hooks-2 1

How it works

Additionally to the hook context async hooks also get an asynchronous next function that controls passing the flow to the next hook in the chain.

The following example registers an application wide before, after and error logger and profiler for all service method calls:

app.hooks({
  async: [
    async (context, next) => {
      try {
        const start = new Date().getTime();

        console.log(`Before calling ${context.method} on ${context.path}`);

        await next();

        console.log(`Successfully called ${context.method} on ${context.path}`, context.result);
        console.log('Calling ${context.method} on ${context.path} took ${new Date().getTime() - start}ms');
        return context;
      } catch(error) {
        console.error(`Error in ${context.method} on ${context.path}`, error.message);
        // Not re-throwing would swallow the error
        throw error;
      }
    }
  ]
});

The following example extracts the address association for a user create method and saves it with the userId reference:

app.service('users').hooks({
  async: {
    create: [
      async (context, next) => {
        const { app, data } = context;
        const { address } = data;

        context.data = _.omit(data, 'address');

        // Get the result from the context returned by the next hook
        // usually the same as accessing `context.result` after the `await next()` call
        // but more flexible and functional
        const { result } = await next();

        // Get the created user id
        const userId = context.result.id;

        // Create address with user id reference
        await app.service('addresses').create(Object.assign({
          userId
        }, address));

        return context;
      }
    ]
  }
});

async hook type

The core of async hooks would be implemented as an individual module that allows to add hooks to any asynchronous method. This has been inspired by the great work @bertho-zero has been doing in #924 to allow using hooks on other service methods. I created a first prototype in https://github.com/daffl/async-hooks (test case can be found here). It uses Koa's middleware composition module koa-compose.

async hooks would be implemented as an additional hook type but all hook dispatching will be done using the new middleware composition library. Existing before after and error hooks will be wrapped and registered in a backwards compatible way:

app.service('users').hooks({
  async: {
    all: [],
    create: [],
    find: []
    // etc.
  },

  // Continue using all your existing hooks here
  before: {
    all: [],
    find: []
    // etc.
  },

  after: {},

  error: {}
});

async hooks will have precedence so the execution order would be async -> before -> after. Even when only using the old hooks there are several advantages:

  • Better stack traces
  • Slightly better performance
  • Less custom code to maintain and the potential to collaborate with the Koa project

Migration path

There will be two breaking changes:

  1. The currently still supported (but undocumented and not recommended) function(context, next) signature for existing before, after and error hooks will no longer work.
  2. koa-compose does not allow to replace the entire context. Returning the context is still possible but other hooks have to retrieve it from the await next(); call with const ctx = await next();.

@daffl daffl added this to the Crow milestone Aug 16, 2018

@eddyystop

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

I would hope this proposal can solve the 2 biggest issues I face with the common hooks using the Buzzard design.

Use case 1: More complex hooks often need to make a call on their own or another service. In my experience its often a get call and the hook needs the record as it appears in the table, e.g. fastJoin, softDelete, softDelete2, stashBefore.

Problems: (1) The get call will run the before and after hooks for the service, thus possibly modifying the record contents.
(2) The hooks on the called service could themselves perform service calls, and the problem escalates.
(3) I considered using a param.$raw flag, but its not elegant and breaks down quickly.
(4) The interaction between the more complex common hooks, authentication hooks and custom hooks results in a fair number of posted issues. People ask if various hooks are compatible together, e.g. cache + softDelete2 + restrictToOwner, cache + authentication. I reply "Try it and see."

Solution: (1) Service calls made on the server need to optionally skip all (some?) hooks.
(2) Allow before hooks to skip the remaining before and AFTER hooks.

Use case 2: When the cache hook, for example, finds a cache hit, it needs to set context.result and skip the following before hooks.

Problem: (1) Its not possible to both set the content and SKIP in the same hook. Two hooks would be needed and that's not elegant.
(2) Presently, cache sets context.result. Since neither common nor custom before hooks check context.result, the remaining before hooks run doing irrelevant processing on context.data, .query and perhaps making some service calls. This causes interaction issues.

@daffl daffl removed this from the Crow milestone Aug 18, 2018

@daffl

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

This should definitely work for a caching hook. If it is one of the first registered, it can set context.result and not call next() which will skip all other hooks:

app.hooks({
  async: [
    async (context, next) => {
      const { app, method, params } = context;

      if(method === 'find' || method === 'get') {
        const key = JSON.stringify(context.params.query);
        const cached = app.cache[key];

        if(cached) {
          context.result = cached;

          // Just return the context, don't call `next`
          return context;
        }

        const { result } = await next();

        app.cache[key] = result;
      }
      
      return context;
    }
  ]
});

For skipping hooks we could add an additional parameter (similar to the returnHook flag) to method calls that indicates that it shouldn't run any of the hooks:

// Return the hook object
app.service('users').get(id, hooks.RETURN);

// Skip all hooks
app.service('users').get(id, hooks.SKIP);

To skip specific ones, I'm not sure if there is a way around wrapping them in a conditional hook or having a hook check it themselves.

@eddyystop

This comment has been minimized.

Copy link
Member

commented Aug 18, 2018

I would suggest the cache contain the records returned by the DB call itself. That way the hooks intended after the DB call can customize the response for the specifics of that call, e.g. the role of the user.

Perhaps the "after" hooks can do that.

@eddyystop

This comment has been minimized.

Copy link
Member

commented Aug 18, 2018

Its been a while since I looked at Koa, but doesn't its middleware bubble downstream and then bubble upstream?

Perhaps you can consider running the async hooks before the 'before" hooks, and before the "after" hooks, and maybe (?) even before the "error" hooks. That would produce a similar downstream/upstream feature.

I assume type would still be before and after so the async function could know what it should do. It would be practical to have another property indicating if the hook was running as an async hook or not, so hooks can prevent themselves from being run in the wrong context.

Perhaps type should just be before, after, async_before and async_after.

@daffl

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

Maybe it's not clear in the proposal but the idea is that Koa style middleware has access to the whole flow that can be controlled by calling next() so there is no explicit before or after:

// Before hook
const beforeHook = async (context, next) => {
  console.log('Before');

  await next();
}

// After hook
const afterHook = async (context, next) => {
  await next();

  console.log('After');
}

// Error hook
const errorHook = async (context, next) => {
  try {
    await next();
  } catch(error) {
    console.error('Error', error.message);
    throw error;
  }
}

// Before, after and error
const afterHook = async (context, next) => {
  try {
    console.log('Before');

    await next();

    console.log('After');
  } catch(error) {
    console.error('Error', error.message);
    throw error;
  }
}
@beeplin

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Totally agree with the proposal. One suggestion: instead of using service.hooks({async: [function]}), just simply use service.hooks([function]). The new koa-style hook is meant to replace the old before/after/error hooks, and in the future there will be only one kind of hooks, so no need to wrap them in the async property.

@beeplin

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

BTW it would be nice if in the next major version we gave service.hooks an alias like service.registerHooks. .hooks is the only public service API that uses a noun as a method name.

@eddyystop

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

I've been too busy to contribute on ths issue and will continue to be till the 12th.

However, as I mentioned above, its important a (async?) hook be able to get/find a record from its own or from another service, without any of the hooks on that service being run. So I'm assuming something like your hooks.SKIP or hooks.RETURN will be available for both get and find.

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.