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

feat: reduce search providers per config value "unified_search_providers_allowed" #48841

Conversation

printminion-co
Copy link
Contributor

@printminion-co printminion-co commented Oct 22, 2024

Summary

This feature should help to restrict not desired search providers by allowing only desired search providers.

Before

One can see the /ocs/v2.php/search/providers deliver various search providers (you may see other providers on your nextcloud instance depending on installed/disabled apps).
Selection_20241023-002

After feature activation

One can see we allowed only the files and settings search providers (since we do not want to allow any other search providers).

Selection_20241023-003

CLI Example: restrict search providers to files and settings

./occ config:app:set --value '["files","settings"]' --type array core unified_search.providers_allowed
  • Resolves: #

TODO

  • Disable search result on "disabled" providers e.g. /ocs/v2.php/search/providers/settings/search?term=f&from=%2Fapps%2Ffiles%2Ffiles

Checklist

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Is the endpoint also not responding when searching in disabled providers?

lib/private/Search/SearchComposer.php Outdated Show resolved Hide resolved
@sorbaugh sorbaugh requested a review from susnux October 22, 2024 15:13
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

I am not sure, as far as I know we do not have documentation about available search providers. So this config would be blind-guessing.
Maybe it is safer to make it an app config first until we have a occ command to list search providers?

@sorbaugh
Copy link
Contributor

would be blind-guessing. Maybe it is safer to make it an app config first until we have a occ command to list search providers?

@printminion-co would this be ok for you?

@printminion-co
Copy link
Contributor Author

printminion-co commented Oct 23, 2024

I am not sure, as far as I know we do not have documentation about available search providers. So this config would be blind-guessing.

I've added more description and screenshots to better describe the feature.
There is no blind-guessing since we allow and not disable the search providers.

Maybe it is safer to make it an app config first until we have a occ command to list search providers?

  • We know which providers we want keep enabled.
  • We do not need a listing.
  • Allowing not existing provider (e.g. foo) will not break the code

cc// @artonge @susnux @sorbaugh

@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch from 8939114 to a30aba1 Compare October 23, 2024 07:59
@susnux
Copy link
Contributor

susnux commented Oct 23, 2024

We know which providers we want keep enabled.
We do not need a listing.

For you this might be true, but my fear is that this will confuse other administrators of Nextcloud systems, because of the missing listing / documentation.

There is no blind-guessing since we allow and not disable the search providers.

Well if you know that API endpoint to retrive the available providers yes. What I meant is:
There is no list of available providers in the documentation and no OCC command to query it,
so this would be a feature mostly hidden and undocumented.

That is why I asked if instead of system config also an app config would work? Instead of writing it to the config.php just have it as an core app setting, we could later built an OCC command around it (listing + dis-/enable).

From my side no blocker, but just wanted to bring this up as this might lead to confusion in the future.

config/config.sample.php Outdated Show resolved Hide resolved
@printminion-co
Copy link
Contributor Author

Instead of writing it to the config.php just have it as an core app setting, we could later built an OCC command around it (listing + dis-/enable).

I will check this

@printminion-co
Copy link
Contributor Author

Instead of writing it to the config.php just have it as an core app setting, we could later built an OCC command around it (listing + dis-/enable).

do you mean here to use

$allowedProviders = $this->appConfig->getValueArray('core', 'unified_search.providers_allowed');

which is set via

./occ config:app:set --value '["files","settings"]' --type array core unified_search.providers_allowed

@susnux

@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch 2 times, most recently from b7a3c9a to 923692a Compare October 28, 2024 10:31
@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch from 923692a to 41d2e62 Compare October 29, 2024 11:33
@artonge
Copy link
Contributor

artonge commented Nov 5, 2024

Looks good to me. Could you answer my previous question? #48841 (review)

@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch from 41d2e62 to 9fcd189 Compare November 5, 2024 11:46
@printminion-co
Copy link
Contributor Author

Looks good to me. Could you answer my previous question? #48841 (review)

Not sure what "endpoint" mean.
Do you mean the API call of /ocs/v2.php/search/providers will deliver reduced providers? If so yes, one can observe it in the screenshots in PR description.

@artonge
Copy link
Contributor

artonge commented Nov 5, 2024

Not sure what "endpoint" mean. Do you mean the API call of /ocs/v2.php/search/providers will deliver reduced providers? If so yes, one can observe it in the screenshots in PR description.

No, I meant if you make requests to the search controllers, I suspect that disabled providers will still answer results.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@printminion-co
Copy link
Contributor Author

Not sure what "endpoint" mean. Do you mean the API call of /ocs/v2.php/search/providers will deliver reduced providers? If so yes, one can observe it in the screenshots in PR description.

No, I meant if you make requests to the search controllers, I suspect that disabled providers will still answer results.

You are right... ..the GET call to "disabled" controller /ocs/v2.php/search/providers/settings/search?term=f&from=%2Fapps%2Ffiles%2Ffiles still delivers a result.

I should also disable it

@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch from 9fcd189 to da9d705 Compare November 8, 2024 10:04
@printminion-co
Copy link
Contributor Author

Not sure what "endpoint" mean. Do you mean the API call of /ocs/v2.php/search/providers will deliver reduced providers? If so yes, one can observe it in the screenshots in PR description.

No, I meant if you make requests to the search controllers, I suspect that disabled providers will still answer results.

I moved the logic into the loadProviders Method

Now API call to disabled provider delivers same response as request to not existing search provider e.g. ocs/v2.php/search/providers/foo/search?term=f&from=%2Fapps%2Ffiles%2Ffiles

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>996</statuscode>
  <message>Internal Server Error
</message>
 </meta>
 <data/>
</ocs>

@nfebe nfebe added the pending documentation This pull request needs an associated documentation update label Nov 12, 2024
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

The admin documentation should mention somewhere that this is possible and how! Nice to have is probably a new follow issue covers the complete implementation of the occ command with list and dis/enable features.

@artonge
Copy link
Contributor

artonge commented Nov 13, 2024

Looks good @printminion-co. Can you rebase?

…rch.providers_allowed

reduce search providers by setting core config value to unified_search.providers_allowed = [ 'files', 'setting' ]

./occ config:app:set --value '["files","settings"]' --type array core unified_search.providers_allowed

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
@printminion-co printminion-co force-pushed the feat/config_unified_search_providers_allowed branch from da9d705 to 8e57004 Compare November 13, 2024 10:16
@printminion-co
Copy link
Contributor Author

Looks good @printminion-co. Can you rebase?

rebase done

@artonge artonge merged commit d660b08 into nextcloud:master Nov 13, 2024
357 of 360 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback-requested pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants