From 21f67c9e28b2760926da496e8d4bef14e3dbf52b Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 09:11:00 -0800 Subject: [PATCH 1/8] Catch error response decoding json in search connector --- bookwyrm/books_manager.py | 3 ++- bookwyrm/connectors/abstract_connector.py | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bookwyrm/books_manager.py b/bookwyrm/books_manager.py index 3b8657686..bc1fa7231 100644 --- a/bookwyrm/books_manager.py +++ b/bookwyrm/books_manager.py @@ -5,6 +5,7 @@ from urllib.parse import urlparse from requests import HTTPError from bookwyrm import models +from bookwyrm.connectors import ConnectorException from bookwyrm.tasks import app @@ -55,7 +56,7 @@ def search(query, min_confidence=0.1): for connector in get_connectors(): try: result_set = connector.search(query, min_confidence=min_confidence) - except HTTPError: + except (HTTPError, ConnectorException): continue result_set = [r for r in result_set \ diff --git a/bookwyrm/connectors/abstract_connector.py b/bookwyrm/connectors/abstract_connector.py index ce1184d8c..221c59c0c 100644 --- a/bookwyrm/connectors/abstract_connector.py +++ b/bookwyrm/connectors/abstract_connector.py @@ -46,7 +46,10 @@ class AbstractMinimalConnector(ABC): ) if not resp.ok: resp.raise_for_status() - data = resp.json() + try: + data = resp.json() + except ValueError as e: + raise ConnectorException('Unable to parse json response', e) results = [] for doc in self.parse_search_data(data)[:10]: From d7db6d50ba4d3480d0167b1f0c9f80d3289cb766 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 09:14:07 -0800 Subject: [PATCH 2/8] Log errors in conenctor search --- bookwyrm/connectors/abstract_connector.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bookwyrm/connectors/abstract_connector.py b/bookwyrm/connectors/abstract_connector.py index 221c59c0c..fd2f8b3db 100644 --- a/bookwyrm/connectors/abstract_connector.py +++ b/bookwyrm/connectors/abstract_connector.py @@ -1,6 +1,7 @@ ''' functionality outline for a book data connector ''' from abc import ABC, abstractmethod from dataclasses import dataclass +import logging from urllib3.exceptions import RequestError from django.db import transaction @@ -11,6 +12,7 @@ from requests.exceptions import SSLError from bookwyrm import activitypub, models +logger = logging.getLogger(__name__) class ConnectorException(HTTPError): ''' when the connector can't do what was asked ''' @@ -49,6 +51,7 @@ class AbstractMinimalConnector(ABC): try: data = resp.json() except ValueError as e: + logger.exception(e) raise ConnectorException('Unable to parse json response', e) results = [] From 6a8353de096b4919fe6bf9913a437b3f189bd3b3 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 09:26:02 -0800 Subject: [PATCH 3/8] Adds test for SearchResult dataclass also just cleans up the styles in the test file for linting --- .../connectors/test_abstract_connector.py | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/bookwyrm/tests/connectors/test_abstract_connector.py b/bookwyrm/tests/connectors/test_abstract_connector.py index d6e02960e..bec9b11d5 100644 --- a/bookwyrm/tests/connectors/test_abstract_connector.py +++ b/bookwyrm/tests/connectors/test_abstract_connector.py @@ -2,12 +2,14 @@ from django.test import TestCase from bookwyrm import models -from bookwyrm.connectors.abstract_connector import Mapping +from bookwyrm.connectors.abstract_connector import Mapping, SearchResult from bookwyrm.connectors.openlibrary import Connector class AbstractConnector(TestCase): + ''' generic code for connecting to outside data sources ''' def setUp(self): + ''' we need an example connector ''' self.book = models.Edition.objects.create(title='Example Edition') models.Connector.objects.create( @@ -38,6 +40,7 @@ class AbstractConnector(TestCase): def test_create_mapping(self): + ''' maps remote fields for book data to bookwyrm activitypub fields ''' mapping = Mapping('isbn') self.assertEqual(mapping.local_field, 'isbn') self.assertEqual(mapping.remote_field, 'isbn') @@ -45,6 +48,7 @@ class AbstractConnector(TestCase): def test_create_mapping_with_remote(self): + ''' the remote field is different than the local field ''' mapping = Mapping('isbn', remote_field='isbn13') self.assertEqual(mapping.local_field, 'isbn') self.assertEqual(mapping.remote_field, 'isbn13') @@ -52,9 +56,24 @@ class AbstractConnector(TestCase): def test_create_mapping_with_formatter(self): + ''' a function is provided to modify the data ''' formatter = lambda x: 'aa' + x mapping = Mapping('isbn', formatter=formatter) self.assertEqual(mapping.local_field, 'isbn') self.assertEqual(mapping.remote_field, 'isbn') self.assertEqual(mapping.formatter, formatter) self.assertEqual(mapping.formatter('bb'), 'aabb') + + + def test_search_result(self): + ''' a class that stores info about a search result ''' + result = SearchResult( + title='Title', + key='https://example.com/book/1', + author='Author Name', + year='1850', + connector=self.connector, + ) + # there's really not much to test here, it's just a dataclass + self.assertEqual(result.confidence, 1) + self.assertEqual(result.title, 'Title') From c8d031e311d9cec540a7fac681152d13a8280fc2 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 09:48:37 -0800 Subject: [PATCH 4/8] Tests connector search --- .../connectors/test_abstract_connector.py | 83 +++++++++++++++---- 1 file changed, 66 insertions(+), 17 deletions(-) diff --git a/bookwyrm/tests/connectors/test_abstract_connector.py b/bookwyrm/tests/connectors/test_abstract_connector.py index bec9b11d5..cab2f834c 100644 --- a/bookwyrm/tests/connectors/test_abstract_connector.py +++ b/bookwyrm/tests/connectors/test_abstract_connector.py @@ -1,7 +1,9 @@ ''' testing book data connectors ''' from django.test import TestCase +import responses from bookwyrm import models +from bookwyrm.connectors import abstract_connector from bookwyrm.connectors.abstract_connector import Mapping, SearchResult from bookwyrm.connectors.openlibrary import Connector @@ -12,12 +14,12 @@ class AbstractConnector(TestCase): ''' we need an example connector ''' self.book = models.Edition.objects.create(title='Example Edition') - models.Connector.objects.create( + self.connector_info = models.Connector.objects.create( identifier='example.com', connector_file='openlibrary', base_url='https://example.com', - books_url='https:/example.com', - covers_url='https://example.com', + books_url='https://example.com/books', + covers_url='https://example.com/covers', search_url='https://example.com/search?q=', ) self.connector = Connector('example.com') @@ -39,6 +41,67 @@ class AbstractConnector(TestCase): ] + def test_abstract_minimal_connector_init(self): + ''' barebones connector for search with defaults ''' + class TestConnector(abstract_connector.AbstractMinimalConnector): + ''' nothing added here ''' + def format_search_result(): + pass + def get_or_create_book(): + pass + def parse_search_data(): + pass + + connector = TestConnector('example.com') + self.assertEqual(connector.connector, self.connector_info) + self.assertEqual(connector.base_url, 'https://example.com') + self.assertEqual(connector.books_url, 'https://example.com/books') + self.assertEqual(connector.covers_url, 'https://example.com/covers') + self.assertEqual(connector.search_url, 'https://example.com/search?q=') + self.assertIsNone(connector.name), + self.assertEqual(connector.identifier, 'example.com'), + self.assertIsNone(connector.max_query_count) + self.assertFalse(connector.local) + + + @responses.activate + def test_abstract_minimal_connector_search(self): + ''' makes an http request to the outside service ''' + class TestConnector(abstract_connector.AbstractMinimalConnector): + ''' nothing added here ''' + def format_search_result(self, data): + return data + def get_or_create_book(self, data): + pass + def parse_search_data(self, data): + return data + connector = TestConnector('example.com') + responses.add( + responses.GET, + 'https://example.com/search?q=a%20book%20title', + json=['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l'], + status=200) + results = connector.search('a book title') + self.assertEqual(len(results), 10) + self.assertEqual(results[0], 'a') + self.assertEqual(results[1], 'b') + self.assertEqual(results[2], 'c') + + + def test_search_result(self): + ''' a class that stores info about a search result ''' + result = SearchResult( + title='Title', + key='https://example.com/book/1', + author='Author Name', + year='1850', + connector=self.connector, + ) + # there's really not much to test here, it's just a dataclass + self.assertEqual(result.confidence, 1) + self.assertEqual(result.title, 'Title') + + def test_create_mapping(self): ''' maps remote fields for book data to bookwyrm activitypub fields ''' mapping = Mapping('isbn') @@ -63,17 +126,3 @@ class AbstractConnector(TestCase): self.assertEqual(mapping.remote_field, 'isbn') self.assertEqual(mapping.formatter, formatter) self.assertEqual(mapping.formatter('bb'), 'aabb') - - - def test_search_result(self): - ''' a class that stores info about a search result ''' - result = SearchResult( - title='Title', - key='https://example.com/book/1', - author='Author Name', - year='1850', - connector=self.connector, - ) - # there's really not much to test here, it's just a dataclass - self.assertEqual(result.confidence, 1) - self.assertEqual(result.title, 'Title') From 8ffb22291a68e2f1ad2ba3240bff348fb355ab58 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 09:51:37 -0800 Subject: [PATCH 5/8] Add connector to tested fields on search result --- bookwyrm/tests/connectors/test_bookwyrm_connector.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bookwyrm/tests/connectors/test_bookwyrm_connector.py b/bookwyrm/tests/connectors/test_bookwyrm_connector.py index 8d866ca25..1833ed4e3 100644 --- a/bookwyrm/tests/connectors/test_bookwyrm_connector.py +++ b/bookwyrm/tests/connectors/test_bookwyrm_connector.py @@ -31,6 +31,7 @@ class BookWyrmConnector(TestCase): def test_format_search_result(self): + ''' create a SearchResult object from search response json ''' datafile = pathlib.Path(__file__).parent.joinpath( '../data/fr_search.json') search_data = json.loads(datafile.read_bytes()) @@ -43,3 +44,4 @@ class BookWyrmConnector(TestCase): self.assertEqual(result.key, 'https://example.com/book/122') self.assertEqual(result.author, 'Susanna Clarke') self.assertEqual(result.year, 2017) + self.assertEqual(result.connector, self.connector) From e5a914c3c211d6125c66d1a871c5fe641b6559da Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 11:37:26 -0800 Subject: [PATCH 6/8] Fixes API search causing 500 error --- bookwyrm/connectors/abstract_connector.py | 8 +++++++- bookwyrm/views.py | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/bookwyrm/connectors/abstract_connector.py b/bookwyrm/connectors/abstract_connector.py index fd2f8b3db..ae73f75e1 100644 --- a/bookwyrm/connectors/abstract_connector.py +++ b/bookwyrm/connectors/abstract_connector.py @@ -1,6 +1,6 @@ ''' functionality outline for a book data connector ''' from abc import ABC, abstractmethod -from dataclasses import dataclass +from dataclasses import asdict, dataclass import logging from urllib3.exceptions import RequestError @@ -241,6 +241,12 @@ class SearchResult: return "".format( self.key, self.title, self.author) + def json(self): + ''' serialize a connector for json response ''' + serialized = asdict(self) + del serialized['connector'] + return serialized + class Mapping: ''' associate a local database field with a field in an external dataset ''' diff --git a/bookwyrm/views.py b/bookwyrm/views.py index 1afb82669..bd8948062 100644 --- a/bookwyrm/views.py +++ b/bookwyrm/views.py @@ -210,7 +210,7 @@ def search(request): if is_api_request(request): # only return local book results via json so we don't cause a cascade book_results = books_manager.local_search(query) - return JsonResponse([r.__dict__ for r in book_results], safe=False) + return JsonResponse([r.json() for r in book_results], safe=False) # use webfinger for mastodon style account@domain.com username if re.match(regex.full_username, query): From 490591623f796da4b4791ca5048d301c2a7b77b3 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 11:37:49 -0800 Subject: [PATCH 7/8] Adds test for search view --- bookwyrm/tests/test_views.py | 81 ++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 bookwyrm/tests/test_views.py diff --git a/bookwyrm/tests/test_views.py b/bookwyrm/tests/test_views.py new file mode 100644 index 000000000..ba4531f69 --- /dev/null +++ b/bookwyrm/tests/test_views.py @@ -0,0 +1,81 @@ +''' test for app action functionality ''' +import json +from unittest.mock import patch + +from django.http import JsonResponse +from django.template.response import TemplateResponse +from django.test import TestCase +from django.test.client import RequestFactory + +from bookwyrm import models, views +from bookwyrm.connectors import abstract_connector +from bookwyrm.settings import DOMAIN + + +class Views(TestCase): + ''' every response to a get request, html or json ''' + def setUp(self): + ''' we need basic test data and mocks ''' + self.factory = RequestFactory() + self.book = models.Edition.objects.create(title='Test Book') + models.Connector.objects.create( + identifier='self', + connector_file='self_connector', + local=True + ) + + + def test_search_json_response(self): + ''' searches local data only and returns book data in json format ''' + # we need a connector for this, sorry + request = self.factory.get('', {'q': 'Test Book'}) + with patch('bookwyrm.views.is_api_request') as is_api: + is_api.return_value = True + response = views.search(request) + self.assertIsInstance(response, JsonResponse) + + data = json.loads(response.content) + self.assertEqual(len(data), 1) + self.assertEqual(data[0]['title'], 'Test Book') + self.assertEqual( + data[0]['key'], 'https://%s/book/%d' % (DOMAIN, self.book.id)) + + + def test_search_html_response(self): + ''' searches remote connectors ''' + class TestConnector(abstract_connector.AbstractMinimalConnector): + ''' nothing added here ''' + def format_search_result(self, search_result): + pass + def get_or_create_book(self, remote_id): + pass + def parse_search_data(self, data): + pass + models.Connector.objects.create( + identifier='example.com', + connector_file='openlibrary', + base_url='https://example.com', + books_url='https://example.com/books', + covers_url='https://example.com/covers', + search_url='https://example.com/search?q=', + ) + connector = TestConnector('example.com') + + search_result = abstract_connector.SearchResult( + key='http://www.example.com/book/1', + title='Gideon the Ninth', + author='Tamsyn Muir', + year='2019', + connector=connector + ) + + request = self.factory.get('', {'q': 'Test Book'}) + with patch('bookwyrm.views.is_api_request') as is_api: + is_api.return_value = False + with patch('bookwyrm.books_manager.search') as manager: + manager.return_value = [search_result] + response = views.search(request) + self.assertIsInstance(response, TemplateResponse) + self.assertEqual(response.template_name, 'search_results.html') + self.assertEqual( + response.context_data['book_results'][0].title, 'Gideon the Ninth') From d3161ea3619677ba228839640d5427ae33d71a24 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 30 Dec 2020 11:42:07 -0800 Subject: [PATCH 8/8] Send appropriate error codes back with error pages --- bookwyrm/views.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views.py b/bookwyrm/views.py index bd8948062..2c30c16fa 100644 --- a/bookwyrm/views.py +++ b/bookwyrm/views.py @@ -38,12 +38,14 @@ def is_api_request(request): def server_error_page(request): ''' 500 errors ''' - return TemplateResponse(request, 'error.html', {'title': 'Oops!'}) + return TemplateResponse( + request, 'error.html', {'title': 'Oops!'}, status=500) def not_found_page(request, _): ''' 404s ''' - return TemplateResponse(request, 'notfound.html', {'title': 'Not found'}) + return TemplateResponse( + request, 'notfound.html', {'title': 'Not found'}, status=404) @login_required