coldbox-modules / quick

A ColdBox ORM Engine
https://quick.ortusbooks.com
MIT License
23 stars 19 forks source link

Improve relationship assignment when creating new entities off of relationships #149

Open elpete opened 3 years ago

elpete commented 3 years ago

Here's two examples for BelongsTo and HasMany: (other relationships would need to be thought through, as well)

When creating an entity off of a BelongsTo relationship, the new related entity should be assigned to the relationship for the child entity. For example:

// Post.cfc
component extends="quick.models.BaseEntity" accessors="true" {

    function author() {
        return belongsTo( "User" );
    }

}
var post = getInstance( "Post" ).findOrFail( 1 );
var newAuthor = post.author().create( { ... } );
post.isRelationshipLoaded( "author" ) == true;
post.getAuthor() == newAuthor;

The same should be true for the child entity when creating it through a HasMany relationship:

// User.cfc
component extends="quick.models.BaseEntity" accessors="true" {

    function posts() {
        return hasMany( "Post" );
    }

}
var user = getInstance( "User" ).findOrFail( 1 );
var newPost = user.posts().create( { ... } );
newPost.isRelationshipLoaded( "author" ) == true;
newPost.getAuthor() == user;

It gets tricky when trying to handle the many side of the relationship. The parent entity may have other child entities related to it, and it would be incorrect to assign the relationship with only the newly created record. Even in the case of creating a new parent entity, it is possible (though unlikely) that the newly created parent entity already has related child entities.

One option is to only append the new record when the relationship is loaded. Loading a relationship is accepting that the data was current at that point, so we can safely add a new record without worry. Another option is to give some way for a developer to initialize the relationship as blank when creating.

One last thing to consider is if we add this feature should there be a way to disable it? I can't think of any use case that this would be a problem with, but it's at least worth asking the question.

elpete commented 3 years ago

cc @MordantWastrel Thanks for the initial ideas. We can continue discussing here.

MordantWastrel commented 3 years ago

I am in favor of an optional argument for this, at least for the first pass. It means no breaking changes and it relies on the developer to provide the assurance that the desired behavior is intended. And then we can consider making that behavior the default for the next major release based on feedback in the meantime.

Appending the new record when the relationship is already loaded seems safe. It doesn't address our use case but I know I've run into it before.