Fix/model with id required #3667

jannyHou · 2019-09-04T19:06:50Z

When id is required in a model, the expected create method in REST controller should be

  @post('/todos', {
    responses: {
      '200': {
        description: 'Todo model instance',
        content: {'application/json': {schema: getModelSchemaRef(Todo)}},
      },
    },
  })
  async createTodo(
    @requestBody({
      content: {
        'application/json': {
          // DIFFERENCE
          // instead of `getModelSchemaRef(Todo, {exclude: ['id']})
          schema: getModelSchemaRef(Todo),
        },
      },
    })
   // DIFFERENCE
   // instead of `Omit<Todo, 'id'>`
    todo: Todo,
  ): Promise<Todo> {
    return this.todoRepo.create(todo);
  }

This PR generates the right signature for create in the controller generator's template.

At first I didn't know if create method is the only one affected, so I added another model with id required in the example todo to figure out the fix's scope. It turns out other methods are fine.

I temporary keep the new TodoWithId model and its related tests in the example package so reviewers know what's the expected controller file content, and in case you have questions regarding other methods.

Will remove the changes in examples/todo when land the PR.
-->

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 👈

raymondfeng · 2019-09-04T19:54:36Z

packages/cli/generators/controller/index.js

+        type: 'confirm',
+        name: 'idRequired',
+        message: 'Is the id required when creating an instance?',
+        default: false,


Can we check the generated flag for the id property? See #3643

@raymondfeng The fix in this PR is in the controller generator, so it doesn't have the generated flag (#3643 is modifying model generator)

Can the controller code generator have access the model definition to check?

It would be great if lb4 controller could automatically infer whether the id property is required or forbidden when creating new models. At the same time, I think the current proposal is a meaningful improvement, therefore it should be landed. We can improve the generator to parse id property definition later.

IIRC, there are multiple ways how to end up with a model that requires id values on creation. The model-level setting forceId plays an important role.

@model({settings: {forceId: false}}) export class TodoWithId extends Entity { @property({ type: 'number', id: true, }) id?: number; // ... }

When forceId is disabled, clients have can either supply their own id, or let the database generate a new unique id for them. (In the schema, the id property should be shown as optional.) That's my understanding of how things should work, I did not run a test to verify the actual behavior.

@raymondfeng

Can the controller code generator have access the model definition to check?

It should have access to the model def if the file exist. Then use ts-morph to read the element.

And I agree with @bajtos that we probably can land this PR first, since in some situations, if idRequired cannot be inferred from the model class, a prompt is still needed.

Like the id name prompt, when id cannot be inferred from the artifactInfo, the prompt will ask for it.

I will submit another PR to read the idRequired from model file and add a when to the prompt. I believe there would be more discussion about how we infer the flag :)

bajtos · 2019-09-05T12:39:24Z

packages/cli/generators/controller/index.js

+        type: 'confirm',
+        name: 'idRequired',
+        message: 'Is the id required when creating an instance?',
+        default: false,


It would be great if lb4 controller could automatically infer whether the id property is required or forbidden when creating new models. At the same time, I think the current proposal is a meaningful improvement, therefore it should be landed. We can improve the generator to parse id property definition later.

IIRC, there are multiple ways how to end up with a model that requires id values on creation. The model-level setting forceId plays an important role.

@model({settings: {forceId: false}}) export class TodoWithId extends Entity { @property({ type: 'number', id: true, }) id?: number; // ... }

When forceId is disabled, clients have can either supply their own id, or let the database generate a new unique id for them. (In the schema, the id property should be shown as optional.) That's my understanding of how things should work, I did not run a test to verify the actual behavior.

bajtos · 2019-09-05T12:41:06Z

packages/cli/test/integration/generators/controller.integration.js

+          Object.assign({}, restCLIInputComplete, {idRequired: true}),
+        );
+
+      checkRestCrudContents(1);


Boolean arguments are difficult to understand. What does 1 mean in this context? I have to look up the definition of checkRestCrudContents to understand that.

Let's use named flags (an options arg) please.

Suggested change

checkRestCrudContents(1);

checkRestCrudContents({idRequired: true});

bajtos · 2019-09-05T12:41:47Z

examples/todo/src/models/todo-with-id.model.ts

+    id: true,
+    required: true,
+  })
+  id?: number;


Since id is required: true, I think we should tell TypeScript to make it require too?

Suggested change

id?: number;

id: number;

bajtos · 2019-09-06T07:09:12Z

@jannyHou one more thing: please update the rest-crud package to stay in sync with the template. See packages/rest-crud/src/crud-rest.controller.ts.

I am not sure if it's possible to apply both changes (JSON schema, TypeScript type). To me, it's important to emit the correct JSON schema, so that the REST API validation does not reject valid requests. As for the TypeScript type, we can start with something like Todo & {id?: ID} - I think that should cover all cases (id required, id forbidden).

jannyHou · 2019-09-10T20:58:39Z

packages/rest-crud/src/crud-rest.controller.ts

+  //  * Implementation of the endpoint `POST /`.
+  //  * @param data Model data
+  //  */
+  // create(data: Omit<T, IdName>): Promise<T>;


cc @bajtos Since the type of data is not determined (w/ or w/o id), I removed the create function in this interface. Thought?

I am quite unhappy with the changes made in this file, I feel they are introducing too much complexity. We have started with one option - id is required or generated - and have two controller classes. Imagine what is going happen when the number of options grows. Are we going to have 8 different classes when the number of options grows to 3?

I prefer to sacrifice compile-time safety and keep the implementation simple. I expect there will be very few people consuming these runtime-generated controllers directly from TypeScript code.

I tried my suggestions to replace Omit<T, IdName> with Todo & {id?: ID} and I see it does not work. Can we use the following type instead?

async create( data: T | Omit<T, IdName>, ): Promise<T>;

Imagine what is going happen when the number of options grows. Are we going to have 8 different classes when the number of options grows to 3?

I share your concern and I am also not satisfied with the current approach, while honestly I couldn't think of any better solution in a reasonable time 😞

The type of data is not the biggest problem, the generated request body spec when id is excluded need to differ from the one that includes id.

See https://github.com/strongloop/loopback-next/blob/master/packages/rest-crud/src/crud-rest.controller.ts#L155

bajtos · 2019-09-12T07:27:37Z

examples/todo/src/__tests__/acceptance/todo-with-id.acceptance.ts

