loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.95k stars 1.07k forks source link

Spike: Operation hooks for models/repositories #1919

Closed bajtos closed 3 years ago

bajtos commented 5 years ago

In LoopBack 3.x, we have a concept of Operation Hooks allowing models to register handlers for common events: access, before/after save, before/after delete, load/persist.

We should provide similar functionality in LoopBack 4 too.

See also the discussion in #1857 which triggered this feature request.

/cc @vvdwivedi @David-Mulder @marioestradarosa @raymondfeng

Acceptance criteria

bajtos commented 5 years ago

I have created this story as a Spike, because I feel we need to do a bit of research first. For example, where to register hook handlers - at model level or at repository level? If the hooks are registered at model level, how can we ensure that all repository implementations are honoring them? Can we find a set of hooks that is universal for all possible implementations of a given repository interface, or are the hooks coupled with a particular repository implementation and/or interface? For example, I can imagine that a KeyValue model/repository needs a different set of hooks than a CRUD model/repository.

marioestradarosa commented 5 years ago

👍 to implement them at the repository level. Keep in mind that by design it is the repository the one that intermediates between both artifacts. Remember that I mentioned you about it in our conference call?. Usually the hooks will interact on CRUD operations on Before/After events, they will receive a record, they can mutate it and then call next hook. So it makes sense in the repository since these CRUD operations occur at this level.

On WANG os, we called them updateExits 😄.

David-Mulder commented 5 years ago

My understanding of the new design was that it's perfectly okay for two repositories to access the same models (in the case of more complex relational data), would this mean that a third abstraction layer would need to be added to share the hooks between those repositories? Or is it bad design ifo two different repositories refer to the same model?

marioestradarosa commented 5 years ago

would this mean that a third abstraction layer would need to be added to share the hooks between those repositories?

By design you shouldn't have more than one repository that belongs to the same datasource pointing to the same model if we use these hooks implementation, I can't figure out a real case scenario for now.

However, you can have the same model used by two different set of datasource/repository with no problem.

bajtos commented 5 years ago

@David-Mulder thank you for joining the discussion.

My understanding of the new design was that it's perfectly okay for two repositories to access the same models (in the case of more complex relational data), would this mean that a third abstraction layer would need to be added to share the hooks between those repositories? Or is it bad design ifo two different repositories refer to the same model?

Could you please be more specific about the scenario you have in mind and give us an example of a model and the different repositories used with this single model? What are the requirements you are addressing with this solution?

IMO, it's perfectly valid to have multiple repositories using the same model class.

The situation seems pretty simple to me when all repositories are CRUD based: one can create a base repository class that implements the hooks, and then let all different repositories to inherit from that base class to ensure operation hooks are shared.

When the repositories are using different data-access patterns, e.g. CRUD vs. KeyValue store, then the situation is much more difficult, because not all operation hooks can be implemented for both CRUD and KeyValue. Is this something you are looking for too?

orshlom commented 5 years ago

Hi @bajtos @marioestradarosa, I came up with a spike roadmap to accomplish in order to get things closer for implementation:

  1. Where to register hook handlers (model level or repository level, or somewhere else?).
  2. A necessity for different sets of hooks for key/value and CRUD.
  3. All implementation options/approaches for operation hooks in LB4.

What do you think? If you can provide more regarding each one can be helpful as well.

bajtos commented 5 years ago

Where to register hook handlers (model level or repository level, or somewhere else?).

I think the hooks should be registered at repository level, because a single model can be attached to repositories of different type (CRUD, KeyValue, etc.), at least in theory. Depending on the repository type, certain hooks may not be possible to support.

A necessity for different sets of hooks for key/value and CRUD.

+1

All implementation options/approaches for operation hooks in LB4.

I don't understand this point, could you please explain in more details?

/cc @raymondfeng

ewrayjohnson commented 5 years ago

@bajtos: How can LoopBack 4 be considered ready for General Availability without this vital capability?

/cc @raymondfeng

dougal83 commented 5 years ago

@bajtos: How can LoopBack 4 be considered ready for General Availability without this vital capability?

