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

TodoList tutorial - GET ​/todos​/{id}​/todo-list gives unexpected response #4147

Closed
2 tasks
emonddr opened this issue Nov 18, 2019 · 11 comments · Fixed by #4325
Closed
2 tasks

TodoList tutorial - GET ​/todos​/{id}​/todo-list gives unexpected response #4147

emonddr opened this issue Nov 18, 2019 · 11 comments · Fixed by #4325
Assignees
Labels
bug Examples Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package

Comments

@emonddr
Copy link
Contributor

emonddr commented Nov 18, 2019

TodoList tutorial - GET ​/todos​/{id}​/todo-list gives unexpected response
for a todo item that is not part of a todo list.

Current Behavior

In the Todo tutorial, a few todo items are already inside db.json.

[
  {
    "id": 1,
    "title": "Take over the galaxy",
    "desc": "MWAHAHAHAHAHAHAHAHAHAHAHAHAMWAHAHAHAHAHAHAHAHAHAHAHAHA"
  },
  {
    "id": 2,
    "title": "destroy alderaan",
    "desc": "Make sure there are no survivors left!"
  },
  {
    "id": 3,
    "title": "play space invaders",
    "desc": "Become the very best!"
  },
  {
    "id": 4,
    "title": "crush rebel scum",
    "desc": "Every.Last.One."
  },
]

They are not associated with any list.
No problem.

I reach the end of the TodoList tutorial
I create a TodoList #1
I add two new todo items to it.
No problem.

[
  {
    "id": 1,
    "title": "Take over the galaxy",
    "desc": "MWAHAHAHAHAHAHAHAHAHAHAHAHAMWAHAHAHAHAHAHAHAHAHAHAHAHA"
  },
  {
    "id": 2,
    "title": "destroy alderaan",
    "desc": "Make sure there are no survivors left!"
  },
  {
    "id": 3,
    "title": "play space invaders",
    "desc": "Become the very best!"
  },
  {
    "id": 4,
    "title": "crush rebel scum",
    "desc": "Every.Last.One."
  },
  {
    "id": 5,
    "title": "carrots",
    "desc": "White Carrots for soup",
    "isComplete": false,
    "todoListId": 1
  },
  {
    "id": 6,
    "title": "bananas",
    "desc": "Yellow bananas for banana bread",
    "isComplete": false,
    "todoListId": 1
  },
]

When I use
GET /todos​/{todo id}/todo-list for the two new todo items (id=5 or 6) above, I get the
proper response of

{
  "id": 1,
  "title": "grocery",
  "color": "green"
}

No problem.

But when I use
GET /todos​/{todo id}/todo-list for the any item items (id=1 through 4) above (not associated with a list), I get the unexpected response of:

{
  "id": 1,
  "title": "grocery",
  "color": "green"
}

todo with id=1 is not part of todoList id=1.

Expected Behavior

Expected a response of [ ].

Link to reproduction sandbox

Related Issues

See Reporting Issues for more tips on writing good issues

Acceptance Criteria

  • Find the underlying resolver's problem which returns orphan related item.
  • Fix the bug ^.
@emonddr emonddr added the bug label Nov 18, 2019
@dhmlau
Copy link
Member

dhmlau commented Nov 18, 2019

@emonddr, I've followed the todo list tutorial and reproduced the problem.

For the Additional Information:

  1. There is a TodoListImageController in finished tutorial that is not part of the tutorial instructions

I think we didn't cover that hasOne relation in the tutorial. I was hoping to add that into the tutorial once we have the cli support: #2980.

  1. The Todo and TodoList models don't seem to have navigational properties. See TodoListRelations as an example ( the completed tutorial has properties: todos? and image? )

To clarify your point, I think the generated code shows:

export interface TodoListRelations {
  // describe navigational properties here
}

but in the example code, it has:

export interface TodoListRelations {
  todos?: TodoWithRelations[];
  image?: TodoListImageWithRelations;
}

The generated code seems to work fine, but it would be good if we have both places consistent. Perhaps @nabdelgadir or @agnes512 can comment on that?

  1. there is a difference in whether the id field is required and generated in the Todo and TodoList models.

Does it make sense to make the id field auto-generated for both cases?

? Enter the property name: id
? Property type: number
? Is id the ID property? Yes
? Is id generated automatically? Yes

@agnes512
Copy link
Contributor

Hmm. I thought 2 was solved in #3742 (add explanations for interfaces such as TodoListRelations..)

And the main problem in 3 is id should be required but in the tutorial is false.

@dhmlau
Copy link
Member

dhmlau commented Nov 18, 2019

For 2, I think the issue here is the generated code is different from the code in the examples/todo-list package.

@emonddr
Copy link
Contributor Author

emonddr commented Nov 19, 2019

Even specifying
id is ID field
id is autogenerated
for both Todo and TodoList models, and following all the instructions in the Todo Tutorial and TodoList tutorial, there is still a bug with:

GET /todos​/{todo id}/todo-list ; where todo id =1 (not part of a todo list)

@emonddr
Copy link
Contributor Author

emonddr commented Nov 19, 2019

@bajtos , are the navigational properties for TodoListRelations supposed to be filled in by lb4 relation command, or by hand? Please advise. thanks. (see comment and Diana's comments above regarding TodoListRelations)

@bajtos bajtos added Examples Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package labels Nov 19, 2019
@bajtos
Copy link
Member

bajtos commented Nov 19, 2019

are the navigational properties for TodoListRelations supposed to be filled in by lb4 relation command, or by hand?

The navigational properties will be filled by lb4 relation once we implement #2988, that's a known feature that we are missing.

For now, users have to fill the interface entries manually.

@bajtos
Copy link
Member

bajtos commented Nov 19, 2019

Does it make sense to make the id field auto-generated for both cases?

Personally, I'd prefer to make the ID always generated by the database (property defined with generated: true and model defined with setting forceId: true unless forceId is applied automatically for auto-generated PKs, which probably is).

@emonddr
Copy link
Contributor Author

emonddr commented Nov 19, 2019

Adding the navigational properties by hand to to the Todo and TodoList models doesn't fix the bug I am seeing.

@dhmlau
Copy link
Member

dhmlau commented Nov 19, 2019

There seems to be more than one issue captured in this ticket. I'd like to propose:

  1. Keep this issue as the bug reported in the original description
  2. Open another issue to make sure the tutorials is up-to-date, e.g.
  • we need to add the navigational properties by hand.
  • make the 2 models' id to be autogen.

@emonddr , could you please create a new ticket for #2? Thanks.

@dhmlau dhmlau added 2020Q1 and removed 2020Q1 labels Nov 19, 2019
@dhmlau
Copy link
Member

dhmlau commented Nov 19, 2019

This is a stretch goal for Q4. Could you please:

  • add acceptance criteria to this issue
  • open a new issue based on the above comment?
    Thanks!

@emonddr
Copy link
Contributor Author

emonddr commented Nov 19, 2019

I have moved the conversation of the differences between the TodoList Tutorial instructions and completed tutorial contents in lb4 example todo-list to the issue: #4161

This issue is now only about the problem described in the Description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Examples Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
None yet
4 participants