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: Consider entry node when filtering in reference editor #5195

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueLessThan;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueLessThanOrEqual;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueStartsWith;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\SharedModel\Id\UuidFactory;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\PropertyName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"}} | {} |
Copy link
Member

Choose a reason for hiding this comment

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

😆

| a2a2 | Neos.ContentRepository.Testing:Page | a2a | {"text": "a2a2", "stringProperty": "the red fox", "booleanProperty": false, "integerProperty": 22, "floatProperty": 12.34, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-14"}} | {} |
# Note that this node is disabled! See below.
| a2a3 | Neos.ContentRepository.Testing:Page | a2a | {"text": "a2a3", "stringProperty": "the red Bear", "integerProperty": 19, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-12"}} | {} |
| a2a4 | Neos.ContentRepository.Testing:Page | a2a | {"text": "a2a4", "stringProperty": "the brown Bear", "integerProperty": 19, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-12"}} | {} |
| a2a5 | Neos.ContentRepository.Testing:Page | a2a | {"text": "a2a5", "stringProperty": "the brown bear", "integerProperty": 19, "dateProperty": {"__type": "DateTimeImmutable", "value": "1980-12-13"}} | {} |
Expand Down Expand Up @@ -109,7 +110,15 @@ Feature: Find and count nodes using the findChildNodes and countChildNodes queri

# Child nodes filtered by search term
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "brown"}' I expect the nodes "a2a1,a2a4,a2a5" to be returned
# The total count highlights that the search is case insensitive
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "bear", "pagination": {"limit": 3, "offset": 1}}' I expect the nodes "a2a5" to be returned and the total count to be 2
# Case insensitive multibyte search
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "äpfel"}' I expect the nodes "a2a1" to be returned
# Search for numbers (could be considered useless)
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "22"}' I expect the nodes "a2a2" to be returned
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "12.34"}' I expect the nodes "a2a1,a2a2" to be returned
# Search for boolean (could be considered useless)
When I execute the findChildNodes query for parent node aggregate id "a2a" and filter '{"searchTerm": "true"}' I expect the nodes "a2a1" to be returned

# Child nodes paginated
When I execute the findChildNodes query for parent node aggregate id "home" and filter '{"pagination": {"limit": 3}}' I expect the nodes "terms,contact,a" to be returned and the total count to be 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ private function __construct(
}

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

Choose a reason for hiding this comment

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

unrelated and otherwise not changed file

Copy link
Member Author

Choose a reason for hiding this comment

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

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

*
* @param Value $value
*/
public static function create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\NodeType\NodeTypeCriteria;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\SharedModel\Node\ReferenceName;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\NodeType\NodeTypeCriteria;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::countChildNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\NodeType\NodeTypeCriteria;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::countDescendantNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\NodeType\NodeTypeCriteria;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\SharedModel\Node\ReferenceName;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\SharedModel\Node\ReferenceName;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::findChildNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::findDescendantNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::findPrecedingSiblingNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\SharedModel\Node\ReferenceName;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\Pagination\Pagination;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueCriteriaInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\PropertyValueCriteriaParser;
use Neos\ContentRepository\Core\Projection\ContentGraph\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;

/**
* Immutable filter DTO for {@see ContentSubgraphInterface::findSucceedingSiblingNodes()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

declare(strict_types=1);

namespace Neos\ContentRepository\Core\Projection\ContentGraph;
namespace Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


/**
* A search term for use in Filters for the {@see ContentSubgraphInterface} API.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

declare(strict_types=1);

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

use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValue;
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValues;
use Neos\ContentRepository\Core\Projection\ContentGraph\Node;

/**
* Performs search term check against the nodes properties
*
* @internal
*/
class SearchTermMatcher
mhsdesign marked this conversation as resolved.
Show resolved Hide resolved
{
public static function matchesNode(Node $node, SearchTerm $searchTerm): bool
{
return static::matchesSerializedPropertyValues($node->properties->serialized(), $searchTerm);
}

public static function matchesSerializedPropertyValues(SerializedPropertyValues $serializedPropertyValues, SearchTerm $searchTerm): bool
{
foreach ($serializedPropertyValues as $serializedPropertyValue) {
if (self::matchesValue($serializedPropertyValue->value, $searchTerm)) {
return true;
}
}
return false;
}

private static function matchesValue(mixed $value, SearchTerm $searchTerm): bool
{
if (is_array($value) || $value instanceof \ArrayObject) {
foreach ($value as $subValue) {
if (self::matchesValue($subValue, $searchTerm)) {
return true;
}
}
return false;
}

return match (true) {
is_string($value) => mb_stripos($value, $searchTerm->term) !== false,
// the following behaviour might seem odd, but is implemented after how the database filtering should behave
is_int($value),
is_float($value) => str_contains((string)$value, $searchTerm->term),
$value === true => $searchTerm->term === 'true',
$value === false => $searchTerm->term === 'false',
default => throw new \InvalidArgumentException(sprintf('Handling for type %s is not implemented.', get_debug_type($value))),
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public function merge(self $other): self
return self::fromArray($nodes);
}

public function append(Node $node): self
Copy link
Member

Choose a reason for hiding this comment

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

Why not use merge?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted as requested :)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

{
return new self([...$this->nodes, $node]);
}

public function reverse(): self
{
return new self(array_reverse($this->nodes));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php

declare(strict_types=1);

namespace Neos\ContentRepository\Core\Tests\Unit\Projection\ContentGraph\Filter\SearchTerm;

use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValue;
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValues;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm;
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTermMatcher;
use PHPUnit\Framework\TestCase;

class SearchTermMatcherTest extends TestCase
{
public static function matchingStringComparisonExamples(): iterable
{
yield 'string found inside string' => ['brown', self::value('the brown fox')];
yield 'string found inside string (ci)' => ['BrOWn', self::value('the brown fox')];

yield 'string found inside multibyte string (ci)' => ['Äpfel', self::value('Auer schreit der bauer die äpfel sind zu sauer.')];

yield 'string matches full string' => ['Sheep', self::value('Sheep')];
yield 'string matches full string (ci)' => ['sheep', self::value('Sheep')];

yield 'string found inside string with special chars' => ['ä b-c+#@', self::value('the example: "ä b-c+#@"')];
}

public static function matchingNumberLikeComparisonExamples(): iterable
{
yield 'string-number found inside string' => [
'22',
self::value('feeling like 22 ;)'),
];

yield 'string-number found inside string-number' => [
'00',
self::value('007'),
];

yield 'string-number found inside int' => [
'23',
self::value(1234),
];

yield 'string-number found inside float' => [
'23',
self::value(1234.56),
];

yield 'string-float matches float' => [
'1234.56',
self::value(1234.56),
];

yield 'string-int matches int' => [
'0',
self::value(0),
];
}

public static function matchingBooleanLikeComparisonExamples(): iterable
{
yield 'string-boolean inside string' => [
'true',
self::value('this is true'),
];

yield 'string-true matches boolean' => [
'true',
self::value(true),
];

yield 'string-false matches boolean' => [
'false',
self::value(false),
];
}

public static function matchingArrayComparisonExamples(): iterable
{
// automates the following:
yield 'inside array: string found inside string' => ['foo', self::value(['foo'])];
yield 'inside array-object: string found inside string' => ['foo', self::value(new \ArrayObject(['foo']))];

foreach([
...iterator_to_array(self::matchingStringComparisonExamples()),
...iterator_to_array(self::matchingNumberLikeComparisonExamples()),
...iterator_to_array(self::matchingBooleanLikeComparisonExamples()),
] as $name => [$searchTerm, $properties]) {
/** @var SerializedPropertyValues $properties */
yield 'inside nested array: ' . $name => [$searchTerm, SerializedPropertyValues::fromArray(
array_map(
fn (SerializedPropertyValue $value) => SerializedPropertyValue::create(
// arbitrary deep nested
[[$value->value]],
'array'
),
iterator_to_array($properties)
)
)];
}
}


public function notMatchingExamples(): iterable
{
yield 'different chars' => ['aepfel', self::value('äpfel')];
yield 'upper boolean string representation' => ['TRUE', self::value(true)];
yield 'string not found inside string' => ['reptv', self::value('eras tour')];
yield 'integer' => ['0999', self::value(999)];
yield 'float with comma' => ['12,45', self::value(12.34)];
yield 'array with unmatched string' => ['hello', self::value(['hi'])];
yield 'array key is not considered matching' => ['key', self::value(['key' => 'foo'])];
yield 'nested array key is not considered matching' => ['key', self::value([['key' => 'foo']])];
}

/**
* @test
* @dataProvider matchingStringComparisonExamples
* @dataProvider matchingNumberLikeComparisonExamples
* @dataProvider matchingBooleanLikeComparisonExamples
* @dataProvider matchingArrayComparisonExamples
*/
public function searchTermMatchesProperties(
string $searchTerm,
SerializedPropertyValues $properties,
) {
self::assertTrue(
SearchTermMatcher::matchesSerializedPropertyValues(
$properties,
SearchTerm::fulltext($searchTerm)
)
);
}

/**
* @test
* @dataProvider notMatchingExamples
*/
public function searchTermDoesntMatchesProperties(
string $searchTerm,
SerializedPropertyValues $properties,
) {
self::assertFalse(
SearchTermMatcher::matchesSerializedPropertyValues(
$properties,
SearchTerm::fulltext($searchTerm)
)
);
}

private static function value(string|bool|float|int|array|\ArrayObject $value): SerializedPropertyValues
{
return SerializedPropertyValues::fromArray([
'test-property' => SerializedPropertyValue::create(
$value,
''
),
]);
}
}
Loading
Loading