nextras / orm

Orm with clean object design, smart relationship loading and powerful collections.
https://nextras.org/orm
MIT License
310 stars 57 forks source link

set() method don't removes other connections #217

Closed honzarac closed 7 years ago

honzarac commented 7 years ago

Hi,

I have problem with set method on relationship ManyHasMany. I have this code

        if($values->salesmen) {
            $this->entity->salesmen->set([$values->salesmen]);
        }

        $this->orm->persistAndFlush($this->entity);

version 2.2@dev

I want set only one entity and others remove but set method behaves same as method add.

Thanks for any help

hrach commented 7 years ago

Does stable version behave correctly?

honzarac commented 7 years ago

No it behaves same as @dev version

hrach commented 7 years ago

Could you please try to debug this? What does entity->salvesmen->get() return? etc...

honzarac commented 7 years ago

I have two saved salesmen in database 5 and 6. In form i select only one -> 5

        dump($this->entity->salesmen->get()->fetchPairs("id")); // [6, 5]
        if(!$this->entity){
            $this->entity = new Region;
        }
        $this->entity->assign($values, ['salesmen']); // setting values, salesman blacklisted
        dump($this->entity->salesmen->get()->fetchPairs("id", "name")); // [6, 5]
        if($values->salesmen) {
            $this->entity->salesmen->set([$values->salesmen->id]); // [5]
        }

        dump($this->entity->salesmen->get()); // [5]
        $this->orm->persistAndFlush($this->entity);
        dump($this->entity->salesmen->get()); // [5]

        $freshEntity = $this->orm->regions->getById(1);
        dump($freshEntity->salesmen->get()); // [5, 6]

// excecuted sqls for this code

::TRANSACTION BEGIN::   
UPDATE `regions` SET `name` = 'Brněnsko' WHERE `id` = 4 
DELETE FROM `users_x_regions` WHERE (`user_id`, `region_id`) IN ((5, 4))    
INSERT INTO `users_x_regions` (`user_id`, `region_id`) VALUES (5, 4)    
::TRANSACTION COMMIT::

Actually this code removes only relationship with salesman 5 and then create the same connection.

hrach commented 7 years ago

This is pretty weird. https://github.com/nextras/orm-demo/blob/e497a1a6d4bf87803d91363e4a430ef84ec68cca/app/presenters/HomepagePresenter.php#L103 actually uses this and it works correctly.

hrach commented 7 years ago

Do you have properly defined foreign keys?

honzarac commented 7 years ago

Yes, I have. You told me, that column id is unnecessary, but it never caused probles.

CREATE TABLE `users_x_regions` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `user_id` int(11) NOT NULL,
  `region_id` int(11) NOT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `user_id_region_id` (`user_id`,`region_id`),
  KEY `region_id` (`region_id`),
  KEY `user_id` (`user_id`),
  CONSTRAINT `users_x_regions_ibfk_1` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`),
  CONSTRAINT `users_x_regions_ibfk_2` FOREIGN KEY (`region_id`) REFERENCES `regions` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
hrach commented 7 years ago

That may be the case. You you please try to create primary on user_id & regoin_id and drop the id?

honzarac commented 7 years ago

I tried with this schema and wiped cache, but it still behaves the same way

CREATE TABLE `users_x_regions` (
  `user_id` int(11) NOT NULL,
  `region_id` int(11) NOT NULL,
  PRIMARY KEY (`user_id`,`region_id`),
  UNIQUE KEY `user_id_region_id` (`user_id`,`region_id`),
  KEY `region_id` (`region_id`),
  KEY `user_id` (`user_id`),
  CONSTRAINT `users_x_regions_ibfk_1` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`),
  CONSTRAINT `users_x_regions_ibfk_2` FOREIGN KEY (`region_id`) REFERENCES `regions` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
hrach commented 7 years ago

Are you able to send me some minimal repro zip?

stpnkcrk commented 7 years ago

Hello,

I discovered the same (or at least similar) bug to one mentioned earlier in this topic. I prepared minimal failing example attached to this post.

The only thing I think it may cause is that ->set() calls ->getCollection() internally without parameter whet ->get() calls internally ->getCollection(true), so with parameter forceNew = true.

When I removed this forceNew = true from ->get(), the behaviour was incorrect same as ->set().

Tested with current stables (nextras/dbal v2.1.0, nextras/orm v2.2.3).

nextras-orm-failing.zip

hrach commented 7 years ago

Well, I'm not sure if @kocourekio issue is connected to the original, which reports issue in ManyHasMany, the @kocourekio 's one is in OneHasMany, where the relationship is nullable.

hrach commented 7 years ago

@kocourekio fixed on master, I will try to fix this on stable branch, but it is not so easy :)

stpnkcrk commented 7 years ago

@hrach Thanks!

zaxxx commented 7 years ago

Hello. If the issue is m:m, check which side you're setting the data on. I was calling ->set() on the side which wasn't marked isMain=true and what it was doing is that it would always add new relations, but never remove those that should be removed. If the ->set() was only removing stuff, it would generate DELETE&INSERT in 2.2.3 and nothing in master. Using ->set() seems to work fine on the side marked with isMain=true. I dunno if it's a bug or by design, but if it's by design, maybe it should throw an exception yelling "hey, you're working on the wrong side", because right now it is stupidly easy to make a mistake without even noticing.

hrach commented 7 years ago

@zaxxx thanks for reporting, this is similar issue but not the same. I have fixed it in master and in v2.2 branch, so you can upgrade and check it out.

hrach commented 7 years ago

Fixed in master, but it pretty complex and difficult to fix in v2.2. Please, upgrade to master, today I wil also tag 3.0-beta1, so I encourage everyone to move to 3.0 :)