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

fix: structure api error messages #214

Closed
wants to merge 10 commits into from
2 changes: 1 addition & 1 deletion config/django/django.env
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
DJANGO_SECRET_KEY=flkdjsflkdsjflkjlk4j4lk3j4l
DATABASE_URL=postgis://wazimap_ng:wazimap_ng@db:5432/wazimap_ng
DJANGO_DEBUG=True
GDAL_LIBRARY_PATH=/usr/share/gdal
GDAL_LIBRARY_PATH=/usr/share/gdal
REDIS_URL=redis://redis:6379
27 changes: 27 additions & 0 deletions tests/test_error_handling.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from test_plus import APITestCase

from rest_framework import status


class TestErrorResponseData(APITestCase):

def test_404_error_format(self):
expected_response = {
'error': {
"message": {
"detail": 'Not found.'
},
'type': 'not_found',
'code': status.HTTP_404_NOT_FOUND
}
}

response = self.get('dataset-detail', pk=234)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the correct test for the 404.
thanks. we need a similar one for the 500

Copy link
Contributor Author

@Geeker1 Geeker1 Mar 13, 2021

Choose a reason for hiding this comment

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

During testing, django raises the exception inplace of calling the handler500 function which returns the json response. I wrote the unit test for the handler500 function which is the test_error_handler_func, should I write the live server test case for 500 error ?


assert response.data == expected_response

def test_custom_exception(self):
response = self.get('/api/v1/datasets/3452/')

assert response.status_code == status.HTTP_404_NOT_FOUND
assert response.data['error']['type'] == 'not_found'
44 changes: 42 additions & 2 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
import csv
import codecs
import pytest
import json
from io import BytesIO

from wazimap_ng.utils import sort_list_using_order, detect_encoding
from wazimap_ng.utils import (
sort_list_using_order, detect_encoding,
error_handler, custom_exception_handler
)

from rest_framework import status

from test_plus import APITestCase


def test_empty_sort_using_order():
Expand Down Expand Up @@ -65,4 +74,35 @@ def generate_file(data, header, encoding="utf8"):
def test_detect_encoding():
buffer = generate_file(data_with_different_encodings, good_header, "Windows-1252")
encoding = detect_encoding(buffer)
assert encoding == "Windows-1252"
assert encoding == "Windows-1252"


class TestErrorHandling(APITestCase):

content_type = 'application/json'

def test_error_handler_func(self):
fake_request = self.get(
'/api/fake-request')

fake_request.wsgi_request.content_type = self.content_type

response = error_handler(fake_request.wsgi_request)
content_type = response['Content-Type']

assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert content_type == self.content_type

def test_error_json(self):
request = self.get('/foo/bar', content_type='application/json')

request.wsgi_request.content_type = self.content_type

response = error_handler(request.wsgi_request)
content_type = response['Content-Type']

data = json.loads(response.content)

assert content_type == self.content_type
assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert data['error']['type'] == 'server_error'
6 changes: 4 additions & 2 deletions wazimap_ng/config/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

os.environ["GDAL_DATA"] = "/usr/share/gdal/"


class Common(QCluster, Configuration):

SERVER_INSTANCE = os.environ.get("SERVER_INSTANCE", "Dev")
Expand Down Expand Up @@ -268,9 +269,9 @@ class Common(QCluster, Configuration):
}
}


# Django Rest Framework
REST_FRAMEWORK = {

"DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.PageNumberPagination",
"PAGE_SIZE": int(os.getenv("DJANGO_PAGINATION_LIMIT", 10)),
"DATETIME_FORMAT": "%Y-%m-%dT%H:%M:%S%z",
Expand All @@ -284,7 +285,8 @@ class Common(QCluster, Configuration):
],
'DEFAULT_PERMISSION_CLASSES': [
"wazimap_ng.profile.authentication.ProfilePermissions",
]
],
'EXCEPTION_HANDLER': 'wazimap_ng.utils.custom_exception_handler'
}

CORS_ORIGIN_ALLOW_ALL = True
Expand Down
2 changes: 1 addition & 1 deletion wazimap_ng/config/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class Local(Common):

## Honor the 'X-Forwarded-Proto' header for request.is_secure()
SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')

CACHES = {
'default': {
'BACKEND': 'django.core.cache.backends.filebased.FileBasedCache',
Expand Down
14 changes: 7 additions & 7 deletions wazimap_ng/datasets/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class DatasetIndicatorsList(generics.ListAPIView):

def get(self, request, dataset_id):
if models.Dataset.objects.filter(id=dataset_id).count() == 0:
raise Http404
raise Http404

queryset = self.get_queryset().filter(dataset=dataset_id)
queryset = self.paginate_queryset(queryset)
Expand All @@ -92,16 +92,16 @@ class GeographyHierarchyViewset(viewsets.ReadOnlyModelViewSet):
def search_geography(request, profile_id):
"""
Search autocompletion - provides recommendations from place names
Prioritises higher-level geographies in the results, e.g.
Provinces of Municipalities.
Prioritises higher-level geographies in the results, e.g.
Provinces of Municipalities.

Querystring parameters
q - search string
max-results number of results to be returned [default is 30]
max-results number of results to be returned [default is 30]
"""
profile = get_object_or_404(Profile, pk=profile_id)
version = profile.geography_hierarchy.root_geography.version

default_results = 30
max_results = request.GET.get("max_results", default_results)
try:
Expand All @@ -121,7 +121,7 @@ def sort_key(x):
return 0

else:
# TODO South Africa specific geography
# TODO South Africa specific geography
return {
"province": 1,
"district": 2,
Expand All @@ -144,7 +144,7 @@ def geography_ancestors(request, geography_code, version):
"""
geos = models.Geography.objects.filter(code=geography_code, version=version)
if geos.count() == 0:
raise Http404
raise Http404

geography = geos.first()
geo_js = AncestorGeographySerializer().to_representation(geography)
Expand Down
2 changes: 1 addition & 1 deletion wazimap_ng/general/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def authenticate_admin(user):
def notifications_view(request):
messages = request.session.pop("notifications", [])
task_list = request.session.get("task_list", [])

if messages and task_list:
for message in messages:
if "task_id" in message:
Expand Down
3 changes: 3 additions & 0 deletions wazimap_ng/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
def trigger_error(request):
division_by_zero = 1 / 0


urlpatterns = [

# Admin
Expand Down Expand Up @@ -152,3 +153,5 @@ def trigger_error(request):
path('__debug__/', include(debug_toolbar.urls)),

] + urlpatterns

handler500 = 'wazimap_ng.utils.error_handler'
64 changes: 55 additions & 9 deletions wazimap_ng/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import codecs
import uuid
import pandas as pd
from django.http import JsonResponse
from django.views.defaults import server_error

from rest_framework import status

from collections import OrderedDict, defaultdict, Mapping
import logging

Expand All @@ -24,7 +29,7 @@ def int_or_none(i):
if noney(i):
return None

return int(i)
return int(i)

def sort_list_using_order(lst, order, key_func=lambda x: x):
if len(lst) == 0:
Expand Down Expand Up @@ -123,7 +128,7 @@ def v(q, key):
arr = value
for el in arr:
current_dict[el] = v(q, args[-1]) # need to handle a tuple as well

else:
if type(args[-1]) == tuple:
current_dict[v(q, args[-2])] = [v(q, el) for el in args[-1]]
Expand Down Expand Up @@ -294,7 +299,7 @@ def test_long_input():
3: {
4: 5,
7: 8
}
}
}
}
}
Expand Down Expand Up @@ -418,13 +423,13 @@ def test_array_at_second_last_position():
1: {
3: {
"x": 5,
"y": 5
"y": 5
}
},
2: {
4: {
"x": 6,
"y": 6
"y": 6
}
},
}
Expand Down Expand Up @@ -454,7 +459,7 @@ def flatten_dict(d):
]

used as a component of the pivot function

"""
if not isinstance(d, Mapping):
return [[d]]
Expand Down Expand Up @@ -512,7 +517,7 @@ def nest(arrays, root=None):
elif len(tail) > 1:
d[head] = nest([tail], d[head])
return d

def pivot(d, order):
"""
Pivots an array by a list of keys
Expand Down Expand Up @@ -548,10 +553,10 @@ def pivot(d, order):
},
}

pivot(d, [2, 1, 0])
pivot(d, [2, 1, 0])

becomes:

d = {
"X": {
"Category1": {
Expand Down Expand Up @@ -615,3 +620,44 @@ def clean_columns(file):
new_columns = df.columns.str.lower().str.strip()
file.seek(0)
return old_columns, new_columns


def error_handler(request, *args, **kwargs):

message = 'Server Error occurred.'
error_type = 'server_error'
code = status.HTTP_500_INTERNAL_SERVER_ERROR

data = {
'error': {
'message': {'detail': message},
'type': error_type,
'code': code
}
}

if request.content_type == 'application/json':
return JsonResponse(
data, status=status.HTTP_500_INTERNAL_SERVER_ERROR)

return server_error(request, *args, **kwargs)


def custom_exception_handler(exc, context):
from rest_framework.views import exception_handler
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved
response = exception_handler(exc, context)

if response is not None:
type = response.data.get('detail', None)
type = getattr(type, 'code', None)

data = {
'error': {
"message": response.data,
"type": type,
"code": response.status_code
}
}
response.data = data

return response