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

Extension: CrudRestController #3582

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Extension: CrudRestController #3582

merged 1 commit into from
Aug 26, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos 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 a team August 20, 2019 14:17
@bajtos bajtos self-assigned this Aug 20, 2019
@bajtos bajtos added REST Issues related to @loopback/rest package and REST transport in general feature feature parity and removed feature parity labels Aug 20, 2019
CODEOWNERS Show resolved Hide resolved
@bajtos bajtos mentioned this pull request Aug 20, 2019
4 tasks
@bajtos bajtos force-pushed the feat/rest-crud branch 3 times, most recently from a664e45 to bec7199 Compare August 22, 2019 12:25
@bajtos
Copy link
Member Author

bajtos 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 August 22, 2019 12:38
@bajtos bajtos requested a review from a team August 22, 2019 12:38
@bajtos bajtos changed the title [WIP] Extension: CrudRestController Extension: CrudRestController Aug 22, 2019
IdType,
IdName extends keyof T,
Relations extends object = {}
>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* The base path where to "mount" the controller.
*/
basePath: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM, a few comments about the status code.

Introduce a new package `@loopback/rest-crud` providing a controller
class implementing default CRUD operations for a given Entity.
@bajtos
Copy link
Member Author

bajtos 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
@bajtos bajtos deleted the feat/rest-crud branch August 26, 2019 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants