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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: generate controller based on Model name #3842

Merged
merged 1 commit into from Oct 3, 2019

Conversation

@hacksparrow
Copy link
Member

hacksparrow commented Sep 30, 2019

defineCrudRestController generates controller based on the Model's name.

Addresses #3732.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 30, 2019

LGTM so far.

Please update API docs and the example code in rest-crud README per acceptance criteria in the linked issue.

IMO, this is a feature, not a bug fix, because the current code works exactly as intended. Let's change the commit message type to feat please.

@hacksparrow hacksparrow marked this pull request as ready for review Sep 30, 2019
@hacksparrow hacksparrow requested review from bajtos and raymondfeng as code owners Sep 30, 2019
Copy link
Contributor

emonddr left a comment

Initially found this code confusing

const defineNamedController = new Function(
    'CrudRestController',
    `return class ${controllerName} extends CrudRestController {}`,
  );
  const controller = defineNamedController(CrudRestControllerImpl);

but @hacksparrow explained it to me.

It would be easier to understand if it looked like this:

const defineNamedController = new Function(
    'controllerClassName',
    `return class ${controllerName} extends controllerClassName {}`,
  );
  const controller = defineNamedController(CrudRestControllerImpl);

since CrudRestController is an interface name that CrudRestControllerImpl actually
implements, but in the function, 'CrudRestController' is actually a parameter name for the function.

Copy link
Contributor

emonddr left a comment

Perhaps a different name for the parameter of the defineNamedController function?
Please see my comment. thx.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Oct 1, 2019

I can see how CrudRestController can be confusing. Should we use CrudRestControllerImpl instead? I feel that controllerClassName is misleading because the argument is set to a class constructor, not the class name. controllerClass is fine with me though.

@hacksparrow hacksparrow force-pushed the fix/defineCrudRestController branch 2 times, most recently from 590cef7 to 2e85763 Oct 1, 2019
super(repo);
}
}
inject('repositories.ProductRepository')(ProductController, undefined, 0);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 1, 2019

Member

Why not using the subclassing for dependency injection? It seems to be easier to understand. We can illustrate both approaches by tests.

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Oct 1, 2019

Author Member

It seems to be easier to understand.

Agreed. However, there some redundancy going on now.

Earlier defineCrudRestController used to return a CrudRestControllerImpl class which was assigned to CrudRestController, so it made sense to do class ProductController extends CrudRestController.

Now defineCrudRestController returns a named controller class like ProductController. Doing class MyProductController extends ProductController, looks weird. "Why can't I just use ProductController?"

Alternative would something like:

const ProductControllerBase = defineCrudRestController<
     Product,
     typeof Product.prototype.id,
     'id'
>(Product, {basePath: '/products'});

class ProductController extends ProductControllerBase {}

@strongloop/loopback-next thoughts?

This comment has been minimized.

Copy link
@bajtos

bajtos Oct 1, 2019

Member

The problem with subclassing is naming - the class created by defineCrudRestController already has the right name (ProductController). How should the new class be called? Should we use an anonymous class?

const controller = const ProductController = defineCrudRestController<
  Product,
  typeof Product.prototype.id,
  'id'
>(Product, {basePath: '/products'});

const ProductController = class extends controller {
  constructor(
    // args decorated with @inject
  ) {
    super(/*forward args*/);
  }
}

I am also concerned about micro-performance - by creating a new class, we are adding more code to be executed for every incoming request.

Anyhow, these instructions will soon be moved to "Advanced use", most LB users are going to use ModelApiBooter that will be introduced by one of the next tasks in this Epic.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 1, 2019

Member

If we always expect to inject a repository, we can either make the repository class an argument of defineCrudRestController() or create a helper function such as:

setupRepositoryInjection(ProductController, 'repositories.ProductRepository');

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Oct 1, 2019

Author Member

I am in favor of making the repository class an argument of defineCrudRestController makes sense to me. @bajtos thoughts?

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Oct 2, 2019

Author Member

@raymondfeng I experimented the "repository class argument" approach:

  1. Passing a binding key would have been best, but since we have no access to the context, it won't work. We could make the context accessible, but would it be worth the changes?
  2. Passing a repository instance requires the repository to be setup with datasource. It brings in the datasource factor, meaning more work and complexity.

We don't always expect to inject a repository, though. Eg: class ProductController extends ProductControllerBase {}.

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Oct 2, 2019

Author Member

I am neutral about setupRepositoryInjection, will go with @bajtos's opinion.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 2, 2019

Member

@hacksparrow Two points:

  1. No context is needed. We only require the binding key or class for the repository so that we can have:
export function defineCrudRestController<
  T extends Entity,
  IdType,
  IdName extends keyof T,
  R extends EntityCrudRepository<T, IdType, Relations>,
  Relations extends object = {}
