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

BUGFIX: Invalidate caches correctly after node move changes have been discarded #4291

Merged
merged 3 commits into from
May 14, 2024
Merged

BUGFIX: Invalidate caches correctly after node move changes have been discarded #4291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a209f3a
BUGFIX: Add base workspace to `nodeDiscarded` signal
grebaldi May 21, 2023
c99cd8b
BUGFIX: Ensure that ContentCacheFlusher registers all tags for actual…
grebaldi May 23, 2023
21ed60f
Merge remote-tracking branch 'origin/8.3' into bugfix/ui-3184/proper-…
mhsdesign Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Neos.ContentRepository/Classes/Domain/Service/PublishingService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ public function discardNode(NodeInterface $node)
*/
protected function doDiscardNode(NodeInterface $node, array &$alreadyDiscardedNodeIdentifiers = [])
{
if ($node->getWorkspace()->getBaseWorkspace() === null) {
$baseWorkspace = $node->getWorkspace()->getBaseWorkspace();
if ($baseWorkspace === null) {
throw new WorkspaceException('Nodes in a workspace without a base workspace cannot be discarded.', 1395841899);
}
if ($node->getPath() === '/') {
Expand Down Expand Up @@ -197,7 +198,7 @@ protected function doDiscardNode(NodeInterface $node, array &$alreadyDiscardedNo
}

$this->nodeDataRepository->remove($node);
$this->emitNodeDiscarded($node);
$this->emitNodeDiscarded($node, $baseWorkspace);
}

/**
Expand Down Expand Up @@ -274,11 +275,12 @@ public function emitNodePublished(NodeInterface $node, Workspace $targetWorkspac
* The signal emits the node that has been discarded.
*
* @param NodeInterface $node
* @param Workspace|null $baseWorkspace
* @return void
* @Flow\Signal
* @api
*/
public function emitNodeDiscarded(NodeInterface $node)
public function emitNodeDiscarded(NodeInterface $node, ?Workspace $baseWorkspace = null)
Copy link
Member

@mhsdesign mhsdesign Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fyi due to this this change needs a proxy cache flush as the Neos Neos PublishingService must be rebuild.

Declaration of Neos\Neos\Service\PublishingService::emitNodeDiscarded(Neos\ContentRepository\Domain\Model\NodeInterface $node) must be compatible with Neos\ContentRepository\Domain\Service\PublishingService::emitNodeDiscarded(Neos\ContentRepository\Domain\Model\NodeInterface $node, ?Neos\ContentRepository\Domain\Model\Workspace $baseWorkspace = null)

{
}

Expand Down
14 changes: 14 additions & 0 deletions Neos.Neos/Classes/Fusion/Cache/ContentCacheFlusher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,20 @@ protected function addTagToFlush(string $tag, string $message = ''): void

protected function registerAllTagsToFlushForNodeInWorkspace(NodeInterface $node, Workspace $workspace): void
{
// Ensure that we're dealing with the variant of the given node that actually
// lives in the given workspace
if ($node->getWorkspace()->getName() !== $workspace->getName()) {
Copy link
Member

Choose a reason for hiding this comment

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

@grebaldi do you think this could be a performance problem when working with a large number >1000 nodes?
Like publishing a whole workspace and for some reason creating a large number of contexts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question :)

Context creation itself doesn't strike me as very expensive, so I guess that would not be a problem. $workspaceContext->getNodeByIdentifier however will be quite expensive given an n=1000 iteration. It cannot be avoided though (IIRC).

Copy link
Member

@mhsdesign mhsdesign Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fyi i was curious when this Workspace $workspace parameter will actually different.

And i followed a bit the path an it seems its only set when publishing (and then its likely the base workspace of course):

neos-development-collection/Neos.ContentRepository/Classes/Domain/Service/PublishingService.php

Line 267 in 595fcba

public function emitNodePublished(NodeInterface $node, Workspace $targetWorkspace = null)

The ContextFactory::create seems to be cached but i agree the getNodeByIdentifier in 158 below might be more expensive.
... But then again isnt this cached? Yes. The firstLevelNodeCache actually caches the nodes by identifier ... huh .. only identifier not workspace? So my thesis is that the node returned is actually the same as passed (as that one likely exists in the firstLevelNodeCache.
The only thing to avoid this cache layer is to use the nodeDataRepository directly.

Maybe we should even consider extending the test flushingANodeWithAnAdditionalTargetWorkspaceWillAlsoResolveThatWorkspace to test this and be sure about it. ^^

Copy link
Member

Choose a reason for hiding this comment

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

Ah i see each Context has its own firstLevelNodeCache, that makes the caching of course workspace ware, which makes sense. And as we create the context just at that very second, the cache is completely empty so indeed we will query the content repository.

Copy link
Member

@mhsdesign mhsdesign Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No wait please ignore the following stuff i think its none sense :D

I dont fully understand this yet, but if we only want to ensure that $node->getWorkspace()->getName() === $workspace->getName()
we can cheat by building our own node:

$workspaceContext = $this->contextFactory->create(
    array_merge(
        $node->getContext()->getProperties(),
        ['workspaceName' => $workspace->getName()]
    )
);
$correctNode = $this->nodeFactory->createFromNodeData($node->getNodeData(), $workspaceContext);

but in case that is not our sole goal but we want to really ask the database, so the if ($node === null) return; will be used, that cheating will of course not work.

$workspaceContext = $this->contextFactory->create(
array_merge(
$node->getContext()->getProperties(),
['workspaceName' => $workspace->getName()]
)
);
$node = $workspaceContext->getNodeByIdentifier($node->getIdentifier());
if ($node === null) {
return;
}
}
$nodeIdentifier = $node->getIdentifier();

if (!array_key_exists($workspace->getName(), $this->workspacesToFlush) || is_array($this->workspacesToFlush[$workspace->getName()]) === false) {
Expand Down
Loading