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

Less details in Solr connection error message #8196

Merged
merged 2 commits into from May 3, 2024

Conversation

FuhuXia
Copy link
Contributor

@FuhuXia FuhuXia commented Apr 23, 2024

Fixes #
Use a generic error message, not connection details in Solr error message.

Proposed fixes:

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

@FuhuXia FuhuXia force-pushed the solr-connection-error-message branch from 4b3f714 to 868b739 Compare April 24, 2024 00:05
@FuhuXia FuhuXia force-pushed the solr-connection-error-message branch from 868b739 to 85bda1a Compare April 24, 2024 00:07
@@ -470,7 +470,7 @@ def _check_query_parser(param: str, value: Any):
raise SearchQueryError('Invalid "sort" parameter')

if "Failed to connect to server" in e.args[0]:
raise SolrConnectionError("Connection Error", message=e.args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Can we get the details logged with a log.warning(e.args[0])? This might help maintainers debug the actual issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes were made so that
Site Visitors sees "Solr returned an error while searching.",
Site Maintainers sees "Connection Error: Failed to connect to Solr server."

@amercader
Copy link
Member

There's another issue with the code merged in #8094 . The SolrConnectionError exception class for some reasons inherits from urllib3's NewConntectionError. This expects a connection as a first parameter, not a string.

This is causing typing errors and will probably cause other problems when we actually raise this exception. I don't see why we need to extend urllib3 class and we can't simply do the following like the rest of the search exceptions:

class SolrConnectionError(Exception):
    pass

@FuhuXia do you mind updating the exception class as part of this PR and check that the behaviour of #8094 is kept? Thanks

This was referenced Apr 24, 2024
@FuhuXia FuhuXia force-pushed the solr-connection-error-message branch from d02c29e to f9bd198 Compare April 24, 2024 22:01
@amercader amercader merged commit b166fa2 into ckan:master May 3, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants