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

BUGFIX: remove nodes from index on discard #25

Open
wants to merge 1 commit into
base: 3.0
Choose a base branch
from

Conversation

dimaip
Copy link

@dimaip dimaip commented Nov 20, 2017

Discarding a node does not emit nodeRemoved, so we should list for nodeDiscarded additionally.

Steps to reproduce:

  1. Create a new document node
  2. Discard it right away
  3. Observe that the node remains in ES index (it will be thrown away on conversion to Node results, but you may search for it manually in ES index).

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.

I did not test this, but after a short discussion with @robertlemke I veto this… I was confused as to why discarding a node does not trigger nodeRemoved, since it removes the node from some workspace…

But a discard operation is not removing that added node, but undoing the addition.

That is what makes this change (probably) dangerous: A discard signal could also mean some removal was discarded, or just a change to some property. Handling that all in removeNode() would lead to very unexpected results…

@dimaip
Copy link
Author

dimaip commented Nov 20, 2017

@kdambekalns look at the code here: https://github.com/neos/neos-development-collection/blob/2dd65a83c73c9dba7c61f7542dd17eb8a0511dd4/Neos.ContentRepository/Classes/Domain/Service/PublishingService.php#L198

Are you sure that by this point it does anything else besides removing a node?

Anyways, will take a closer look and try to reproduce what you are saying, but if someone with more experience in this area wants to take over, that'd be cool.

@kdambekalns
Copy link
Member

Not sure, no. As I said, I didn't test. That removal is handling NodeData, not Node, but… it would be correct, if all discard operations end up removing the node from the corresponding workspace.

But the index update might still need to be doing something else than "remove". Because if a removal or change is discarded, removing the data from the index of my workspace would not be correct.

@dimaip dimaip changed the base branch from master to 3.0 March 13, 2018 07:57
@kdambekalns
Copy link
Member

Doh, that's old by now… Does someone feel like investigating the current state?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants