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

Spike: Operation hooks for models/repositories #1919

Closed
1 task
bajtos opened this issue Oct 26, 2018 · 34 comments
Closed
1 task

Spike: Operation hooks for models/repositories #1919

bajtos opened this issue Oct 26, 2018 · 34 comments
Labels
feature parity feature Repository Issues related to @loopback/repository package spike stale

Comments

@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

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

  • A draft pull request that:
    • finds out the minimum infrastructure and what other use cases would use this hook
    • see if use cases can be handled by life cycle observers(for data events) or interceptors
    • high level proposal for the hooks in the repository level
    • finds out that which kind of hooks are more suitable for LB4: a set of hooks that is universal for all possible implementations of a given repository interface, or the hooks coupled with a particular repository implementation. ( e.g different sets of hooks for key/value and CRUD)
@bajtos bajtos added spike feature Repository Issues related to @loopback/repository package labels Oct 26, 2018
@bajtos
Copy link
Member Author

bajtos commented Oct 26, 2018

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
Copy link
Contributor

marioestradarosa commented Oct 26, 2018

👍 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

bajtos commented Nov 6, 2018

@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
Copy link

orshlom commented Jan 22, 2019

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
Copy link
Member Author

bajtos commented Jan 25, 2019

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

This comment has been minimized.

@dougal83

This comment has been minimized.

@ghost
Copy link

ghost commented Feb 18, 2019

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
Copy link
Member Author

bajtos commented Apr 11, 2019

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
Copy link
Contributor

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

@bajtos
Copy link
Member Author

bajtos commented Jun 14, 2019

@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
Copy link
Contributor

jannyHou commented Jul 4, 2019

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

@bajtos
Copy link
Member Author

bajtos commented Jul 30, 2019

Cross-posting from loopbackio/loopback-connector-mongodb#534 (comment):

We need DefaultCrudRepository class to expose hooks allowing mixins to change the load/save/query behavior (think of Operation Hooks in LB3, see also #1919). We already have toEntity method acting as load hook. In #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
Copy link

shadyanwar commented Dec 29, 2019

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

@bajtos Thanks for proposing this workaround (cross-posted in #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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link

dzhiwei commented Aug 14, 2020

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
Copy link

FottyM commented Aug 14, 2020

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
Copy link
Contributor

f-w commented Sep 8, 2020

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
Copy link

tipuban commented Sep 13, 2020

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
Copy link

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
Copy link

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

@mycolaos
Copy link

mycolaos commented Oct 15, 2020

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
Copy link

mycolaos commented Oct 15, 2020

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
Copy link

tipuban commented Oct 16, 2020

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
Copy link

tipuban commented Oct 16, 2020

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
Copy link

tipuban commented Oct 16, 2020

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

@f-w
Copy link
Contributor

f-w commented Dec 29, 2020

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
Copy link
Member

achrinza commented Dec 29, 2020

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:

  • RestBIndings.Http.REQUEST
  • MiddlewareBindings.CONTEXT

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
Copy link

stale bot commented Jul 14, 2021

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 stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

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.

@stale stale bot closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature parity feature Repository Issues related to @loopback/repository package spike stale
Projects
None yet
Development

No branches or pull requests