-
Notifications
You must be signed in to change notification settings - Fork 40
Added log for avoid silent incoherence between user input and app behaviour - non-existing fields #189
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
Added log for avoid silent incoherence between user input and app behaviour - non-existing fields #189
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,17 @@ def __init__(self, endpoint: HttpUrl): | |
| log.debug(f"Working on endpoint: {self.endpoint}") | ||
| self.UNIQUE_KEY = requests.get(urljoin(self.endpoint.encoded_string(), 'schema/uniquekey')).json()['uniqueKey'] | ||
| log.debug(f"uniqueKey found: {self.UNIQUE_KEY}") | ||
| # Solr default behavior, passing nonexistent fields results in a silent failure with no logging -- added logging | ||
| try: | ||
| # Ask the endpoint the existing fields | ||
| schema_resp = requests.get(urljoin(self.endpoint.encoded_string(), 'schema/fields')).json() | ||
| # subset only the present fields | ||
| self.schema_fields = {field['name'] for field in schema_resp.get('fields', [])} | ||
| except Exception as exc: | ||
| # In unit tests the mock response may not contain the expected structure -> store empty set | ||
| log.debug(f"Schema fields endpoint did not return expected payload: {exc}; defaulting to empty schema list") | ||
| self.schema_fields = set() | ||
| log.debug(f"Schema fields loaded: {len(self.schema_fields)} fields found.") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this entire PR is for writing this string in the logs. Why do you think this is useful?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I as developer / user rather knowing when some fields are present or not, than just executing and expect to behave nice. The problem was the silent omission of fields if not present on the schema
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another option could be implementing this directly on the _search method. Maybe makes more sense. But I'd like to know when the searched fields are not present |
||
|
|
||
| def _template_to_json_payload(self, template_payload: str) -> Dict[str, Any]: | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see
self.schema_fieldsused somewhere. Am I wrong?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is needed only in this small piece of code,
schema_fieldsshould be a local variableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be useful for checking the payload when building the queries to the engine