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

TASK: re-implement NodeSearchService #3771

Closed
Tracked by #4498
skurfuerst opened this issue May 7, 2022 · 14 comments · Fixed by #4555
Closed
Tracked by #4498

TASK: re-implement NodeSearchService #3771

skurfuerst opened this issue May 7, 2022 · 14 comments · Fixed by #4555
Labels

Comments

@skurfuerst
Copy link
Member

skurfuerst commented May 7, 2022

(see 2a9cc3b where it was removed)

@mhsdesign mhsdesign added the 9.0 label Apr 5, 2023
@mhsdesign mhsdesign changed the title re-implement NodeSearchService (see 2a9cc3bd2d3ad133b610e2b1da5ddd4a4ac04b96 where it was removed) TASK: re-implement NodeSearchService Sep 19, 2023
@mhsdesign mhsdesign mentioned this issue Sep 23, 2023
7 tasks
@mhsdesign
Copy link
Member

mhsdesign commented Sep 23, 2023

in the last weekly we actually discussed that we rather want to remove this search service as the findDescendantNodes subgraph api is powerful enough.

the only gain this interface / service provides is that all places using it could possibly take advantage of an improved implementation of this interface via like elastic search. But that also means that the results may vary by implementation and we concluded that it’s not worth it.

So instead the goal is to remove service and interface.

cc @bwaidelich and original author of this draft issue @skurfuerst

@mhsdesign
Copy link
Member

This would be a reimplementation

#4554

and this would be the full removal

#4555

lets decide ;)

@nezaniel
Copy link
Member

I'm all for reimplementation; Excluding the possibility to use specialized tools like ES for really large instances makes little sense to me

@bwaidelich
Copy link
Member

@mhsdesign thanks for the initiative – having concrete PRs makes it easier to discuss this topic!

I'm all for reimplementation

I beg to differ, well partly ;)
I'd be OK with a capable abstraction to find nodes (e.g. some kind of query builder or DSL (cypher?)) that you can replace the implementation for but IMO we don't gain anything by an interface whose default implementation is basically a one-line-wrapper around SubgraphInterface::findDescendantNodes() with a worse API.

If we look at todays implementation we can see to what weirdnesses such an over-general interface can lead to:

  • It is very implicit – e.g. we have to look at the implementation to understand whether multiple search terms are combined via AND or OR
  • the implementation has additional parameters to satisfy edge cases
  • we do some special handling of search terms that match NodeIdentifierValidator::PATTERN_MATCH_NODE_IDENTIFIER – which is basically matches any string since 6 years – other implementations will probably behave different which makes it really hard to depend on this API

To cut a long story short: I'm all for removing it until we can come up with a better interface (and IMO that doesn't even need to be part of the core)

@mhsdesign
Copy link
Member

I totally agree with Bastian. I couldn't come up with a good interface, and when swapping out the implementation - there is no guarantee that the behavior is the same (about and vs or...).

@nezaniel
Copy link
Member

But we already have an Interface (the subgraph one's), why not use that for now? Doesn't have to be perfect, let's ship something sensible in 9 and something better in 10

@mhsdesign
Copy link
Member

Yes lets remove it, ill await your formal approval of #4555

meanwhile ill close the reimplementation pr.

@mhsdesign
Copy link
Member

FYI as seen in https://neos-project.slack.com/archives/C050C9FKB/p1716545105918369?thread_ts=1716291704.931399&cid=C050C9FKB there are some real cool uses of replacing the search service. So we might have to reconsider our decision or implement some other kind of extension point:

Or as @Sebobo wrote:

Would be great if the search service learns to understand protocols and then calls a registered resolver for a given protocol, and uses the normal search if nothing matches. This way I wouldn’t need to replace the whole service in various projects

@kitsunet
Copy link
Member

Mmmm, yeah, although you can just implement somthing like it as EelHelper? I did always like it though, was a good extension point.

@mhsdesign
Copy link
Member

We couldnt decide for a new api signature (due to new complexity of multi crs) ill think its okay for the next person to do who will really want this feature, say for Neos 9.1 - we have to make some cutbacks to get Neos 9 out at all without to much bike shedding ^^

@Sebobo
Copy link
Member

Sebobo commented May 25, 2024

Is there a replacement for the node search in the document tree?

My customers all use the search in the backend a ton and also via my terminal and commandbar plugins which use the same service as it was so easy to use and okayish to modify. Just some days ago I made it full text capable via the Sandstorm Kissearch in one large project. Therefore I would need definitely a replacement before I can upgrade any those projects and my plugins. Having nothing is not better than having something not so awesome which worked good enough.

Maybe some of topics like these could be marked and discussed during the sprint? Maybe a simple solution be worked on? I would help of course.

@mhsdesign
Copy link
Member

yeah sure feel free to readd missing extension points ;) ... we should probably discuss this in a new issue in the Neos Ui. The global NodeSearchService is dead.

@bwaidelich
Copy link
Member

yeah sure feel free to readd missing extension points

extension points should be carefully crafted and coordinated such that we don't repeat mistakes from the past.
But obviously BE search has to work.
If it's broken right now I'd suggest to re-implement it using the Subgraph API for now. That should be capable and fast enough for most cases (for others, a custom package could replace the implementation and target ES or similar)

@bwaidelich
Copy link
Member

bwaidelich commented May 25, 2024

@Sebobo FYI I just tested it and the backend search still works because it uses the weird (but still supported) FlowQuery API and the custom SearchOperation that internally calls

Subgraph::findDescendantNodes(
    $contextNode->aggregateId,
    FindDescendantNodesFilter::create(
        searchTerm: $arguments[0],
        nodeTypes: $$arguments[1]
    )
);

It won't find nodes by their aggregate id though..
I added that with neos/neos-ui#3789 (requires #5098)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment