Method attach on model.Model does not seem to handle many-to-many relationship correctly

[garzola]

Describe the bug

The attach method on model.Model is currently broken with respect to many-to-many. It is looking for an attribute called foreign_key in the relationship. A many-to-many relationship does not have that attribute -- it has two foreign key attributes called local_foreign_key and other_foreign_key. Attempting to use attach for a many-to-many relationship results in an exception for *** AttributeError: 'QueryBuilder' object has no attribute 'foreign_key'

To Reproduce
Steps to reproduce the behavior:

1. Using the masonite-orm documentation, create a many-to-many relationship between two Models
2. modelA has the @BelongsToMany decorator, modelB does not
3. instance_of_modelA.attach("decorated_attribute", instance_of_modelB)

Expected behavior
I expect the linking table to get a new row with ids from both models

Screenshots or code snippets
*** AttributeError: 'QueryBuilder' object has no attribute 'foreign_key'

Desktop (please complete the following information):

• Mac OS X
• Big Sur and Catalina

What database are you using?

• SQLite
• Version 3.32.3
• Masonite ORM 1.0.55 (what ever is latest)

Additional context
None

[garzola]

So I have been thinking about how to address the different handling required by @BelongsToMany vs the other kinds of relationships. I've created a gist on what I am thinking for changes to Model.attach(). Would like feedback if this thinking is solid.

https://gist.github.com/garzola/bafc53150d296e868e81c9afd6aa1a78

I realize that similar changes will be required for save_many but I haven't gotten there yet

[josephmancuso]

The correct way to do to keep everything loosely coupled it to have each relationship class be responsible for handling the attachment.

Having this one method be responsible for all the relationships will make the code tightly coupled which goes against how I intended to maintain the codebase

[garzola]

So instead of attach being a method of Model, move it into the relationship object? That makes sense to me, but I'm going to have to dig in much more to understand what else might be affected.

[josephmancuso]

No not exactly. It would be both. When you called the attach on the model it would pass off that to the relationship object and return the result. This keeps everything loosely coupled:

Something like this:

def attach(self, relation, related_record):
        related = getattr(self.__class__, relation) # THIS IS THE RELATIONSHIP CLASS.

        setattr(
            related_record, related.foreign_key, self.__attributes__[related.local_key]
        ) # not sure if we need whatever this is doing

        return related.attach(PASS STUFF HERE WE NEED).

Then when we make future relationship classes, it would just need an attach method which handles attaching.

The current implementation could go inside the BaseRelationship and any differing implementation of attach can be overridden in the child class