New objects with IDs are not saved and no errors are thrown

[apiuser]

Feature request from flancer64, posted on GitHub Jun 08, 2016

Hello,

I create new stock object in case there is no stock with given ID (I know that ID is autoincremented value, this is code to create test data):

    try {
        /** @var \Magento\CatalogInventory\Api\StockRepositoryInterface $mageRepoStock */
        $stock = $mageRepoStock->get(self::DEF_STOCK_ID);
    } catch (\Magento\Framework\Exception\NoSuchEntityException $e) {
        /** @var \Magento\Framework\ObjectManagerInterface $manObj */
        $stock = $manObj->create(\Magento\CatalogInventory\Model\Stock::class);
        $stock->setStockId(self::DEF_STOCK_ID); // this string should be removed to create new object
    } finally {
        $stock->setStockName('Stock Name');
        $mageRepoStock->save($stock);
    }

There is no errors in this code and no new object is created. New object will be created if I remove following string from code:

        $stock->setStockId(self::DEF_STOCK_ID);

I debugged code and found that updateObject is called instead of saveNewObject in \Magento\Framework\Model\ResourceModel\Db\AbstractDb::save method:

            if ($this->isObjectNotNew($object)) {
                $this->updateObject($object);
            } else {
                $this->saveNewObject($object);
            }

This is a code for isObjectNotNew:

protected function isObjectNotNew(\Magento\Framework\Model\AbstractModel $object)
{
    return $object->getId() !== null && (!$this->_useIsObjectNew || !$object->isObjectNew());
}

I suppose we need additional validation in isObjectNotNew method or we need a error message.

OK, I will remove the ID setter from code but this behavior is not expected - no result and no error.

[apiuser]

Comment from orlangur, posted on GitHub Jun 09, 2016

Not sure if I understood your issue correct, but.

The new object is the one which does not have id, as soon as you save an object it now has id and you can refer to particular object using this id.

So, when you try to save object with id it means that such object is already present and you only need to update outdated data.

Obviously, if there is no such object in database, nothing will happen - the system just updated empty set of outdated fields. I see no reason to trigger any error in such case.

Talking about test data, hardcoding ids is a not reliable solution which may result in collisions and hard to find bugs (when one test in suite causes troubles for another one and the latter fails). It's better to isolate database between testcases so that autoincremented field has a predictable value, something like https://github.com/magento/magento2/blob/6ea7d2d85cded3fa0fbcf4e7aa0dcd4edbf568a6/dev/tests/integrat...

[apiuser]

Comment from flancer64, posted on GitHub Jun 09, 2016

Thanks for the answer.

The case is following. I create new object using Object Manager:

    $stock = $manObj->create(\Magento\CatalogInventory\Model\Stock::class);

this object exists in the memory only. It's OK. Let I set ID = 8 for the object that exists in memory only:

    $stock->setStockId(8);

Let there is no object with ID=8 in the DB. It's OK too. Then I try to save this object to DB using repository class:

    $mageRepoStock->save($stock);

No new object is created in DB. No error is thrown. It is not OK. This is unexpected for me. I think that there is object with ID=8 in the DB after save() operations completed without errors. But it is not true.

[apiuser]

Comment from flancer64, posted on GitHub Jun 09, 2016

OK, I allow that ID can be changed on save if autoincrement option is used for the ID field, but I expect that object itself is saved to DB if there are no errors on save.

[apiuser]

Comment from piotrekkaminski, posted on GitHub Jun 09, 2016

@flancer64 seems reasonable to me that for consistency, if there is no error thrown, object is saved @orlangur

[apiuser]

Comment from orlangur, posted on GitHub Jun 09, 2016

Where did you get an id, for instance, 8? The only way some entity can obtain an id is when you saved new entity without id.

If the system would allow to save new entities with arbitrary id specified, it would open the doors for various attacks, like, save entity with id near to max_int and just overflow the autoincrement.

So when your code deals with any id it assumes such object was already created before. So, you only can update it some of its fields, not create from scratch.

[apiuser]

Comment from flancer64, posted on GitHub Jun 09, 2016

@orlangur , I came up with this number. You can use any not existing ID to break up the saving. I suppose it is a good practice don't allow things that break other things. To deny ID setting for in-memory objects or to throw an error if object cannot be saved for any reason ("ID does not exist" for example).