>(
  modelCtor: typeof Entity & {prototype: T & {[key in IdName]: IdType}},
  repoCtor: R,
  options: CrudRestControllerOptions,
): CrudRestControllerCtor<T, IdType, IdName, Relations> {
  // ...
  @api({basePath: options.basePath, paths: {}})
  class CrudRestControllerImpl
    implements CrudRestController<T, IdType, IdName> {
    constructor(
      @repository(repoCtor) public readonly repository: R
    ) {}
  1. Without the repository injection setup, the controller class is not functional.

This comment has been minimized.

Copy link
@bajtos

bajtos Oct 3, 2019

Member

I am strongly against setting up DI in defineCrudRestController. It's a code smell when DI-related constructs are littered across the code base. The class created by defineCrudRestController should be usable without DI too, it is the responsibility of the caller to decide how they want to supply the needed repository instance.

In some cases, the user may not want to inject the repository via DI, they may want to receive the datasource instance via DI instead and create a Repository instance on the fly, for example by calling new DefaultCrudRepository(Product, ds).

IMO, this discussion has uncovered a major design limitation of our IoC container, where it's not possible to wire up dependencies from outside. In a well-designed app, it's the app that defines how are dependencies resolved, not the individual classes!

Ideally, I would like to wire my IoC setup along the following lines (a low-level example, we would want a nicer API in a real implementation):

const ProductController = defineCrudRestController(...);
const binding = app.controller(ProductController);
binding.injectConstructorArg(0, 'repositories.ProductRepository');

This comment has been minimized.

Copy link
@bajtos

bajtos Oct 3, 2019

Member

I am proposing to land the current version and open a new issue or a new pull request to discuss how to improve the example code snippet.

As I mentioned above, this code snippet will be used by a very small fraction of our users, let's invest our time into improvements with bigger impact.

@hacksparrow hacksparrow force-pushed the fix/defineCrudRestController branch from 2e85763 to 079cc9b Oct 1, 2019
@bajtos
bajtos approved these changes Oct 1, 2019
Copy link
Member

bajtos left a comment

LGTM, please resolve comments from other reviewers before landing.

defineCrudRestController generates controller based on the Model's name.
@hacksparrow hacksparrow force-pushed the fix/defineCrudRestController branch from 079cc9b to 823b0bb Oct 2, 2019
@hacksparrow hacksparrow changed the title fix: generate controller based on Model name feat: generate controller based on Model name Oct 2, 2019
@emonddr
emonddr approved these changes Oct 2, 2019
@hacksparrow hacksparrow merged commit 04a3318 into master Oct 3, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.003%) to 91.796%
Details
@hacksparrow hacksparrow deleted the fix/defineCrudRestController branch Oct 3, 2019
@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Oct 3, 2019

Follow up issue for doc improvement - #3862

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Oct 4, 2019

I am strongly against setting up DI in defineCrudRestController. It's a code smell when DI-related constructs are littered across the code base. The class created by defineCrudRestController should be usable without DI too, it is the responsibility of the caller to decide how they want to supply the needed repository instance.

Setting up DI does not mandate DI to use the controller class. It's just the metadata to enable DI. The defineCrudRestController already leverages decorators in the base class for REST mapping. My expectation is that once the controller class is created via defineCrudRestController, it should be ready to be mounted using app.controller(...). That's the whole purpose to generate REST CRUD APIs by convention from a model definition and corresponding datasource.

In some cases, the user may not want to inject the repository via DI, they may want to receive the datasource instance via DI instead and create a Repository instance on the fly, for example by calling new DefaultCrudRepository(Product, ds).

It's perfectly fine to explicitly call the controller constructor with a repository arg, for example:

class MyController {
   constructor(private @repository(MyRepository) repo: MyRepository) {}
}

const myRepo = new MyRepository(...);
const myController = new MyController(myRepo);

Please note @inject only applies metadata to the class and it does not perform the injection!

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Oct 4, 2019

IMO, this discussion has uncovered a major design limitation of our IoC container, where it's not possible to wire up dependencies from outside. In a well-designed app, it's the app that defines how are dependencies resolved, not the individual classes!

Not really. The @inject is used to declare dependencies to be injected, NOT the how the dependencies should be resolved. We use binding keys as the connection for most cases, but other criteria (such as a binding filter for the interface) can be leveraged to further decouple. Only the individual classes has the knowledge about required dependencies for the business logic. It's the responsibility for the application to fulfill such dependencies. Our IoC framework magically glues the provider and consumer together leveraging the DI metadata.

const binding = app.controller(ProductController);
binding.injectConstructorArg(0, 'repositories.ProductRepository');

This is wrong. Please note ProductController can be bound to many bindings. Changing the @inject from a binding will impact other other bindings that use the same class.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Oct 15, 2019

const binding = app.controller(ProductController);
binding.injectConstructorArg(0, 'repositories.ProductRepository');

This is wrong. Please note ProductController can be bound to many bindings. Changing the @inject from a binding will impact other other bindings that use the same class.

Please note that my example code is modifying the injected constructor argument only for the single binding returned by app.controller. Other bindings using ProductController won't be affected.

Here is another example to illustrate what I mean:

// setup logger used during request handling
app.bind('request.logger').to(ConsoleLogger).injectConstructorArg(0, {
  // print info, warnings and errors 
  level: LogLevel.INFO,
});

// setup logger used during app bootstrap
app.bind('app.logger').to(ConsoleLogger).injectConstructorArg(0, {
  // we don't care about INFO level, want to see only problems
  level: LogLevel.WARN,
});
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Oct 15, 2019

It seems that you are looking for a way to make the injection binding-aware. We have explored the possibility to allow bindings to have metadata for configuration/customization but we now settle on a different path:

  1. We introduce ctx.configure() and @config to resolve configuration for a binding with a configuration binding.

  2. A custom resolve can be leveraged for @inject to provide the injected value based on information from the binding chain.

Again, we need to separate setting up injection point (`@Inject.*) from the injection where classes are instantiated and injection points are fulfilled with resolved values.
2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.