feat(rest-crud): add CrudRestApiBuilder #4589

nabdelgadir · 2020-02-07T18:10:19Z

CrudRestApiBuilder is a model API builder that builds the default repository and controller class for a given Entity class.

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 👈

agnes512

Great job! I have a few questions and suggestions.

packages/rest-crud/README.md

packages/rest-crud/src/crud-rest-builder.plugin.ts

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts

packages/rest-crud/src/crud-rest-builder.plugin.ts

jannyHou

@nabdelgadir Good stuff 👍 I post a few comments, mostly LGTM.

jannyHou · 2020-02-11T16:20:30Z

packages/rest-crud/src/crud-rest-builder.plugin.ts

+  inject(`datasources.${modelConfig.dataSource}`)(
+    repositoryClass,
+    undefined,
+    0,


@nabdelgadir might be a silly question: I am looking at the inject function, how do we know the index is 0 and why the member name is undefined?

From my understanding, the only reason we're using a number (that happens to be 0 in this case) is so that it goes into this if statement, but I don't think the number actually matters. And we're only passing in member since it's required and so we're able to pass in the number as the third argument, so leaving it as undefined is the easiest way to go about it (but I don't actually know the role of member in the injection, maybe @bajtos could comment on this and why we're passing in 0?).

@bajtos ^ (I have the same question)

When you set the member to undefined, it means the constructor function. (The constructor does not have any name.

The index 0 is identifying the first argument of the target function (the class constructor in this case).

In this particular invocation of inject, we are decorating the first argument of the repository constructor to inject the datasource instance. It's an equivalent of the following TypeScript code:

class MyRepository /*extends ...*/ { constructor( @inject(`datasources.${modelConfig.dataSource}`) public dataSource: juggler.DataSource ) {} }

Because decorators are converted to JavaScript calls of TypeScript API at compile time, we cannot use decorator syntax when building the repository class dynamically at runtime, we have to invoke the decorator function manually.

Can we create a constant and use that instead of 0? Perhaps also one for the undefined value? So both constants can provide some clarity to the method call?

inject(`datasources.${modelConfig.dataSource}`)( repositoryClass, UNDEFINED_CLASS_CONSTRUCTOR_FUNCTION, INDEX_OF_INJECTED_DATASOURCE_PARAMETER_INSIDE_REPOSITORY_CONSTRUCTOR,

I know the names are silly, but you get the point.

Because decorators are converted to JavaScript calls of TypeScript API at compile time, we cannot use decorator syntax when building the repository class dynamically at runtime, we have to invoke the decorator function manually.

Thanks everyone for the reply. This is something I didn't know before 👍

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts

emonddr

great start, nora

emonddr · 2020-02-12T16:30:55Z

packages/rest-crud/src/crud-rest-builder.plugin.ts

+
+    if (!(modelClass.prototype instanceof Entity)) {
+      throw new Error(
+        `CrudRestController requires an Entity, Models are not supported. (Model name: ${modelName})`,


CrudRestController requires an Entity, Models are not supported. (Model name: ${modelName}),

What are we trying to say here? That the model needs to extend Entity?

Perhaps

CrudRestController requires a model that extends 'Entity'. (Model name: ${modelName} does not extend 'Entity')

Why are we calling the file 'plugin'?

packages/rest-crud/src/crud-rest-builder.plugin.ts

Why are we calling the file 'plugin'?

packages/rest-crud/src/crud-rest-builder.plugin.ts

Following the terminology in the spike

@nabdelgadir ,

Hmm. Let's not use the word plugin. We don't mention it in the overall LB4 docs.

As an example, AuthenticationStrategyProvider is a class that defines an extension point where developers can create extensions (custom authentication strategies) and register them against the extension point... but we don't call any of the files .plugin.ts .

Same for the extensions for greeter extension point : https://github.com/strongloop/loopback-next/blob/master/examples/greeter-extension/src/greeters/greeter-en.ts#L6 , we don't use .plugin.ts .

https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/providers/auth-strategy.provider.ts#L28

Let's not use the word plugin. We don't mention it in the overall LB4 docs.

Considering that we are using ExtensionPoint/Extension design pattern, maybe we can use "extension" instead of "plugin"?

On the other hand, since this file contains CrudRestApiBuilder class which is bound asModelApiBuilder, maybe crud-rest.model-api-builder.ts would be a good name, using model-api-builder as the type suffix (similar to controller and repository we use elsewhere). If model-api-builder is too long, the perhaps api-builder is good enough too? (I.e. crud-rest.api-builder.ts).

I like crud-rest.api-builder.ts because it's shorter (and the builder is CrudRestApiBuilder instead of CrudRestModelApiBuilder, so the Model part is implied).

packages/rest-crud/src/crud-rest-builder.plugin.ts

emonddr · 2020-02-13T18:33:51Z

packages/rest-crud/README.md

+};
+```
+
+Now your `Product` model will have a default repository and default controller


So there are 2 ways that a developer can create the default controller and default repository for Product?

Performing Basic steps and not performing Advanced steps

Performing Advanced steps and not performing Basic steps

So they are mutually exclusive then? If so let's specify that Basic and Advanced are mutually exclusive. Another question: why would someone want to do Advanced steps if it is shorter to perform Basic steps?

So they are mutually exclusive then? If so let's specify that Basic and Advanced are mutually exclusive.

Yeah, I'll specify that, thanks!

Another question: why would someone want to do Advanced steps if it is shorter to perform Basic steps?

The advanced one helps if you only want to create the default controller or default repository but not both.

bajtos

Great work, @nabdelgadir ❤️

Let's make the implementation easier to understand for all maintainers, including our future selves - see my comments bellow.

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts

packages/rest-crud/README.md

packages/rest-crud/src/crud-rest-builder.plugin.ts

bajtos · 2020-02-18T14:11:17Z

packages/rest-crud/README.md

-`@loopback/rest-crud` exposes two helper methods (`defineCrudRestController` and
-`defineCrudRepositoryClass`) for creating controllers and respositories using
-code.
+`@loopback/rest-crud` can be used along with `@loopback/model-api-builder` to


I think this is not entirely correct. @loopback/model-api-builder provides only Types and helpers for packages contributing Model API builders (quoting from README).

Did you perhaps mean the built-in ModelApiBooter?

Suggested change

`@loopback/rest-crud` can be used along with `@loopback/model-api-builder` to

`@loopback/rest-crud` can be used along with the built-in `ModelApiBooter` to

In this case, CrudRestApiBuilder implements ModelApiBuilder (not the booter). Quoting from the README: it also "provides a contract for extensions that contribute builders for repositories and controllers".

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts

bajtos

The code looks great now!

It would be great to add a test to verify the scenario when the booter detects an existing repository class and does not define a new one, see #4589 (comment).

I have few minor comments, see below.

Please allow some time for other reviewers to have a chance to comment on the new version before landing.

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts

packages/rest-crud/src/crud-rest.api-builder.ts

`CrudRestApiBuilder` is a model API builder that builds the default repository and controller class for a given Entity class. Co-authored-by: Miroslav Bajtoš <[email protected]>

nabdelgadir self-assigned this Feb 7, 2020

nabdelgadir added the feature label Feb 7, 2020

nabdelgadir marked this pull request as ready for review February 7, 2020 20:01

agnes512 reviewed Feb 10, 2020

View reviewed changes

raymondfeng reviewed Feb 10, 2020

View reviewed changes

packages/boot/src/__tests__/acceptance/crud-rest-builder.acceptance.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Feb 10, 2020

View reviewed changes

packages/rest-crud/src/crud-rest-builder.plugin.ts Outdated Show resolved Hide resolved

jannyHou reviewed Feb 11, 2020

View reviewed changes

emonddr suggested changes Feb 12, 2020

View reviewed changes

emonddr reviewed Feb 12, 2020

View reviewed changes

packages/rest-crud/src/crud-rest-builder.plugin.ts Outdated Show resolved Hide resolved

nabdelgadir force-pushed the crud-rest branch from bcc3000 to 0b75cca Compare February 13, 2020 08:08

emonddr reviewed Feb 13, 2020

View reviewed changes

bajtos suggested changes Feb 14, 2020

View reviewed changes

nabdelgadir force-pushed the crud-rest branch from 0b75cca to 5aef80d Compare February 17, 2020 06:35

nabdelgadir requested review from emonddr and bajtos February 18, 2020 03:30

bajtos reviewed Feb 18, 2020

View reviewed changes

bajtos approved these changes Feb 18, 2020

View reviewed changes

nabdelgadir force-pushed the crud-rest branch 2 times, most recently from 7c822d3 to bd0e910 Compare February 19, 2020 16:57

feat(rest-crud): add CrudRestApiBuilder

5286f45

`CrudRestApiBuilder` is a model API builder that builds the default repository and controller class for a given Entity class. Co-authored-by: Miroslav Bajtoš <[email protected]>

nabdelgadir force-pushed the crud-rest branch from bd0e910 to 5286f45 Compare February 20, 2020 02:38

jannyHou approved these changes Feb 20, 2020

View reviewed changes

nabdelgadir merged commit bc5d56f into master Feb 20, 2020

nabdelgadir deleted the crud-rest branch February 20, 2020 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rest-crud): add CrudRestApiBuilder #4589

feat(rest-crud): add CrudRestApiBuilder #4589

nabdelgadir commented Feb 7, 2020

agnes512 left a comment

jannyHou left a comment

jannyHou Feb 11, 2020

nabdelgadir Feb 13, 2020 •

edited

Loading

emonddr Feb 13, 2020

bajtos Feb 13, 2020

emonddr Feb 13, 2020

jannyHou Feb 20, 2020

emonddr left a comment

emonddr Feb 12, 2020

emonddr Feb 12, 2020

nabdelgadir Feb 13, 2020

emonddr Feb 13, 2020

bajtos Feb 13, 2020

bajtos Feb 14, 2020

nabdelgadir Feb 17, 2020

emonddr Feb 13, 2020

nabdelgadir Feb 17, 2020

bajtos left a comment

bajtos Feb 18, 2020

nabdelgadir Feb 19, 2020

bajtos left a comment

	`@loopback/rest-crud` can be used along with `@loopback/model-api-builder` to
	`@loopback/rest-crud` can be used along with the built-in `ModelApiBooter` to

feat(rest-crud): add CrudRestApiBuilder #4589

feat(rest-crud): add CrudRestApiBuilder #4589

Conversation

nabdelgadir commented Feb 7, 2020

Checklist

agnes512 left a comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

nabdelgadir Feb 13, 2020 •

edited

Loading