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

Extension: CrudRestController #3582

Merged
merged 1 commit into from Aug 26, 2019

Conversation

@bajtos
Copy link
Member

commented Aug 20, 2019

This pull request adds CrudRestController extension as described in #2736. See the top-level EPIC describing the big picture & plan: #2036 From model definition to REST API with no custom repository/controller classes.

Example use of the new controller:

  1. Create a base REST CRUD controller class for your model.

    const CrudRestController = defineCrudRestController<
      Product,
      typeof Product.prototype.id,
      'id'
    >(Product, {basePath: '/products'});
  2. Create a new subclass of the base controller class to configure repository
    injection.

    class ProductController extends CrudRestController {
      constructor(@repository(ProductRepository) repo: ProductRepository) {
        super(repo);
      }
    }
  3. Register the controller with your application.

    app.controller(ProductController);

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 bajtos added this to the Aug 2019 milestone milestone Aug 20, 2019

@bajtos bajtos requested review from raymondfeng, hacksparrow and strongloop/loopback-maintainers Aug 20, 2019

@bajtos bajtos self-assigned this Aug 20, 2019

CODEOWNERS Show resolved Hide resolved
@bajtos bajtos referenced this pull request Aug 20, 2019
4 of 4 tasks complete

@bajtos bajtos force-pushed the feat/rest-crud branch 3 times, most recently from a664e45 to bec7199 Aug 22, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Added docs and implemented the missing operations.

The pull request is ready for review. /cc @raymondfeng @strongloop/loopback-maintainers

@bajtos bajtos marked this pull request as ready for review Aug 22, 2019

@bajtos bajtos requested a review from strongloop/loopback-maintainers Aug 22, 2019

@bajtos bajtos changed the title [WIP] Extension: CrudRestController Extension: CrudRestController Aug 22, 2019

IdType,
IdName extends keyof T,
Relations extends object = {}
>(

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 22, 2019

Member

Is it feasible to define a CrudRestController from a Repository (instance or class) directly? Ideally, if a developer adds a repository using lb4 repository to connect a model to a data source, we should allows him/her to expose the repository as CRUD REST APIs out of box.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 23, 2019

Author Member

That's an interesting idea, we can explore it later if you like. This new module is in 0.x version, so even if we need to make breaking changes to support your idea, then that's ok.

Our current plan is to allow developers to go from lb4 model to REST API with no custom repository class needed - see #2036

The idea is that the user will tell us that let's say Product model should be exposed via CRUD REST API (as opposed to e.g. KeyValue REST API) using datasource db, and the framework will take care of the rest.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 23, 2019

Member

The current implementation requires a repository class to be defined. I added a few comments inline. I'm open to incremental improvements.

@bajtos bajtos referenced this pull request Aug 23, 2019
2 of 2 tasks complete
/**
* The base path where to "mount" the controller.
*/
basePath: string;

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 23, 2019

Contributor

Is it possible to make this optional and use a default basePath if one isn't provided?

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 23, 2019

Author Member

That would require us to implement some logic to convert a model name (e.g. Product) to a path (e.g. /products). This gets quickly tricky - do we want /people or /persons for Person model?

I'd prefer to keep basePath required for this initial implementation, we can make it optional later.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 23, 2019

Author Member

Perhaps the purpose of this field is not clear enough and we should improve the api docs? Can you suggest a better wording? Would an example help?

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 23, 2019

Contributor

That would require us to implement some logic to convert a model name (e.g. Product) to a path (e.g. /products). This gets quickly tricky - do we want /people or /persons for Person model?

I was thinking we could use something similar to how the controller generator does it which in this scenario would make it /people.

I'd prefer to keep basePath required for this initial implementation, we can make it optional later.

That's fine with me 馃憤

Perhaps the purpose of this field is not clear enough and we should improve the api docs? Can you suggest a better wording? Would an example help?

It's clear to me. My suggestion was so that we could make options optional and be able to exclude it from the function call:

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

vs.

   const CrudRestController = defineCrudRestController<
     Product,
     typeof Product.prototype.id,
     'id'
   >(Product);

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Author Member

I was thinking we could use something similar to how the controller generator does it which in this scenario would make it /people.

Makes sense! I had mixed experience with auto-generated base path for CRUD endpoints in LB3. Some people found the pluralization rules confusing ("people" vs. "persons"), some wanted different mechanism for converting pascal-cased model names (e.g. "ProductReview") to HTTP paths (e.g. "/ProductReviews", "/product-reviews", etc.).

Let's keep basePath optional for now and wait until we learn more about this area while working on #2036.


1. Define your model class, e.g. using `lb4 model` tool.

2. Create a Repository class, e.g. using `lb4 repository` tool.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 23, 2019

Member

Is it required? IIUC, you want to skip Repository class so that a model can be used to expose REST CRUD APIs with a datasource.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Author Member

A custom repository class is not required, this is just to make the documentation easier to follow.

This new component is meant to be a low-level building block to support #2036, I am expecting that most people most people will use #2036 and don't leverage the current API directly.

For the initial docs, I choose a scenario that's easy to understand (because it's using existing concepts) and that's matching the scenario used for testing.

Depending on the outcome of the next tasks (especially the spike #2738), we may add more "meat" to this component and update or even rework the README along the way.


```ts
class ProductController extends CrudRestController {
constructor(@repository(ProductRepository) repo: ProductRepository) {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Aug 23, 2019

Member

If we skip the repository, can we inject a datasource instead?

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Author Member

Yes, I believe injecting a datasource should be a viable option too. The actual implementation does not prescribe any IoC setup, the base controller class is expecting to receive a repository instance. It's up to the code extending the controller class to decide how to obtain the repository.

I believe the following solution should work as an alternative approach too:

class ProductController extends CrudRestController {
  constructor(
    @inject('datasources.db') dataSource: juggler.DataSource,
  ) {
    const repo = new DefaultCrudRepository<
      Product,
      typeof Product.prototype.id
    >(Product, db);

    super(repo);
  }
}

@raymondfeng Would you like me to open a new issue to add a test & documentation for this approach?

@raymondfeng
Copy link
Member

left a comment

I left a few more comments, which can be addressed in follow-up tasks.

@jannyHou
Copy link
Contributor

left a comment

馃憤 LGTM, a few comments about the status code.

feat(rest-crud): initial implementation
Introduce a new package `@loopback/rest-crud` providing a controller
class implementing default CRUD operations for a given Entity.

@bajtos bajtos force-pushed the feat/rest-crud branch from dbdd4da to 959e4ee Aug 26, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@raymondfeng @nabdelgadir @jannyHou thank you for the review and thoughtful comments. I am going to land this pull request as it is now. Please let me know if you would like me to improve any parts of this initial implementation as part of the story #2736.

@bajtos bajtos merged commit 4374160 into master Aug 26, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on feat/rest-crud at 91.51%
Details

@bajtos bajtos deleted the feat/rest-crud branch Aug 26, 2019

@bajtos bajtos referenced this pull request Aug 26, 2019
0 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.