!!! BUGFIX: Consider entry node when filtering in reference editor #5195

mhsdesign · 2024-08-04T11:44:51Z

If you search via the reference editor, the site node will never be found since we use ContentSubgraphInterface::findDescendantNodes from the starting point.
We need a means to also check the entry point against the search term $somehow.

This pr introduces an internal SearchTermMatcher similar to the PropertyValueCriteriaMatcher to compare a nodes php-object properties against any given search term. Note that the behaviour seems currently a little weird but its modelled after our mariadb database implementation of the subgraph (see \Neos\ContentGraph\DoctrineDbalAdapter\NodeQueryBuilder::addSearchTermConstraints).
The intentional quirks might be adjusted and fixed altogether with the postgres implementation but since this is a purely internal api for now this is fine.

Further the SearchTerm behaviour was adjusted to imply no filtering when being empty "".

The new official behaviour is as follows:

test all properties if one contains the term
the term is checked case-insensitive
an empty term will lead to no filtering
FIXME: define the search behaviour across non-string-typed properties

Upgrade instructions

The SearchTerm was moved from Neos\ContentRepository\Core\Projection\ContentGraph into Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm

Review instructions

See also our slack conversation: https://neos-project.slack.com/archives/C04PYL8H3/p1722437173713789

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

…ase filtering

…reference editor)

bwaidelich · 2024-08-04T11:48:48Z

Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeTraversal/ChildNodes.feature

@@ -76,8 +76,9 @@ Feature: Find and count nodes using the findChildNodes and countChildNodes queri
      | a1              | Neos.ContentRepository.Testing:Page        | a                      | {"text": "a1"}                                                                                                                                                                                       | {}                                       |
      | a2              | Neos.ContentRepository.Testing:Page        | a                      | {"text": "a2"}                                                                                                                                                                                       | {}                                       |
      | a2a             | Neos.ContentRepository.Testing:SpecialPage | a2                     | {"text": "a2a"}                                                                                                                                                                                      | {}                                       |
-      | a2a1            | Neos.ContentRepository.Testing:Page        | a2a                    | {"text": "a2a1", "stringProperty": "the brown fox", "booleanProperty": true, "integerProperty": 33, "floatProperty": 12.345, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-13"}} | {}                                       |
+      | a2a1            | Neos.ContentRepository.Testing:Page        | a2a                    | {"text": "a2a1", "stringProperty": "the brown fox likes Äpfel", "booleanProperty": true, "integerProperty": 33, "floatProperty": 12.345, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-13"}} | {}                                       |


bwaidelich

Sorry for the veto, but I have a couple of concerns. The main one being: This adds/duplicates logic from the core to the controller/runtime.
E.g. replicating search term matching to PHP code might behave slightly different from the other implementations and that would be a huge can of worms..

I think, extending the FindDescendantNodesFilter by some includeEntryNode as discussed via Slack would be a more stable and clean solution – even though it has that slight naming inaccuracy (which we could solve if we really wanted to)

bwaidelich · 2024-08-04T11:51:06Z

Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Filter/SearchTerm/SearchTerm.php

@@ -12,7 +12,7 @@

 declare(strict_types=1);

-namespace Neos\ContentRepository\Core\Projection\ContentGraph;
+namespace Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm;


I think, we had it in the main ContentGraph folder by intention because it is likely to be reused for other parts.
Can be discussed, but IMO we should not sneak in such changes in unrelated PRs as it makes the review needlessly harder and could confuse debugging

Yep, I would also let it stay where it was.

With the rest of the changes I came to understand why you moved it

I would almost argue the SearchTermMatcher doesn't belong in Projection/ContentGraph/Filters and that strengthens the argument the SearchTerm should be generic as well. Yes the SearchTermMatcher applies to contentgraph projection nodes, BUT I tihnk it's clear that everything else in filters is used in conjunction with a ContentGraph or Subgraph query (query in the loosest term not necessarily DB query), while the SearchTermMatcher applies to a set of nodes that have already been assembled. I think that is its own kind of operation.

i moved it to align it to the PropertyValueCriteriaMatcher it resides as well in the PropertyValue namespace and it was only logical to me that the same should apply for SearchTerm things. Id be fine with that non-ideal namespace for now as the SearchTermMatcher is marked as @internal for now and we can later decide on a better location for this and the PropertyValueCriteriaMatcher. Or is there a better suggestion out there? Probably with the introduction of the inmemory content graph well have a better namespace at hand.

...ntentRepository.Core/Classes/Projection/ContentGraph/Filter/SearchTerm/SearchTermMatcher.php

bwaidelich · 2024-08-04T11:53:51Z

Neos.Neos/Classes/Controller/Service/NodesController.php

+                }
+                $nodes = $nodes->merge(
+                    $subgraph->findDescendantNodes($entryNode->aggregateId, $filter)
+                );


uh oh.. that adds so much "domain logic" to the controller.. Why did you decide to go for this approach instead of extending the filter and leaving implementation to the adapters?

mhsdesign · 2024-08-04T17:21:05Z

Sorry for the veto, but I have a couple of concerns. The main one being: This adds/duplicates logic from the core to the controller/runtime.
E.g. replicating search term matching to PHP code might behave slightly different from the other implementations and that would be a huge can of worms.

The introduction of a SearchTermMatcher was done in alignment to the PropertyValueCriteriaMatcher see #4701. Previously i was also not really fond of a php PropertyValueCriteriaMatcher but in the end it turned out a good idea hear me out:
The behaviour of say search term and property value criteria was originally only defined by the respective database implementation and tests but there was no exact human readable documentation for each and every exact case (i consider the php code readable vs some sql queries *g)
That lead to the fact that we didn't know truly how the matching is defined and when implementing the php matcher we found out that our assumptions regarding case sensitivity were wrong and in that turn we overhauled the collation configuration and introduced further case insensitive matchers.
After these iterations the php matcher now aligns as far as the e2e tests say and we know to the database and document every bit what is possible.

With this pr introducing the SearchTermMatcher i hope to do the same to document what the respective db implementation should be capable of doing. Having that written in php and unit tested is a great way. And additionally i have run the e2e test locally using the php matcher and runtime filtering instead of the db to make sure the assumptions are fully correct.

The part where we use the SearchTermMatcher and node type filtering in the controller doesnt look that well currently, granted but that can be simplified further with one main api if we want.

So tltr. as we previously agreed on introducing the PropertyValueCriteriaMatcher - which helped us in lots of ways to define the functionality - its only logical to do the same with the SearchTermMatcher.

kitsunet · 2024-08-05T07:18:35Z

Neos.ContentRepository.Core/Classes/Feature/NodeModification/Dto/SerializedPropertyValue.php

@@ -38,8 +38,6 @@ private function __construct(
    }

    /**
-     * If the value is NULL an unset-property instruction will be returned instead.


unrelated and otherwise not changed file

I know yeah that the comment is false was my fault and i fixed it ^^

@mhsdesign

@mhsdesign Thanks for the elaboration. I'm still a bit undecided to be honest, but I'll remove by -1 for now

nezaniel · 2024-08-05T07:44:59Z

Regarding the code duplication issue: We'll need this for the InMemory content graph anyway once it is to be fully implemented

kitsunet · 2024-08-05T07:20:49Z

Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Nodes.php

@@ -124,6 +124,11 @@ public function merge(self $other): self
        return self::fromArray($nodes);
    }

+    public function append(Node $node): self


Why not use merge?

With merge I would expect an instance of Nodes. To add a single node to a set, "append" seems fine for me

True, but it wouldn't be the first place we use a merge(Nodes::fromArray([$node])) kind of construct ("upcast" single to collection). I just wanted to throw the question out there if we want to go down this route, because IMHO then we should add append (or similar) to other places probably?

In other places we named it more explicitly, like NodeTypeNames::withAdditionalNodeTypeName().
So we could call it withAppendedNode() for example.
But IMO merge(Nodes::fromArray([$node])) would be worse because it's

harder to comprehend

less explicit (with "append" it's clear that the new node is added to the end of the set)

slower (OK, that is probably negligible)

btw: I think prepend() would make more sense in this example

Yep, was mostly arguing for remembering to do it everywhere like this :) I am fine with having it, and yeah in htis case prepend seems sensible

Adjusted as requested :)

Sorry for re-opening, but when I suggested "Prepend" what I actually meant was withPrependedNode() – but I'm not sure whether we use the wither construct consistently everywhere.

ahh i see for collections the pattern as far as i have seen is to have merge or a method union and then i thought append as well but i get your point.

kitsunet · 2024-08-05T07:49:11Z

Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Filter/SearchTerm/SearchTerm.php

@@ -12,7 +12,7 @@

 declare(strict_types=1);

-namespace Neos\ContentRepository\Core\Projection\ContentGraph;
+namespace Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm;


I would almost argue the SearchTermMatcher doesn't belong in Projection/ContentGraph/Filters and that strengthens the argument the SearchTerm should be generic as well. Yes the SearchTermMatcher applies to contentgraph projection nodes, BUT I tihnk it's clear that everything else in filters is used in conjunction with a ContentGraph or Subgraph query (query in the loosest term not necessarily DB query), while the SearchTermMatcher applies to a set of nodes that have already been assembled. I think that is its own kind of operation.

mhsdesign · 2024-08-15T10:45:46Z

@nezaniel and me went over this again and cleaned up the cosmetics. Regarding the odd search behaviour for non-string properties we agreed to keep it like the db implementation for now. And since there is no InMemory namespace for now we think its best for consistency to have this new matcher in the Filter namespace right now. Especially under the circumstances it being internal. So in the future we can and will definitely be able to move it.

But are there any other suggestions for now?

bwaidelich · 2024-08-15T11:08:34Z

phpstan-baseline.neon

+		-
+			message: "#^Parameter \\#2 \\$searchTerm of static method Neos\\\\ContentRepository\\\\Core\\\\Projection\\\\ContentGraph\\\\Filter\\\\SearchTerm\\\\SearchTermMatcher\\:\\:matchesNode\\(\\) expects Neos\\\\ContentRepository\\\\Core\\\\Projection\\\\ContentGraph\\\\Filter\\\\SearchTerm\\\\SearchTerm, Neos\\\\ContentRepository\\\\Core\\\\Projection\\\\ContentGraph\\\\Filter\\\\SearchTerm\\\\SearchTerm\\|null given\\.$#"
+			count: 1
+			path: Neos.Neos/Classes/Controller/Service/NodesController.php


o.O don't we hide a potential bug by skipping this warning?

Thanks for the good qa :D Actually yes how did i miss that 🤯 :O

bwaidelich · 2024-08-15T11:11:42Z

Neos.Neos/Classes/Controller/Service/NodesController.php

@@ -110,6 +114,11 @@ public function indexAction(
        string $contextNode = null,
        array|string $nodeIdentifiers = []
    ): void {
+        $searchTerm = $searchTerm === '' ? null : SearchTerm::fulltext($searchTerm);


Ah, that's probably why we have to add some exceptions to the PHPStan baseline.
My IDE will complain about the inline type change:

And IMO it is correct. We really shouldn't do that

btw: if you do use a different variable name, the error is detected by the IDE (or at least by the Php Inspections plugin):

It also suggests a follow-up change that makes sense:

bwaidelich · 2024-08-15T11:12:29Z

Neos.Neos/Classes/Controller/Service/NodesController.php

+                    )
+                );
+                if (
+                    SearchTermMatcher::matchesNode($entryNode, $searchTerm)


If I read the code correctly, $searchTerm can indeed be null at this point leading to an exception..

mhsdesign · 2024-08-15T12:17:11Z

Pushed an additional bugfix as discussed with @nezaniel: BUGFIX: Ignore empty search term in subgraph api when filtering

previously using an SearchTerm dto with empty string was not the same as passing null instead. The doctrine dbal adapter would do its json search and match all nodes with at least one arbitrary property defined. We consider this weird and now empty string is a noop. By that we could simplify the code in the backendcontroller as well.

kitsunet · 2024-08-16T08:01:26Z

Seems fine, I still think this should reside somewhere else but as it is internal I agree with could still move it, although this seems to be the kind of useful thing people will want to use and half of them will ignore the internal annotation then 😆
Anyways +1 for now, as it is definitely a useful improvement.

nezaniel

As discussed a temporary but sensible solution

mhsdesign added 4 commits August 3, 2024 19:58

!!! TASK: Move SearchTerm to own namespace like other filters

9084d69

TASK: Add tests asserting the search term database filtering behaviour

989e7c5

FEATURE: Introduce SearchTermMatcher implemented according to datab…

ef93f94

…ase filtering

BUGFIX: Consider entry node in nodes endpoint for filtering (used in …

4715f33

…reference editor)

github-actions bot added Bug 9.0 labels Aug 4, 2024

bwaidelich reviewed Aug 4, 2024

View reviewed changes

bwaidelich previously requested changes Aug 4, 2024

View reviewed changes

TASK: Introduce array support for SearchTermMatcher

3bbabb3

kitsunet reviewed Aug 5, 2024

View reviewed changes

TASK: Introduce Nodes::prepend and cleanup code

5a8ab49

mhsdesign changed the title ~~BUGFIX: Consider entry node when filtering in reference editor~~ !!! BUGFIX: Consider entry node when filtering in reference editor Aug 15, 2024

mhsdesign requested review from nezaniel, bwaidelich and kitsunet August 15, 2024 10:39

bwaidelich reviewed Aug 15, 2024

View reviewed changes

mhsdesign added 2 commits August 15, 2024 14:11

TASK: Fix boolean behaviour for SearchTermMatcher

5bce349

BUGFIX: Ignore empty search term in subgraph api when filtering

0190e16

kitsunet approved these changes Aug 16, 2024

View reviewed changes

nezaniel approved these changes Aug 16, 2024

View reviewed changes

nezaniel merged commit 33fa7b0 into neos:9.0 Aug 16, 2024
10 checks passed

mhsdesign deleted the bugfix/consider-entry-node-when-filtering-in-reference-editor branch August 19, 2024 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!! BUGFIX: Consider entry node when filtering in reference editor #5195

!!! BUGFIX: Consider entry node when filtering in reference editor #5195

mhsdesign commented Aug 4, 2024 •

edited

Loading

bwaidelich Aug 4, 2024

bwaidelich left a comment

bwaidelich Aug 4, 2024

kitsunet Aug 5, 2024

bwaidelich Aug 5, 2024

kitsunet Aug 5, 2024

mhsdesign Aug 5, 2024

bwaidelich Aug 4, 2024

mhsdesign commented Aug 4, 2024

kitsunet Aug 5, 2024

mhsdesign Aug 5, 2024

nezaniel commented Aug 5, 2024

kitsunet Aug 5, 2024

bwaidelich Aug 5, 2024

kitsunet Aug 5, 2024

bwaidelich Aug 5, 2024

bwaidelich Aug 6, 2024

kitsunet Aug 6, 2024

mhsdesign Aug 15, 2024

bwaidelich Aug 15, 2024

mhsdesign Aug 15, 2024

kitsunet Aug 5, 2024

mhsdesign commented Aug 15, 2024

bwaidelich Aug 15, 2024

mhsdesign Aug 15, 2024

bwaidelich Aug 15, 2024

bwaidelich Aug 15, 2024

bwaidelich Aug 15, 2024

mhsdesign commented Aug 15, 2024

kitsunet commented Aug 16, 2024

nezaniel left a comment

!!! BUGFIX: Consider entry node when filtering in reference editor #5195

!!! BUGFIX: Consider entry node when filtering in reference editor #5195

Conversation

mhsdesign commented Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Aug 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezaniel commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Aug 15, 2024

kitsunet commented Aug 16, 2024

nezaniel left a comment

Choose a reason for hiding this comment

mhsdesign commented Aug 4, 2024 •

edited

Loading