/cc @raymondfeng

https://en.wikipedia.org/wiki/Software_release_life_cycle

ghost commented 5 years ago

In my application, I would like to set a UUID in the request header. In my sequence I just add this to request.headers and then I want to access this UUID in my REST datasource so that I can pass this on as a header(or query param or body, doesn't matter) to my REST call. Important thing is to access the headers of request in my datasource. In LB3, we could just use the hooks to do this. I am assuming having operation hooks on repository should help in this scenario as well.

@bajtos @marioestradarosa As long as we don't have the operation hooks, is there a workaround for the same?

bajtos commented 5 years ago

As a temporary workaround, it's possible to leverage existing LB 3.x Operation Hooks in the custom per-model repository class.

An example:

export class MyModelRepository extends DefaultCrudRepository<
  MyModel,
  typeof MyModel.prototype.id
> {
  constructor(@inject('datasources.db') dataSource: juggler.DataSource) {
    super(MyModel, dataSource);

    (this.modelClass as any).observe('persist', async (ctx: any) => {
      delete ctx.data.computed;
    });
  }
}
rahulrkr08 commented 5 years ago

Hi @bajtos, Above observe('persist', callback) will work for after/save right ? Any workaround for before/save ?

bajtos commented 5 years ago

@rahulrkr08

Quoting from https://loopback.io/doc/en/lb3/Operation-hooks.html#persist, emphasis is mine:

  • before save – Use this hook to observe (and operate on) model instances that are about to be saved (for example, when the country code is set and the country name not, fill in the country name).

  • persist – Use this hook to observe (and operate on) data just before it is going to be persisted into a data source (for example, encrypt the values in the database).

Both "before save" and "persist" hooks are called before the data is sent to the database.

jannyHou commented 5 years ago

Discussion in the estimation meeting: maybe we can leverage interceptor to implement hooks

bajtos commented 5 years ago

Cross-posting from https://github.com/strongloop/loopback-connector-mongodb/issues/534#issuecomment-516363114:

We need DefaultCrudRepository class to expose hooks allowing mixins to change the load/save/query behavior (think of Operation Hooks in LB3, see also https://github.com/strongloop/loopback-next/issues/1919). We already have toEntity method acting as load hook. In https://github.com/strongloop/loopback-next/issues/3446, we will be introducing fromEntity method to serve as a save/persist hook, and normalizeFilter which I think can be adapted to serve as a query/access hook.

shadyanwar commented 4 years ago
    (this.modelClass as any).observe('persist', async (ctx: any) => {
      delete ctx.data.computed;
    });

@bajtos Thanks for proposing this workaround (cross-posted in https://github.com/strongloop/loopback-next/issues/2707). However, in MySQL, the SELECT query sent always includes the computed fields (saw it using DEBUG=loopback:connector:mysql npm start).

But I tried using the access hook and it worked:

(this.modelClass as any).observe('access', async (ctx: any) => {
        delete ctx.Model.definition.properties.computed;
});
kyle-apex commented 4 years ago

Is there an intension to have this completed by the December 2020 EOL for Loopback 3 or might EOL for Loopback 3 be extended? We've been delaying migration, but now we're worried that we still won't have the ability to fully migrate due to lack of features before reaching EOL.

Thanks, Kyle

achrinza commented 4 years ago

@kyle2829 There's currently a documented method to use LoopBack 3-style operation hooks:

https://loopback.io/doc/en/lb4/migration-models-operation-hooks.html

kyle-apex commented 4 years ago

Thanks @achrinza! I wasn't sure if it was safe to use judging by "temporary" wording in the documentation: "In the meantime, we are providing a temporary API for enabling operation hooks in LoopBack 4". We'll go ahead with using that method, thanks.

dzhiwei commented 4 years ago

As a temporary workaround, it's possible to leverage existing LB 3.x Operation Hooks in the custom per-model repository class.

An example:

export class MyModelRepository extends DefaultCrudRepository<
  MyModel,
  typeof MyModel.prototype.id
> {
  constructor(@inject('datasources.db') dataSource: juggler.DataSource) {
    super(MyModel, dataSource);

    (this.modelClass as any).observe('persist', async (ctx: any) => {
      delete ctx.data.computed;
    });
  }
}

@bajtos Is the MyModel only can be a LoopBack 4 model? Can this approach compatible with LoopBack 3 models in lb3app/server/models?

FottyM commented 4 years ago

As a temporary workaround, it's possible to leverage existing LB 3.x Operation Hooks in the custom per-model repository class. An example:

export class MyModelRepository extends DefaultCrudRepository<
  MyModel,
  typeof MyModel.prototype.id
> {
  constructor(@inject('datasources.db') dataSource: juggler.DataSource) {
    super(MyModel, dataSource);

    (this.modelClass as any).observe('persist', async (ctx: any) => {
      delete ctx.data.computed;
    });
  }
}

@bajtos Is the MyModel only can be a LoopBack 4 model? Can this approach compatible with LoopBack 3 models in lb3app/server/models?

https://loopback.io/doc/en/lb3/Operation-hooks.html

f-w commented 4 years ago

As a temporary workaround, it's possible to leverage existing LB 3.x Operation Hooks in the custom per-model repository class.

An example:

export class MyModelRepository extends DefaultCrudRepository<
  MyModel,
  typeof MyModel.prototype.id
> {
  constructor(@inject('datasources.db') dataSource: juggler.DataSource) {
    super(MyModel, dataSource);

    (this.modelClass as any).observe('persist', async (ctx: any) => {
      delete ctx.data.computed;
    });
  }
}

Operation hook in LB3 can access httpContext through ctx.options.httpContext. But in this workaround ctx.options is {}. @inject doesn't work as DI cannot be applied to listener function parameter. I am stuck.

Being able to access httpContext is necessary in my use case.

tipuban commented 4 years ago

Operation hook in LB3 can access httpContext through ctx.options.httpContext. But in this workaround ctx.options is {}. @inject doesn't work as DI cannot be applied to listener function parameter. I am stuck.

Being able to access httpContext is necessary in my use case.

@f-w You could try injecting a getter function for RestBindings.Http.CONTEXT in your model repository constructor method and get its value when the hook is triggered:

export class MyModelRepository extends DefaultCrudRepository<
  MyModel,
  typeof MyModel.prototype.id
> {
  constructor(
    @inject('datasources.db') dataSource: juggler.DataSource,
// -> GETTER
    @inject.getter(RestBindings.Http.CONTEXT) protected getHttpContext: Getter<Context>,
// <------
  ) {
    super(MyModel, dataSource);

    (this.modelClass as any).observe('persist', async (ctx: any) => {
// -> GETTER VALUE
      const httpCtx = await this.getHttpContext();
      console.log(httpCtx);
// <------
      delete ctx.data.computed;
    });
  }
}
puranjayjain-mtxb2b commented 4 years ago

The above unfortunately gives us a context not found error

Cannot start the application. ResolutionError: The key 'rest.http.request' is not bound to any value in context ApiServerApplication-YHAq-QSFTE_VUeFcZFYo6g-0 (context: ApiServerApplication-YHAq-QSFTE_VUeFcZFYo6g-0, binding: rest.http.request, resolutionPath: repositories.SomeRepository --> @SomeRepository.prototype.request)
    at ApiServerApplication.getValueOrPromise
puranjayjain-mtxb2b commented 4 years ago

Thanks, it worked for me, but not in the migration script, so ended up injecting an env variable to guard it.

mycolaos commented 4 years ago

I would like to use this workaround to edit the query. The problem is that the listener is created for every request.

(this.modelClass as any).observe('access', async (ctx: any) => {
        console.log('this is a leak.')
});

First request you will see one console log, second request you see two more. Also, as I modify the query, it grows with every "access", ctx.query.where = { and: [ctx.query.where, myAdditionalRestrictions] };, it seems like the ctx is shared among instances.

How to solve this?

mycolaos commented 4 years ago

Found this https://loopback.io/doc/en/lb4/migration-models-operation-hooks.html,

class ProductRepository extends DefaultCrudRepository<
  Product,
  typeof Product.prototype.id,
  ProductRelations
> {
  constructor(dataSource: juggler.DataSource) {
    super(Product, dataSource);
  }

  definePersistedModel(entityClass: typeof Product) {
    const modelClass = super.definePersistedModel(entityClass);
    modelClass.observe('before save', async ctx => {
      console.log(`going to save ${ctx.Model.modelName}`);
    });
    return modelClass;
  }
}

Seems to work. Is there any reason why the other approach was recommended?

UPDATE: not really working, it keeps the reference to the first RequestContext.

tipuban commented 4 years ago

I'm facing a similar issue: I need to get the authenticated user (@inject(SecurityBindings.USER)) from within the model observer in order to secure access to data & auto-magically assign entity ownership & other security-related attributes on entity save, however I can't get the authenticated user since the observer is running in an APPLICATION context and the user gets authenticated in a REQUEST context. I've been struggling with this for days now.

I've tried using my own JWTAuthenticationProvider in order to bind the user to the application context but that's going to cause trouble when multiple users hit the app at the same time since the APPLICATION context is a singleton.

Does anybody have any suggestions on how to get the REQUEST context from within an observer callback? Any help would be much appreciated.

tipuban commented 4 years ago

I'm actually considering creating a custom sequence where I bind model operation hooks for this request only, and then unbind them once the request is completed. Does that sound like a good idea? Or does anybody have a better suggestion?

tipuban commented 4 years ago

Nevermind. I figured out I can use Global Interceptors to bind/unbind my model observers :).

f-w commented 3 years ago

Found this https://loopback.io/doc/en/lb4/migration-models-operation-hooks.html,

class ProductRepository extends DefaultCrudRepository<
  Product,
  typeof Product.prototype.id,
  ProductRelations
> {
  constructor(dataSource: juggler.DataSource) {
    super(Product, dataSource);
  }

  definePersistedModel(entityClass: typeof Product) {
    const modelClass = super.definePersistedModel(entityClass);
    modelClass.observe('before save', async ctx => {
      console.log(`going to save ${ctx.Model.modelName}`);
    });
    return modelClass;
  }
}

Seems to work. Is there any reason why the other approach was recommended?

UPDATE: not really working, it keeps the reference to the first RequestContext.

I also confirmed the workaround keeps the reference to the request context defined in the first repository instance. For example, with code

export class ProductRepository extends DefaultCrudRepository<
  Product,
  typeof Product.prototype.id
> {
  constructor(
   ...
    @inject.getter(MiddlewareBindings.CONTEXT)
    protected getHttpContext: Getter<MiddlewareContext>,
  ) {
    ...
  }

  definePersistedModel(
    entityClass: typeof Product,
  ): typeof juggler.PersistedModel {
    const modelClass = super.definePersistedModel(entityClass);
    modelClass.observe('access', async ctx => {
      const httpCtx = await this.getHttpContext();
      console.log(httpCtx);
    });
    return modelClass;
  }
}
  1. Subsequent http requests use the httpCtx of the first request
  2. In a test, when first inserting items using the repository (which has no http context), then querying using super test client, the httpCtx of the query request is null.

Not only this is not working, but also poses security vulnerability. Please remove https://loopback.io/doc/en/lb4/migration-models-operation-hooks.html.

achrinza commented 3 years ago

Thanks for providing your detailed findings, looking into this right now.


These are my preliminary findings:

  1. Subsequent http requests use the httpCtx of the first request
  2. In a test, when first inserting items using the repository (which has no http context), then querying using super test client, the httpCtx of the query request is null.

I have not been able to replicate this. In my testing, this has always resulted in a key 'xxx' is not bound to any value error.

I have tested and gotten the aforementioned behaviour with these bindings:


To help expedite this, I've created issue #6962, quoting the relevant comments.

Could you please add additional information and a GitHub repo with a minium-reproduction example into issue #6962 so that we can pinpoint the issue?

stale[bot] commented 3 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

stale[bot] commented 3 years ago

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.