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

Add unified search API #20916

Merged
merged 4 commits into from
Jun 24, 2020
Merged

Add unified search API #20916

merged 4 commits into from
Jun 24, 2020

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented May 11, 2020

See details and design decisions in the code. I felt like it's the better and more discoverable place than putting it into the PR description.

## Client flow

We decided that the providers should be independent in a way that one slow provider does not slow down the whole search experience, and also that the number of providers register does not influence the total time needed to get all the data.

Therefore the flow for the client (js) code is split into two parts.

  1. Get the current list of providers ``
  2. Fetch the results for each of the providers concurrently

So we can benefit from the async/concurrent nature of JavaScript in the browser and run multiple search requests in parallel on the server. It's up to the UI code to decide whether the completion of all requests will be awaited or results are shown as they come in.

Get list of providers

GET https://localhost/search/providers

returns

[
  "files",
  "mail"
]

Get individual search results

GET https://localhost/search/providers/files/search?term=cat

returns

{
  "name": "Files",
  "isPaginated": false,
  "entries": [
    {
      "thumbnailUrl": "path/to/icon.png",
      "title": "cute cats.jpg",
      "subline": "/Cats",
      "resourceUrl": "/f/21156"
    },
    {
      "thumbnailUrl": "path/to/icon.png",
      "title": "cat 1.jpg",
      "subline": "/Cats",
      "resourceUrl": "/f/21192"
    },
    {
      "thumbnailUrl": "path/to/icon.png",
      "title": "cat 2.jpg",
      "subline": "/Cats",
      "resourceUrl": "/f/25942"
    }
  ],
  "cursor": null
}

Docs are at nextcloud/documentation#2166


@ChristophWurst ChristophWurst added enhancement 2. developing Work in progress feature: search integration pending documentation This pull request needs an associated documentation update labels May 11, 2020
@ChristophWurst ChristophWurst added this to the Nextcloud 20 milestone May 11, 2020
@ChristophWurst ChristophWurst self-assigned this May 11, 2020
@georgehrke

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@rullzer

This comment has been minimized.

core/routes.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jun 23, 2020

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 23, 2020
@ChristophWurst ChristophWurst marked this pull request as ready for review June 23, 2020 08:42
@ChristophWurst ChristophWurst force-pushed the feature/unified-search-api branch from ed19296 to 1a58419 Compare June 23, 2020 16:55
@@ -77,6 +77,8 @@
['name' => 'RecommendedApps#index', 'url' => '/core/apps/recommended', 'verb' => 'GET'],
['name' => 'Svg#getSvgFromCore', 'url' => '/svg/core/{folder}/{fileName}', 'verb' => 'GET'],
['name' => 'Svg#getSvgFromApp', 'url' => '/svg/{app}/{fileName}', 'verb' => 'GET'],
['name' => 'UnifiedSearch#getProviders', 'url' => '/search/providers', 'verb' => 'GET'],
Copy link
Member

Choose a reason for hiding this comment

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

Can these be OCS so it's easier to use for clients and API consumers?

Copy link
Member

Choose a reason for hiding this comment

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

So we decided to delay OCS, until the frontend is there, so we are more sure it "works" and can do everything we need.

apps/comments/lib/Search/LegacyProvider.php Show resolved Hide resolved
apps/files/lib/Search/FilesSearchProvider.php Outdated Show resolved Hide resolved
@ChristophWurst ChristophWurst force-pushed the feature/unified-search-api branch from 1a58419 to bd6c598 Compare June 23, 2020 19:24
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

@ChristophWurst ChristophWurst force-pushed the feature/unified-search-api branch from bd6c598 to 3df8676 Compare June 24, 2020 08:17
@ChristophWurst
Copy link
Member Author

@since annotations were missing

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the feature/unified-search-api branch from 3df8676 to 2c699e0 Compare June 24, 2020 12:20
@ChristophWurst
Copy link
Member Author

And now a few tests failed 🙄

@ChristophWurst ChristophWurst merged commit 654cd18 into master Jun 24, 2020
@ChristophWurst ChristophWurst deleted the feature/unified-search-api branch June 24, 2020 13:38
@skjnldsv
Copy link
Member

skjnldsv commented Jul 16, 2020

I found some issues @ChristophWurst

@ChristophWurst
Copy link
Member Author

Files limit is not taken into account

This is about the current files search implementation, right? It's not a paginated one, so there is no limit per page. A limit only makes sense if you break down the result set into chunks as pages. When the provider does not support that, the unification logic is out of luck. You need to factor that in on the UI end.

Summary

  • Full results have an arbitrary number of entries
  • Paginated results should adhere to the specified limit, thus only return a certain number of entries

You're using OCP\Search\Provider & OC\Search\Result\File that are deprecated

This PR deprecated them 🙈 The files search provider is something that has to be redone. I just used the existing code and plugged it into the new one. This needs storage experts knowledge to transform properly, I lack that.

@skjnldsv
Copy link
Member

It's not a paginated one, so there is no limit per page. A limit only makes sense if you break down the result set into chunks as pages

And I can't enforce that?
Part of the spec is being able to paginate every results. I need to be able to show only 5 per types

@ChristophWurst
Copy link
Member Author

As I tried to explain, no, you can't.

The only thing we could do for this is to cap the results at a certain point for those providers that are not paginated. But then the user will never see the full list.

Part of the spec is being able to paginate every results.

But you can still do this. Just because the server gives you more data does not mean you have to show everything? Why not store the results as you get them, show the first x entries and when you would have to get the next page for a paginated provider you just show more of the already known entries.

It's a bit of additional work on the front-end, sure, but that is the best we can do if we want to support paginated and non-paginated providers.

@ChristophWurst
Copy link
Member Author

Docs at nextcloud/documentation#2166

@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants