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
57 changes: 57 additions & 0 deletions tests/test_response.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import pytest
import json
from test_plus import APITestCase

from wazimap_ng.utils import error_handler

from django.test import RequestFactory


@pytest.mark.django_db
class TestErrorResponseData(APITestCase):

def test_error_handler(self):
factory = RequestFactory()
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved
request = factory.get('/api/v1/jkhj')

response = error_handler(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

you're headed in the wrong direction here.
you want to test the result of the exception raised by DRF and django and not call your own error_handler

status = response.status_code
content_type = response['Content-Type']

assert status == 500 and content_type == 'application/json'

def test_error_not_json(self):
factory = RequestFactory()
request = factory.get('/foo/bar')

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

assert content_type != 'application/json'

def test_error_format(self):
from wazimap_ng.renderer import CustomRenderer

expected_response = {
'error': {
"message": {
"detail": 'Not Found'
},
'type': None,
'code': 404
}
}

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

renderer = CustomRenderer()

renderer_context = dict(response=response)

data = renderer.render(
data=dict(detail='Not Found'),
renderer_context=renderer_context).decode('utf-8')

data = json.loads(data)

assert data == expected_response
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 @@ -271,11 +272,12 @@ 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",
"DEFAULT_RENDERER_CLASSES": (
"rest_framework.renderers.JSONRenderer",
"wazimap_ng.renderer.CustomRenderer",
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved
"rest_framework.renderers.BrowsableAPIRenderer",
"rest_framework_csv.renderers.PaginatedCSVRenderer",
),
Expand All @@ -284,7 +286,7 @@ class Common(QCluster, Configuration):
],
'DEFAULT_PERMISSION_CLASSES': [
"wazimap_ng.profile.authentication.ProfilePermissions",
]
],
}

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
23 changes: 23 additions & 0 deletions wazimap_ng/renderer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
from rest_framework.renderers import JSONRenderer


class CustomRenderer(JSONRenderer):

def error_data(self, data, type, code):
return dict(
error={
"message": data,
"type": type,
"code": code
}
)

def render(self, data, accepted_media_type=None, renderer_context=None):
response = renderer_context.get('response')

if response.status_code >= 400:
type = data.get('detail', None)
type = getattr(type, 'code', None)
data = self.error_data(data, type, response.status_code)

return super().render(data, accepted_media_type, renderer_context)
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'
42 changes: 33 additions & 9 deletions wazimap_ng/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import sys
import codecs
import uuid
import pandas as pd
from django.http import JsonResponse, HttpResponseServerError
from django.views.defaults import server_error
from collections import OrderedDict, defaultdict, Mapping
import logging

Expand All @@ -24,7 +27,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 +126,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 +297,7 @@ def test_long_input():
3: {
4: 5,
7: 8
}
}
}
}
}
Expand Down Expand Up @@ -418,13 +421,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 +457,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 +515,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 +551,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 +618,24 @@ def clean_columns(file):
new_columns = df.columns.str.lower().str.strip()
file.seek(0)
return old_columns, new_columns


def error_handler(request, **kwargs):
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved

if request.path.startswith('/api/v1'):
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using the path you should rather look for the content_type and if it's json return json otherwise leave as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this fix is to structure all error messages in json, I'll remove the if statement and render all 500 error responses in json.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, the issue was that all API requests should respond with JSON.
Not all requests. Otherwise the Admin will show JSON errors and the DRF preview potentially as well.

That's why we need the check for content type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. During test , I had to assign content_type to underlying request object so function could be tested without errors.


message = 'Server Error occurred.'
error_type = 'server_error'
code = 500
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved

data = dict(
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved
error={
'message': {'detail': message},
'type': error_type,
'code': code
}
)

return JsonResponse(data, status=500)
milafrerichs marked this conversation as resolved.
Show resolved Hide resolved

return server_error(request)