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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Moving node back and forth in nested workspace causes SQL error #1640

Merged
merged 6 commits into from Jul 26, 2017
Merged

BUGFIX: Moving node back and forth in nested workspace causes SQL error #1640

merged 6 commits into from Jul 26, 2017

Conversation

robertlemke
Copy link
Member

@robertlemke robertlemke commented Jul 14, 2017 •
edited

This change fixes a problem in the content repository which can lead to a uniqueness constraint error when a node is moved back and forth in a nested workspace.

Fixes #1639

@robertlemke
TASK: Add Behat test which reproduces move node bug
c09653e 
This change contains a Behat test which reproduces the bug described in
issue neos#1639
@robertlemke robertlemke self-assigned this Jul 14, 2017
@robertlemke
BUGFIX: Moving node back and forth in nested workspace causes SQL error
2aeacd4 
This change fixes a problem in the content repository which can lead to
a uniqueness constraint error when a node is moved back and forth in a
nested workspace.

Fixes neos#1639
@robertlemke
Copy link
Member Author

The latest commit I pushed will fix the described bug and satisfies the Behat test which exposed the erroneous behavior.

However, when you move a node, publish, move it back and publish again, the target workspace won't be empty, as you might expect. It will have all previously touched nodes as materialized nodes, but without any further content (property) changes. Technically, this is somewhat expected and correct, but ideally could be optimized as a separate feature.

@robertlemke
Copy link
Member Author

Okay, looks like Postgres doesn't like publishing nodes to the target workspace which already exist. Maybe we need to implement the optimize I mentioned.

@kdambekalns
Copy link
Member

Ok, can confirm it fails, but …

  • if running all CR behat tests, it fails like with Travis CI
  • if running only MultiLayeredWorkspaces.feature or MultiLayeredWorkspaces.feature:201 a simple The EntityManager is closed. is the outcome of the step And I publish the workspace "user-admin".

For the MultiLayeredWorkspaces.feature:201 alone, query logs are attached (mysql.txt
& pgsql.txt). Notable differences start way down the files, the first few thousand lines only differ in schema management, UID generation and UUIDs.

Later (around line 6730 in mysql.txt) it becomes clear that PostgreSQL lacks a number of inserts and later updates, and in line 7227 (in mysql.txt) an obvious difference in checks for workspaces appears…

@robertlemke
TASK: Refactor filterRemovedNodes()
65ca355 
This change splits up filterRemovedNodes() into two meaningful
methods.
@robertlemke
BUGFIX: Fix for failing Postgres Behat test
fcbd208 
This change fixes the Behat test which exploits the given
bug in neos#1640. But further checks and fixes might be needed.
kitsunet
kitsunet approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for digging in :)

@robertlemke
BUGFIX: Moving node back and forth in nested workspace causes SQL error
be5743f 
This change fixes further issues with moving nodes back and forth which
occurred when moving nodes with sub structures. Also fixes the issue of
leftover shadow nodes in the source workspace when such back-and-forth
nodes were published.

Extended the Behat test to check this scenario more thoroughly.

Fixes neos#1639
@robertlemke robertlemke requested a review from hlubek July 20, 2017 17:08
kdambekalns
kdambekalns approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

Well done.

@kitsunet
Copy link
Member

I re-read the code tonight and (still) seems fine. IMHO should be merged.

@kdambekalns kdambekalns merged commit fe5f390 into neos:2.3 Jul 26, 2017
@kdambekalns kdambekalns deleted the bugfix-1639 branch July 26, 2017 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdambekalns kdambekalns kdambekalns approved these changes

@kitsunet kitsunet kitsunet approved these changes

@hlubek hlubek Awaiting requested review from hlubek

Assignees

@robertlemke robertlemke

Labels
Bug P: Content Repository P: Neos T: PHP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@robertlemke @kdambekalns @kitsunet