Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve order of operations #620

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from
Open

Preserve order of operations #620

wants to merge 4 commits into from

Conversation

dantleech
Copy link
Contributor

This PR is aiming to preserve the order of operations, it tries to do this whilst preserving the current behavior as much as possible.

  • Everytime an INSERT, MOVE or DELETE is made, we add an Operation to the TreeOperationQueue
  • No such action is taken for REORDER or UPDATE

When flush is called:

  • The execute* methods are called with batches of operations, e.g. INSERT, INSERT, INSERT would be a single batch, however INSERT, INSERT, MOVE, INSERT would be three batches, so that the move is executed after the first 2 inserts.

@@ -813,7 +800,7 @@ private function doScheduleInsert($document, &$visited, $overrideIdGenerator = n
// TODO: Change Tracking Deferred Explicit
break;
case self::STATE_REMOVED:
unset($this->scheduledRemovals[$oid]);
$this->treeOpQueue->unqueue(TreeOperation::OP_REMOVE, $oid);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure about the validity of this. If the Document was previously removed, and then re-inserted we remove all REMOVE operations from the stack. Not sure if that could conflict somehow with MOVE or REMOVE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to record operations, i think you would need to queue another add operation instead. i am sure it would be possible to find some hairbrained scenario where this makes a difference. and probably some very real usecase with listeners doing many things could actually result in the same scenario and actually make sense...

@dantleech
Copy link
Contributor Author

@dbu any idea why this is not working on Jackrabbit? Something on our end?

@dantleech
Copy link
Contributor Author

CREATE /functional/user1
CREATE /functional/user2
CREATE /functional/user2/subuser
MOVE /functional/user2/subuser /functional/user1/subuser
REMOVE /functional/user2
FLUSH

Jackalope-jackrabbit sends the following POST:

--0935d494bb0a33542b1ed402934cca2a
--0935d494bb0a33542b1ed402934cca2a
--0935d494bb0a33542b1ed402934cca2a
Content-Disposition: form-data; name=":diff"
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

-/functional/user2 : buser : /functional/user1/subuserstructured","jcr:mixinTypes":["phpcr:managed"],"phpcr:class":"Doctrine\\Tests\\ODM\\PHPCR\\Functional\\User2","username":"test",}
--0935d494bb0a33542b1ed402934cca2a--

Resulting in:

PHPCR\PathNotFoundException: HTTP 409: /functional/user1/subuser

If I catch the PathNotFoundException the specific INSERT, MOVE, DELETE test works as expected.

@@ -927,16 +914,22 @@ public function scheduleMove($document, $targetPath)

switch ($state) {
case self::STATE_NEW:
unset($this->scheduledInserts[$oid]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a behaviour change i think, if we previously auto-persisted documents upon move operations.

@dbu
Copy link
Member

dbu commented Mar 27, 2015

interesting! about the jackrabbit issue: can you try to do that in a phpcr-api-test (CombinedOperationsTest i guess) to see if we have a bug there?

i think there is a philosophical question we need to decide. as i understood, doctrine calculates the optimal path to transform an object model into database operations, and does not record changes procedurally. but if we don't manage to calculate this transition, maybe explicit ordered operations make more sense?

@Ocramius @lsmith77 what do you think about this?

@dantleech dantleech self-assigned this Mar 29, 2015
@lsmith77 lsmith77 changed the title Preserve order of operations [WIP] Preserve order of operations May 1, 2015
@lsmith77 lsmith77 changed the title [WIP] Preserve order of operations Preserve order of operations May 1, 2015
@lsmith77 lsmith77 added ready and removed in progress labels May 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants