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

[Model Relation] hasAndBelongsToMany #2308

Open
dhmlau opened this issue Jan 30, 2019 · 13 comments
Open

[Model Relation] hasAndBelongsToMany #2308

dhmlau opened this issue Jan 30, 2019 · 13 comments
Assignees
Labels
feature parity feature Relations Model relations (has many, etc.)

Comments

@dhmlau
Copy link
Member

dhmlau commented Jan 30, 2019

Description / Steps to reproduce / Feature proposal

Originated from #2043

This issue is created specific for hasAndBelongsToMany because the original issue contains other types of model relation.

A few comments from #2043 that are related to hasAndBelongsToMany.

cc @RaphaelDrai @TomerSalton @codejamninja - Since you've expressed interests to implement this model relation type, I'm assigning it to you. Please let me know if i'm wrong and i can remove you from the assignees list. Also, if there's anything I can help facilitating, please let me know.

@clayrisser
Copy link
Contributor

I briefly reviewed the document. It seems to be a great start. Thanks @RaphaelDrai

@clayrisser
Copy link
Contributor

clayrisser commented Jan 31, 2019

@RaphaelDrai I'm assuming you want this code in the has-and-belongs-to-many.decorator.ts file, not the has-and-belongs-to-many.repository.ts
screen shot 2019-01-31 at 12 54 38 am

@clayrisser
Copy link
Contributor

I've started work on this. You can see my progress at #2315.

@RaphaelDrai
Copy link

@codejamninja Hello, yes it is a mistake in my document. The file name in bullet 8 is indeed "has-and-belongs-to-many.decorator.ts" and not "has-and-belongs-to-many.repository.ts"

@clayrisser
Copy link
Contributor

clayrisser commented Feb 7, 2019

One challenge I ran into that I would like some feedback on is where and when to create the junction table schemas. The other relations only use schemas a user defines in models. In this particular relation, we will need schemas created outside of the user defined models.

@clayrisser
Copy link
Contributor

I just realized I don't have to worry about creating the junction table with the hasManyThrough relation because the junction table is defined by the user in a model. I'm going to start implementing hasManyThrough before I finish this implementation. Thanks for the suggestion @bajtos.

@TomerSalton
Copy link

TomerSalton commented Mar 4, 2019

Hey,

I would like to discuss the definition of the default API that will be provided by this relation.

Assuming STUDENTS hasAndBelongsToMany CLASS, my opinion is to provide the following API:

GET ALL THE CLASSES OF A SPECIFIC STUDENT:

HTTP Method GET
URL http://root/students/{student_id}/classes/
Returns Array of Classes

CREATE A CLASS AND CONNECT IT TO THE STUDENT

HTTP Method POST
URL http://root/students/{student_id}/classes/ + [class data in body]
Returns The new class that created

FINDS A CLASS AND CONNECT IT TO THE STUDENT

HTTP Method POST
URL http://root/students/{student_id}/classes/{class_id}
Returns The already existing class, that is now connected to the student

REMOVES A CLASS FROM THE STUDENT

HTTP Method DELETE
URL http://root/students/{student_id}/classes/{class_id}
Returns The id of the class that is removed

REMOVES ALL CLASSES FROM THE STUDENT

HTTP Method DELETE
URL http://root/students/{student_id}/classes/
Returns Count of classes that removed

UPDATE A CLASS OF THE STUDENT

HTTP Method PUT
URL http://root/students/{student_id }/classes/{class_id}/ + [class data in body]
Returns The updated class

UPDATE ALL CLASSES OF THE STUDENT

HTTP Method PUT
URL http://root/students/{student_id}/classes/ + [shared data to update]
Returns Count of updated classes

GET ALL STUDENTS BY CLASS

HTTP Method GET
URL http://root/students/classes/{class_id}
Returns Count of updated classes

Would be happy to hear your opinions about it.

@bajtos
Copy link
Member

bajtos commented Mar 5, 2019

@TomerSalton thank you for writing down your proposal, I think it's a good start. Please note that we need to consider the Repository API provided to TS/JS consumers too, not only the REST API.

I have two comments regarding the proposed REST endpoints:

FINDS A CLASS AND CONNECT IT TO THE STUDENT
POST http://root/students/{student_id}/classes/{class_id}
REMOVES A CLASS FROM THE STUDENT
DELETE http://root/students/{student_id}/classes/{class_id}

I am concerned about these URLs, I think they may be confusing for API consumers - are we deleting the relation (link) only, or deleting also the target model?

In LoopBack 3.x, we have two flavours:

  • http://root/students/{student_id}/classes/{class_id} creates/deletes both the link and the target model
  • http://root/students/{student_id}/classes/rel/{class_id} creates/deletes the link only

I think it would be good to preserve both endpoints in LB4 too.

GET ALL STUDENTS BY CLASS
GET http://root/students/classes/{class_id}

IMO, this endpoint does not belong to Student REST API, it should be implemented on Class REST API as GET http://root/classes/{class_id}/students.

What do you think?

/cc @strongloop/loopback-next

@TomerSalton
Copy link

TomerSalton commented Mar 5, 2019

FINDS A CLASS AND CONNECT IT TO THE STUDENT
POST http://root/students/{student_id}/classes/{class_id}
REMOVES A CLASS FROM THE STUDENT
DELETE http://root/students/{student_id}/classes/{class_id}

I am concerned about these URLs, I think they may be confusing for API consumers - are we deleting the relation (link) only, or deleting also the target model?

In LoopBack 3.x, we have two flavours:

  • http://root/students/{student_id}/classes/{class_id} creates/deletes both the link and the target model
  • http://root/students/{student_id}/classes/rel/{class_id} creates/deletes the link only

I think it would be good to preserve both endpoints in LB4 too.

Sounds perfect.
Just wants to add a question about cascading;
A scenario where I delete class, and the class is connected to a student.
What should happen?

  • Deleting the relevant relations as well?
  • Preventing the deletion with the error?
    In case class has another connected object, such as notebook (if notebook has no class it shouldn't exist).
  • By deleting the class I should delete the related notebooks as well?

GET ALL STUDENTS BY CLASS
GET http://root/students/classes/{class_id}

IMO, this endpoint does not belong to Student REST API, it should be implemented on Class REST API as GET http://root/classes/{class_id}/students.

What do you think?

The reason I put this relation last is because I wasn't sure about it - from the same reason.
But giving this functionality enables a real 'hasAndBelongsToMany' by configuring the relation just from one side, without the need to configure it from ther other side as well.

@zhaoqin-github
Copy link

@TomerSalton @bajtos I am very glad to see you start to contribute many-to-many relation! My personal idea about the API is that we may support to create the relation just from one side as @TomerSalton proposed, and also support to retrieve the relation from both sides. For instance,

GET ALL STUDENTS BY CLASS
GET http://root/classes/{class_id}/students

GET ALL CLASSES BY STUDENT
GET http://root/students/{student_id}/classes

The API, GET http://root/students/classes/{class_id}, in previous comment looks a little strange. And I am not quite sure if HTTP router may regard 'classes' as a student id, according to some other API path registration.

@stale
Copy link

stale bot commented Mar 1, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Dec 25, 2020
@stale stale bot removed the stale label Mar 11, 2021
@stale
Copy link

stale bot commented Sep 7, 2021

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Sep 7, 2021
@achrinza achrinza removed the stale label Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature parity feature Relations Model relations (has many, etc.)
Projects
Status: Icebox
Development

Successfully merging a pull request may close this issue.

8 participants