+  HttpCachingProxy,
+} from '../helpers';
+
+describe('TodoApplication - test TodoWithId', () => {


How does TodoWithId fit into the narrative of our Todo tutorial?

In my mind, the primary purpose of Todo example app is to support the tutorial and show a simple & meaningful LB4 application. It's not the best place for adding acceptance tests that are not related to the Todo tutorial.

@bajtos Oh the tests will be removed when land the PR, see my comment in #3667 (comment).

I should have added the comment in the code.

At first I didn't know if create method is the only one affected, so I added another model with id required in the example todo to figure out the fix's scope. It turns out other methods are fine.
I temporary keep the new TodoWithId model and its related tests in the example package so reviewers know what's the expected controller file content, and in case you have questions regarding other methods.
Will remove the changes in examples/todo when land the PR.

bajtos · 2019-09-12T07:28:49Z

packages/rest-crud/src/__tests__/acceptance/default-model-crud-rest.acceptance.ts

-      expect(created)
-        .to.have.property('id')
-        .of.type('number');
+    context('id not required', () => {


Suggested change

context('id not required', () => {

context('id generated by the database', () => {

bajtos · 2019-09-12T07:29:10Z

packages/rest-crud/src/__tests__/acceptance/default-model-crud-rest.acceptance.ts

-        .post('/products')
-        .send({id: 1, name: 'a name'})
-        .expect(422);
+    context('id required', () => {


Suggested change

context('id required', () => {

context('id required (supplied by the user)', () => {

bajtos · 2019-09-12T07:37:16Z

packages/rest-crud/src/crud-rest.controller.ts

+  //  * Implementation of the endpoint `POST /`.
+  //  * @param data Model data
+  //  */
+  // create(data: Omit<T, IdName>): Promise<T>;


I am quite unhappy with the changes made in this file, I feel they are introducing too much complexity. We have started with one option - id is required or generated - and have two controller classes. Imagine what is going happen when the number of options grows. Are we going to have 8 different classes when the number of options grows to 3?

I prefer to sacrifice compile-time safety and keep the implementation simple. I expect there will be very few people consuming these runtime-generated controllers directly from TypeScript code.

I tried my suggestions to replace Omit<T, IdName> with Todo & {id?: ID} and I see it does not work. Can we use the following type instead?

async create( data: T | Omit<T, IdName>, ): Promise<T>;

bajtos · 2019-09-12T07:45:49Z

packages/rest-crud/src/crud-rest.controller.ts

@@ -254,8 +241,43 @@ export function defineCrudRestController<
    }
  }

-  // See https://github.com/microsoft/TypeScript/issues/14607
-  return CrudRestControllerImpl;
+  if (options && options.idRequired) {


At runtime, we already know whether the model requires the id to be provided by the user or whether it will be generated by the database. Let's use that information please and don't ask the user to supply it to us again.

Relevant code from LB3:

https://github.com/strongloop/loopback-swagger/blob/fe5cfd7a11fb4cb0c9804db510d461a3d2ad53a6/lib/specgen/model-helper.js#L46-L60

https://github.com/strongloop/loopback/blob/aad97c20360de236af6afcb4e364f42d071664b0/lib/model.js#L794-L799

https://github.com/strongloop/loopback-datasource-juggler/blob/de4718d5b81d29ac23597172d8ee591617924fe0/lib/model.js#L841-L844

https://github.com/strongloop/loopback-datasource-juggler/blob/de4718d5b81d29ac23597172d8ee591617924fe0/lib/model-builder.js#L343-L362

Please note that we need to support more update-only properties, for example the Cloudant connector uses property _rev that is always generated by the database.

I see this is topic is more complex than I initially thought :(

If you prefer, then I am ok to leave the changes in packages/rest-crud out of scope of this pull request, so that it can be landed sooner. Just open a follow-up issue and add it to the epic #2036 From model definition to REST API with no custom repository/controller classes please.

I see thank you for all the references. Yeah I would like to address it with @raymondfeng 's comment #3667 (comment) in another story.

The bug fix is not a committed story in September, so I would like to merge this quick fix first, then improve it :)

Sounds good. Please open two stories - one to update rest-crud, another to improve the CLI tool.

jannyHou · 2019-09-13T05:29:38Z

@bajtos Thanks for the detailed explanation, follow-up story created #3728 and also reverted the acceptance tests. PTAL

bajtos

LGTM, but please consider my comment below.

bajtos · 2019-09-13T14:51:59Z

packages/cli/generators/controller/index.js

+      {
+        type: 'confirm',
+        name: 'idRequired',
+        message: 'Is the id required when creating an instance?',


I am not sure if this question makes it clear what are the implications of yes and no answers. Every model instance must eventually have some id value set in the database, thus one can argue that the id property is always required (in a way).

I think it may be better to instead ask the user if the id property is generated by the database. When the id is generated, then it must be excluded from create requests - I thinks this implication is easier to understand.

Suggested change

message: 'Is the id required when creating an instance?',

message: 'Is the id (primary key) value generated by the database?',

If you decide to accept my suggestion, then please rename the question/answer name from idRequired to something more sensible.

@bajtos Hmm, I have a different opinion on this, I am thinking of being consistent with the LB 3 options.

See https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html

required: Implies "Whether a value for the property is required. If true, then adding or updating a model instance requires a value for the property." and this is my purpose of adding the prompt.

generated: "The generated property indicates the ID will be automatically generated by the database. If true, the connector decides what type to use for the auto-generated key. For relational databases, such as Oracle or MySQL, it defaults to number. If your application generates unique IDs, set it to false." It describes a more complicated scenario than whether a property is required or not in the request payload...

I see your point.

There is still one missing piece though: the id property is special, there are three possible variants:

The id is required when creating an instance. (It's always generated by the client, useful e.g. for offline-first.) TS type to use: T (e.g. Product)

The id is not required but can be provided when creating an instance. (The client can decide whether they want to supply their own id or let the DB create one.) TS type to use: T & {id?: <number|string>} (I am not entirely sure about the right type)

The id must not be provided and will be always generated by the database. TS type to use: Omit<T, 'id'>.

By default, LB apps are configured for the option 3 - the id property is not allowed in "create" requests.

That's why I find your prompt confusing, because to me, it selects between the first and second variant, but then emits TypeScript type for the third variant.

@bajtos I see, maybe let me change the prompt to be "Is the id omitted when creating an instance?"

Yes: use Omit<T, 'id'>

No: use T

For the 2nd scenario, I think it could be same as 1 if id is defined as optional property in the model, like:

// scenario 1 @model() class Product{ @property() id: string; } // scenario 2 @model() class Product{ @property() id?: string; }

jannyHou · 2019-09-16T14:47:17Z

@bajtos see my comment and I applied the change accordingly: #3667 (comment), thank you

bajtos

LGTM 👍

Please clean up the git history and wait for green CI before landing.

bajtos · 2019-09-16T15:22:36Z

packages/cli/generators/controller/index.js

-        message: 'Is the id required when creating an instance?',
-        default: false,
+        name: 'idOmitted',
+        message: 'Is the id omitted when creating an instance?',


Nitpick: I would use "creating a new instance" to make it even more obvious that this prompt is related to new instances only

jannyHou requested review from bajtos and raymondfeng as code owners September 4, 2019 19:06

jannyHou force-pushed the fix/model-with-id-required branch from fca2503 to e14031f Compare September 4, 2019 19:07

raymondfeng reviewed Sep 4, 2019

View reviewed changes

bajtos reviewed Sep 5, 2019

View reviewed changes

dhmlau mentioned this pull request Sep 10, 2019

Semantic Error at OASv3 for 'MessageExcluding[id]': #3698

Closed

jannyHou force-pushed the fix/model-with-id-required branch 2 times, most recently from 03e8048 to 17f9bca Compare September 10, 2019 20:50

jannyHou commented Sep 10, 2019

View reviewed changes

bajtos suggested changes Sep 12, 2019

View reviewed changes

jannyHou mentioned this pull request Sep 13, 2019

Update controller class in package rest-crud when id is a required property #3728

Closed

1 task

bajtos approved these changes Sep 13, 2019

View reviewed changes

bajtos approved these changes Sep 16, 2019

View reviewed changes

jannyHou force-pushed the fix/model-with-id-required branch 2 times, most recently from 11a65f3 to 4ffd1b3 Compare September 16, 2019 16:59

fix: model with id required

7af132c

jannyHou force-pushed the fix/model-with-id-required branch from 4ffd1b3 to 7af132c Compare September 16, 2019 17:36

jannyHou merged commit 270e13e into master Sep 16, 2019

jannyHou deleted the fix/model-with-id-required branch September 16, 2019 18:05

This was referenced Sep 16, 2019

why Loopback + mongodb is not working? #3645

Closed

For controller generator, infer id omitted from the existing model class file #3757

Closed

docs: update generators #3791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/model with id required #3667

Fix/model with id required #3667

jannyHou commented Sep 4, 2019 •

edited

Loading

raymondfeng Sep 4, 2019

jannyHou Sep 4, 2019

raymondfeng Sep 4, 2019

bajtos Sep 5, 2019

jannyHou Sep 5, 2019

bajtos Sep 5, 2019

bajtos Sep 5, 2019

bajtos Sep 5, 2019

bajtos commented Sep 6, 2019

jannyHou Sep 10, 2019

bajtos Sep 12, 2019

jannyHou Sep 12, 2019

bajtos Sep 12, 2019

jannyHou Sep 12, 2019

bajtos Sep 12, 2019

bajtos Sep 12, 2019

bajtos Sep 12, 2019

bajtos Sep 12, 2019

jannyHou Sep 12, 2019

bajtos Sep 13, 2019

jannyHou commented Sep 13, 2019

bajtos left a comment

bajtos Sep 13, 2019

jannyHou Sep 13, 2019

bajtos Sep 16, 2019

jannyHou Sep 16, 2019

jannyHou commented Sep 16, 2019

bajtos left a comment

bajtos Sep 16, 2019

	checkRestCrudContents(1);
	checkRestCrudContents({idRequired: true});

	context('id not required', () => {
	context('id generated by the database', () => {

	context('id required', () => {
	context('id required (supplied by the user)', () => {

	message: 'Is the id required when creating an instance?',
	message: 'Is the id (primary key) value generated by the database?',

Fix/model with id required #3667

Fix/model with id required #3667

Conversation

jannyHou commented Sep 4, 2019 • edited Loading

Checklist

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 commented Sep 6, 2019

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

Choose a reason for hiding this comment

jannyHou commented Sep 13, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Sep 16, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Sep 4, 2019 •

edited

Loading