Cannot update a model instance in a many to many relation

[federicoparroni]

Describe the bug
Updating a model instance involved in a many to many relation throws the following exception:

'(1054, "Unknown column \'mytable.pivot\' in \'field list\'")'

To Reproduce

1. Declare two models connected in a many to many relation:

   class Ssl(Model):
       __table__ = 'ssls'
       @belongs_to_many('ssl_id', 'app_id', 'id', 'id', table='ssl_app')
       def apps(self):
           from .app import App
           return App
    
    class App(Model):
       __table__ = 'apps'
       @belongs_to_many('app_id', 'ssl_id', 'id', 'id', table='ssl_app')
       def ssls(self):
           from .ssl import Ssl
           return Ssl

2. Update an entity:

   app = App.find(2)
   
   for ssl in app.ssls:
      # custom logic to decide if update or not
      ssl.my_column = my_value
      ssl.save()   # Exception!

Expected behavior
Model instance should be updated without issues.

Screenshots or code snippets
The generated query is:

"UPDATE `ssls` SET `ssls`.`pivot` = '?', `ssls`.`my_column` = '?' WHERE `ssls`.`id` = '?'"

with the following param bindings:

0:<masoniteorm.models.Pivot.Pivot object at 0x7f656007ebf0>
1:None
2:54

Desktop (please complete the following information):

• OS: Ubuntu
• Version: 21.04

What database are you using?

• Type: MySQL
• Version: 14.14 Distrib 5.7.30, for Linux (x86_64)
• Masonite ORM 2.6.1

[josephmancuso]

Thanks. Looks like the pivot table name is being overridden by the defaults instead of the one specified using the table argument

[josephmancuso]

actually looks like theres just an extra random pivot column trying to update somewhere

[federicoparroni]

Hi @josephmancuso. Thank you for the prompt reply.
I think that we need to add a check in Model.save() or QueryBuilder.update() that will allow to decide whether the pivot has to be inserted in the query or not from the __dirty_attributes__. In my example, I have no field to update from the pivot object, but it is inserted anyway in the columns to update

[federicoparroni]

Hello @josephmancuso! Can you please share any idea/suggestion that can help solving this issue so I can try to work on it?

[josephmancuso]

@federicoparroni

Thanks for the interest in fixing this. I believe the root of the issue is this block of code here https://github.com/MasoniteFramework/orm/blob/2.0/src/masoniteorm/relationships/BelongsToMany.py#L142-L149

what this is doing is it is setting the pivot object as on the model (typically named as pivot which would be the self._as attribute)

then when you go to save it, since we set the pivot attribute it is trying to save with the pivot column. This is because it thinks we set an attribute called pivot (because we did).

I THINK the solution is to actually just this this manually on the __original_attributes__ method like this:

model.__original_attributes__.update({self._as: (Pivot.on(query.connection)
                .table(self._table)
                .hydrate(pivot_data)
                .activate_timestamps(self.with_timestamps))})

Something like that. That would be the first place I start looking on this issue

A little more low level but I would imagine that this prevents the pivot attribute from being picked up in the update changes

[federicoparroni]

While debugging my specific case, it appears that this code is never reached: https://github.com/MasoniteFramework/orm/blob/2.0/src/masoniteorm/relationships/BelongsToMany.py#L142-L149

I think the problem is in here:

orm/src/masoniteorm/models/Model.py

Line 707 in a23458b

result = builder.update(self.__dirty_attributes__)

In the self.__dirty_attributes__ there is also the pivot attribute. Few lines before that, I see that we remove the builder from the __dirty_attributes__:

if "builder" in self.__dirty_attributes__:
    self.__dirty_attributes__.pop("builder")

Can we pop also for the pivot? Or are there some cases in where it is needed to be in the __dirty_attributes__?

[josephmancuso]

@federicoparroni no we can't do that. That code is in the model and the query builder is tightly coupled to the model so its ok to pop the builder there.

The relationship classes are not tightly coupled to the models so we need to find another way. Somewhere it is being set on the model so we just need to find where and modify that

[federicoparroni]

@josephmancuso Yes, you are right. I am currently trying to find out the exact point where the pivot is set in the __dirty_attributes__

[josephmancuso]

could be this line https://github.com/MasoniteFramework/orm/blob/2.0/src/masoniteorm/relationships/BelongsToMany.py#L273-L280

[federicoparroni]

Checking ;)

[federicoparroni]

@josephmancuso I think I should have fixed the issue. I'm going to open a PR!

[josephmancuso]

Ok ill review it when you do :)

[federicoparroni]

#649

[stoutput]

@federicoparroni Can this one be closed now? #649 has been merged.