-
Notifications
You must be signed in to change notification settings - Fork 5
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
Allow users to handle invalid nodes coming from a search against the API #431
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
from json import JSONDecodeError | ||
import json | ||
from typing import Dict, Union | ||
from urllib.parse import quote | ||
|
||
|
@@ -33,6 +33,7 @@ class Paginator: | |
_current_position: int | ||
_fetched_nodes: list | ||
_number_fetched_pages: int = 0 | ||
auto_load_nodes: bool = True | ||
|
||
@beartype | ||
def __init__( | ||
|
@@ -119,8 +120,11 @@ def _fetch_next_page(self) -> None: | |
# if converting API response to JSON gives an error | ||
# then there must have been an API error, so raise the requests error | ||
# this is to avoid bad indirect errors and make the errors more direct for users | ||
except JSONDecodeError: | ||
response.raise_for_status() | ||
except json.JSONDecodeError as json_exc: | ||
try: | ||
response.raise_for_status() | ||
except Exception as exc: | ||
raise exc from json_exc | ||
|
||
# handling both cases in case there is result inside of data or just data | ||
try: | ||
|
@@ -137,8 +141,10 @@ def _fetch_next_page(self) -> None: | |
if api_response["code"] != 200: | ||
raise APIError(api_error=str(response.json()), http_method="GET", api_url=temp_url_path) | ||
|
||
node_list = load_nodes_from_json(current_page_results) | ||
self._fetched_nodes += node_list | ||
# Here we only load the JSON into the temporary results. | ||
# This delays error checking, and allows users to disable auto node conversion | ||
json_list = current_page_results | ||
self._fetched_nodes += json_list | ||
|
||
def __next__(self): | ||
if self._current_position >= len(self._fetched_nodes): | ||
|
@@ -147,14 +153,24 @@ def __next__(self): | |
raise StopIteration | ||
self._fetch_next_page() | ||
|
||
self._current_position += 1 | ||
try: | ||
return self._fetched_nodes[self._current_position - 1] | ||
next_node_json = self._fetched_nodes[self._current_position - 1] | ||
except IndexError: # This is not a random access iteration. | ||
# So if fetching a next page wasn't enough to get the index inbound, | ||
# The iteration stops | ||
raise StopIteration | ||
|
||
if self.auto_load_nodes: | ||
return_data = load_nodes_from_json(next_node_json) | ||
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. This is the ❤️ of this PR. |
||
else: | ||
return_data = next_node_json | ||
|
||
# Advance position last, so if an exception occurs, for example when | ||
# node decoding fails, we do not advance, and users can try again without decoding | ||
self._current_position += 1 | ||
|
||
return return_data | ||
|
||
def __iter__(self): | ||
self._current_position = 0 | ||
return self |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,11 +24,29 @@ def test_api_search_node_type(cript_api: cript.API) -> None: | |
|
||
# test search results | ||
assert isinstance(materials_paginator, Paginator) | ||
materials_list = list(materials_paginator) | ||
materials_list = [] | ||
while True: | ||
try: | ||
try: | ||
material_node = next(materials_paginator) | ||
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. This is using the change in a test. |
||
except cript.CRIPTException as exc: | ||
materials_paginator.auto_load_nodes = False | ||
material_json = next(materials_paginator) | ||
print(exc, material_json) | ||
else: | ||
materials_list += [material_node] | ||
finally: | ||
materials_paginator.auto_load_nodes = True | ||
except StopIteration: | ||
break | ||
# We don't need to search for a million pages here. | ||
if materials_paginator._number_fetched_pages > 6: | ||
break | ||
|
||
# Assure that we paginated more then one page | ||
assert materials_paginator._current_page_number > 0 | ||
assert materials_paginator._number_fetched_pages > 0 | ||
assert len(materials_list) > 5 | ||
first_page_first_result = materials_list[0]["name"] | ||
first_page_first_result = materials_list[0].name | ||
# just checking that the word has a few characters in it | ||
assert len(first_page_first_result) > 3 | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
FAQ to explain to users of how to recover from bad search results.
(Hopefully rarely needed.)
But good to have for debugging anyways.