pomm-project / ModelManager

Model Manager for the Pomm database framework.
MIT License
66 stars 27 forks source link

IdentityMapper and modified FlexibleEntities #23

Closed tlode closed 9 years ago

tlode commented 9 years ago

Pomm uses the Identity Map pattern to avoid having multiple instances of the same FlexibleEntity instance (identified by the primary key). In other words, issuing the same query twice should return the same FlexibleEntity reference.

But how about modified FlexibleEntities? Suppose a simple user object, e.g.

$user = $UsersModel->findWhere('username = $*', 'pika')->current();
$user2 = $UsersModel->findWhere('username = $*', 'pika')->current();
// $user === $user2 is true

$user->setFirstname('Pika');
// $user === $user2 is still true 
// ($user->status() & FlexibleEntityInterface::STATUS_MODIFIED) ===  FlexibleEntityInterface::STATUS_MODIFIED

$user3 = $UsersModel->findWhere('username = $*', 'pika')->current();
// ($user->status() & FlexibleEntityInterface::STATUS_MODIFIED) ===  FlexibleEntityInterface::STATUS_MODIFIED

// but $user->getFirstname() !== 'Pika'

Is this behavior the correct one? So either the entity status should be reset to "not modified" or the entity has to keep the modified values.

The latter could be achieved by simply swapping the arguments of array_merge in FlexibleContainer::hydrate():

    /**
     * hydrate
     *
     * @see FlexibleEntityInterface
     */
    public function hydrate(array $values)
    {
        // was $this->container = array_merge($this->container, $values);

        $this->container = array_merge($values, $this->container);
        return $this;
    }

Any opinions on this?

tlode commented 9 years ago

On the other hand, with the above change in hydrate(), discarding any changes on a FlexibleEntity would not be possible anymore, e.g.

// load user
$user = $UsersModel->findWhere('username = $*', 'chu')->current();
// modify username, don't update
$user->setUsername('pika'); 

...
// resync with db to discard changes
$user = $UsersModel->findWhere('username = $*', 'chu')->current();
// $userResynced->getUsername() is still 'pika'

Some control over the entity cache would be necessary, e.g.

// load user
$user = $UsersModel->findWhere('username = $*', 'chu')->current();
// modify username, don't update
$user->setUsername('pika'); 

// discard changes
$user->discardChanges();

// $user->getUsername() should be 'chu'
// $user->status() should be FlexibleEntityInterface::STATUS_EXIST
chanmix51 commented 9 years ago

I think changing the hydrate method is not the right approach because when you hydrate an entity you want to change its values and overwrite existing ones.

The question is more about how the identity mapper deals with instances. For now, if an existing instance is fetched from the database, the values coming from the database overwrite the existing ones in the instance, it acts like a refresh without changing the state (EXISTING, MODIFIED).

Is that the right way to do it ?

tlode commented 9 years ago

I think changing the hydrate method is not the right approach because when you hydrate an entity you want to change its values and overwrite existing ones.

I agree, this is the wrong approach.

it acts like a refresh without changing the state

This at least seems not right. If an instance got changed (but not stored to db) this should be reflected until someone calls refresh or reloads it by another query. Shouldn't then all changes been thrown away?

Ok, coming back to my original problem which led to this thread.

I am trying to use symfony's form component to manage request handling, validations and updates. Therefor I create the form, along with the loaded entity, simplified like this

$userProfile = $userProfilesModel->findById($userId);

$form = $this->createForm(new UserForm(), $userProfile);
$form->submit($request->request->all()); // this applies the request data on $userProfile entity

if ($form->isValid()) {
   $userProfilesModel->updateOne($userProfile);
} else {
  ... //error handling
}

This worked as long as I haven't added a unique entity validator into the chain of validations. This validator of course queries for the user and by doing this, it refreshs it! - and thereby loosing all the changes which were applied through the form submit method.

There might be other ways around this, but it bothers me, that this does not work. Only because some other object (the unique entity validator) is re-using the object.

What about Unit Of Work pattern? Could this be of help with this issue?

chanmix51 commented 9 years ago

Although the Identity mapper may not be perfect, I think your problem comes from not having the right action.

If I understand correctly :

1 You query a row, hydrate a FlexibleEntity 2 You apply changes on the entity 3 You query the db for rows having a field with the same value as the FlexibleEntity 4 Results are hydrated as FlexibleEntity instances 5 The identity mapper returns you with the same instance but it refreshes it, discarding any changes previously made.

Why not directly ask the database if another entity has this field assigned with this value ?

select exists (select true from :relation where pk_field != $* and field = $*)

This method would return a boolean: true if another row has field assigned to value, false otherwise without dealing with the identity mapper

Maybe we can add this query in the ReadQueries trait ?

tlode commented 9 years ago

Thanks @chanmix51.

Of course, changing the query finds its way around the problem. I think countWhere() would also do the job. And even findWhere() works as long as only count() on the result will be called and not the iterator.

This seems to be one of the pitfalls of Identity Map.

$entity = $model->findByPk(['id' => 1]);
$entity->setSomething('chu');
// until now you "know" the state of your entity

$anotherObject->someStuffToDo();

// now u cannot know anymore what the current state of the entity is
$model->updateOne($entity);

I close this, as it's not a bug, but - at least for me - it's still an interesting question.

chanmix51 commented 9 years ago

Be aware that countWhere() will traverse the whole table to count occurences. If the table is large this may be a problem. The exists SQL predicate stops the process as soon as an occurence is detected so this is performance wise and should be preferred for this kind of validators.

chanmix51 commented 9 years ago

A existWhere() method is added in ee9df72