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

Add related savings methods #415

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RomainMazB
Copy link

@RomainMazB RomainMazB commented Dec 31, 2021

This PR will adds an easier way to save related models, without having to pay attention to fill the id of the related model.
It will take some time to be implemented, but I already submitted it as a draft-PR so we can discuss about the API.

API

// Automatically fill the bakery_id into the baker_bob Model and then insert it
seafront_bakery
    .insert_related(
        bakery::Relation::Baker,
        baker_bob,
        &ctx.db
    )
    .await?;

// Update the bakery_id column to change a one-to-n relationships
baker_bob
    .set_related(
        baker::Relation::Bakery,
        seaside_bakery, // Bob changed his boss from seafront to seaside
        &ctx.db
    )
    .await?;

// Automatically fill the bakery_id into both the mud_cake and cheese_cake Models
// and then insert them
seaside_bakery
    .insert_many_related(
        bakery::Relation::Cake,
        vec![mud_cake, cheese_cake],
        &ctx.db
    )
    .exec(&ctx.db)
    .await?;

// Attach multiple many-to-many relations
// mud_cake and cheese_cake was already inserted in the database previously
baker_bob
    .attach_related(
        cakes_bakers::Relation::Cake,
        vec![mud_cake, cheese_cake],
        &ctx.db
    )
    .await?;

// Detach some of the many-to-many relationships
baker_bob
    .detach_related(
        cakes_bakers::Relation::Cake,
        vec![mud_cake],
        &ctx.db
    )
    .await?;

// Detach all related models, works whatever the relationship type is
baker_bob
    .detach_all_related(
        cakes_bakers::Relation::Cake,
        &ctx.db
    )
    .await?;

@billy1624 billy1624 mentioned this pull request Jan 2, 2022
@PauMAVA
Copy link

PauMAVA commented Jan 3, 2022

The API seems good. I would probably change attach_related for set_many_related so that it's clearer what this function does.

So if I'm not wrong the different failure points would be:

  • insert_related: When the related records have already been saved into the database.
  • set_related: When the related records have not been saved into the database.
  • insert_many_related: Whenever one of the related records has already been saved into the database.
  • attach_related: Whenever one of the related records has not been saved into the database previously.
  • detach_related: If the related record does not exist in the database.
  • detach_all_related: Never?

Maybe we could create a new method so that the record is automatically inserted if not saved already and then it updates the relation IDs. This method should be called set_related and set_may_related and the current set_related and set_many_related (attach_related) could be renamed to update_related and update_many_related.

Thoughts on this?

@billy1624
Copy link
Member

If I understand it correctly

  • insert_* will insert the related models in db and link the related models by setting the *_id foreign key columns correctly
  • attach_* & detach_* will just update the *_id columns of related models without deleting it from db

Correct?

@PauMAVA
Copy link

PauMAVA commented Jan 3, 2022

Yes, that is what I have understood from the tests @RomainMazB has made.

About the delete part that seems the most reasonable thing as in order to delete the related record from the database, we should need to check that it is not related to other models first.

Maybe @RomainMazB can clarify if we have understood this correctly?

@tyt2y3
Copy link
Member

tyt2y3 commented Jan 4, 2022

Have no thoughts on the API for now, I guess we'd want more input from users!

Do have a thought in the implementation strategy. I don't think we should fiddle too much with the current ActiveModel.

We can try introduce one more layer of abstraction, a RelationManager or something that builds atop ActiveModel. Because as we went by, we'd probably created a mini GraphQL api in the end.

@wzzrd
Copy link

wzzrd commented Jan 5, 2022

@RomainMazB pinged me on Discord about this, so I'll give my input fwiw.

I like @PauMAVA's suggestion on renaming attach_related to set_many_related. That makes sense. I'm not quite sure if the detach_related method works for all of one-to-one, one-to-many and many-to-many relations. If so, naming is good. If there are dedicated methods for one-to-* and many-to-many, this method too should be named differently. I'm assuming detach_related works on all types of relations though?

As for deletions, I'm thinking that in this context deletions are deletions of the relation, not of the related object. So if I have a many-to-many relation between bakeries and cakes, the fact that I removed the relation between a bakery and a cake does not mean I need to remove the cake. Removing the related object is probably something that should be done explicitly.

Something like that, anyway.

Finally, the suggestion to implement an additional abstraction layer to implement these relations, I'm not sure about that, if I may be so frank. I like my ORMs smart, powerful and simple. I use them for convenience reasons. Additional layers of abstraction might make an ORM smarter and more powerful, but not necessarily simpler.

When I started with programming, I used to use the ORMs that came with Django and Rails. Different times, for sure, but in terms of relation management, I like what they do. Simple, powerful API, implemented directly on the models themselves and smart.

I think the suggested API comes reasonably close to that, so I would be happy with this.

Anyway, I'm an amateur, so I might get this all wrong ;)

@RomainMazB
Copy link
Author

RomainMazB commented Dec 13, 2024

Reviving this old PR ✌🏻.

I don't think we should fiddle too much with the current ActiveModel.

We can try introduce one more layer of abstraction, a RelationManager or something that builds atop ActiveModel.

To me, the Related<R> trait already acts as the RelationManager as it already contains find_related method, or maybe we should move the find_related method somewhere else too?

About the delete part that seems the most reasonable thing as in order to delete the related record from the database, we should need to check that it is not related to other models first.

To clarify my vision, this is why I used detach and not delete in the method name.
Detachment means to me setting the _id column to null (or deleting the pivot table entry for many-to-many relationship).
The detached model would still live inside the database and it would be up to you to delete it or not through an additional query.
The situation where you are deleting a model that may still be attached to another one should be tackled from the database level through a fk cascade rule or at runtime if the developer wants to manually detach the model before deleting it.

To keep it clear and remind an up-to-date API, here is the list of the methods that would be added.

insert_related

one-to-one relationship only
Create the related model and set the _id column to this new id

insert_many_related

one-to-many and many-to-many relationships only
Same as insert_related for many models

update_related

one-to-one relationship only
With two living models in the database, would update the _id column to the correct id

update_many_related

one-to-many and many-to-many relationships only
Same as update_related for many models

attach_related

one-to-many and many-to-many relationships only
For one-to-many relationship: update the _id column
For many-to-many relationship: create an entry inside the pivot table

detach_related

one-to-many and many-to-many relationships only
Reverse of attach_related.
For one-to-many relationship: set the _id column to null
For many-to-many relationship: delete the entry from the pivot table

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

5 participants