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

How to migrate CRUD Operation Hooks #3952

Closed
3 tasks done
bajtos opened this issue Oct 17, 2019 · 6 comments
Closed
3 tasks done

How to migrate CRUD Operation Hooks #3952

bajtos opened this issue Oct 17, 2019 · 6 comments

Comments

@bajtos
Copy link
Member

bajtos commented Oct 17, 2019

This is a follow-up for #3718 and #3922.

Write content for docs/site/migration/models/operation-hooks.md, explain how to migrate operation hooks (see LB3 docs: Operation Hooks).

Explain that we don't have first-class support for Operation Hooks in LB4 yet, refer to #1919

As a temporary solution, describe how to leverage PersistedModel used by our legacy juggler bridge. Example implementation:

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

    this.modelClass.observe('save', async ctx => {
      console.log(
        'Going to write to an instance of model %s',
        ctx.Model.modelName,
      );
    });
  }
}

Caveats:

(1)
Repository constructor is executed once for each incoming request. We don't want to install another copy of the same operation hook on each request, we need to find a way how to install hooks only once. Ideally, DefaultCrudRepository should provide a protected method that subclasses can use to supply one-time model setup.

For example:

export class TodoRepository extends DefaultCrudRepository<
  Todo,
  typeof Todo.prototype.id,
  TodoRelations
> {
  constructor(@inject('datasources.db') dataSource: juggler.DataSource) {
    super(Todo, dataSource);
  }
  
  protected setupPersistedModel() {
    super.setupPersistedModel();

    this.modelClass.observe('save', async ctx => {
      console.log(
        'Going to write to an instance of model %s',
        ctx.Model.modelName,
      );
    });
  }
}

(2)
Current TypeScript typings in loopback-datasource-juggler do not contain operation hooks and .observe() method. We need to fix this.

Acceptance criteria

  • Add typings for Operation Hooks to loopback-datasource-juggler
  • Add a protected method definePersistedModel to DefaultCrudRepository, this method should overriden by user's repository class to access the model class where the operation hooks can be applied.
  • Migration guide describing how to move LB3 operation hooks to LB4 repositories.
@marioestradarosa
Copy link
Contributor

marioestradarosa commented Nov 10, 2019

@bajtos

How about decorators inside the repository to allow a particular method to become part of the event (before/after etc)

@observe('before-save')
async myMethodInsideRepository(rec: MyModel) : Promise<MyModel> {

}

@hacksparrow
Copy link
Contributor

@bajtos how about this alternate interface?

// repo is a Repository instance
repo.observe('before save', async ctx => {
  console.log(
    'Going to write to an instance of model %s',
    ctx.Model.modelName,
  );
});

The hooks are not hard-coded in the repository class, you add them as required using an instance method.

@bajtos
Copy link
Member Author

bajtos commented Feb 21, 2020

@hacksparrow can you please clarify how is your interface dealing with the following problem?

Repository constructor is executed once for each incoming request. We don't want to install another copy of the same operation hook on each request, we need to find a way how to install hooks only once

Also IMO, setup of operation hooks is a responsibility of per-model repository class (e.g. ProductRepository), it's not a responsibility of repository consumers (controllers, etc.).

@hacksparrow
Copy link
Contributor

@bajtos I am yet to start working on a solution for preventing registration of an operation hook multiple times, but it would be the same for whatever interface we choose ultimately.

I am wondering, since the repository instance is destroyed after a request is responded and a new one created for each request. Where is the possibility of installing the same hook multiple times? Unless it is done intentionally by the developer in the repository? Even that should be taken care of by Juggler, if it is not doing it already.

... operation hooks is a responsibility of per-model repository ...

I was thinking, the code in the model's repo would be:

this.observe('before save', async ctx => {
  console.log(
    'Going to write to an instance of model %s',
    ctx.Model.modelName,
  );
});

observe being implemented in DefaultCrudRepository.

@bajtos
Copy link
Member Author

bajtos commented Feb 21, 2020

I am wondering, since the repository instance is destroyed after a request is responded and a new one created for each request. Where is the possibility of installing the same hook multiple times? Unless it is done intentionally by the developer in the repository? Even that should be taken care of by Juggler, if it is not doing it already.

DefaultCrudRepository needs a backing PersistedModel attached to the datasource in order to perform CRUD operations. The datasource instance is long-living (almost like a singleton) and so are the PersistedModel classes attached to it.

At the moment, DefaultCrudRepository checks if the backing PersistedModel has been created before (by another instance created previously) and does not recreate the model in such case. See here:

https://github.com/strongloop/loopback-next/blob/267b074a93dc7483333486e2b381b3d7168ebc79/packages/repository/src/repositories/legacy-juggler-bridge.ts#L133-L149

Now we need to find such design that will make it easy for people writing their custom Repository classes inheriting from DefaultCrudRepository to register operation hooks on the backing PersistedModel only once.

The following naive solution will register a new hook every time a new instance of MyRepo is created, and that's what we need to avoid.

class MyRepo extends DefaultCrudRepository</*...*/> {
  constructor(/*...*/) {
    super(/*...*/);
    this.modelClass.observe('before save', async ctx => {/*...*/});
  }
}

... operation hooks is a responsibility of per-model repository ...

I was thinking, the code in the model's repo would be:

this.observe('before save', async ctx => {
 console.log(
   'Going to write to an instance of model %s',
   ctx.Model.modelName,
 );
});

observe being implemented in DefaultCrudRepository.

I like that proposal 👍 as long as observe is API specific to DefaultCrudRepository, not an API contract expected from all Repository implementations.

Please keep in mind that in this user story, we want to find a solution that will allow LB3 developers to migrate their operation hooks to LB4 with minimal extra implementation effort required on our side. The solution based on PersistedModel's observe API is just a temporary workaround until we implement proper hooks as part of #1919. The trick is to find the right balance between doing too little (ending up with a poor UX) and spending too much time (on superb UX).

@hacksparrow
Copy link
Contributor

Closed via #4730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants