From 7abaecd8b7aa26ce35f026b1c7fee77f41a14539 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 24 Oct 2013 18:40:37 -0400 Subject: [PATCH] Improve forum error handling CommentClientError now has sane subclasses that are meaningfully distinct, and each subclass is handled appropriately. Errors raised by the requests library are no longer handled by turning them into CommentClientErrors, since there is no meaningful handling we can do, and this way we will get more visibility into why errors are occurring. Also, HTTP status codes from the comments service indicating client error are correctly passed through to the client. --- CHANGELOG.rst | 4 ++ .../django_comment_client/forum/views.py | 31 +++-------- .../django_comment_client/middleware.py | 12 ++-- .../tests/test_middleware.py | 24 +++++--- lms/lib/comment_client/__init__.py | 5 +- lms/lib/comment_client/comment.py | 6 +- lms/lib/comment_client/models.py | 4 +- lms/lib/comment_client/thread.py | 6 +- lms/lib/comment_client/user.py | 10 ++-- lms/lib/comment_client/utils.py | 55 ++++++++++--------- 10 files changed, 78 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1318a13155..a4b5d31fea 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Improve forum error handling so that errors in the logs are +clearer and HTTP status codes from the comments service indicating +client error are correctly passed through to the client. + LMS: The wiki markup cheatsheet dialog is now accessible to people with disabilites. (LMS-1303) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 7281e779d5..f9cb2b2713 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -29,8 +29,8 @@ log = logging.getLogger("edx.discussions") @newrelic.agent.function_trace() def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAGE): """ - This may raise cc.utils.CommentClientError or - cc.utils.CommentClientUnknownError if something goes wrong. + This may raise an appropriate subclass of cc.utils.CommentClientError + if something goes wrong. """ default_query_params = { 'page': 1, @@ -113,13 +113,9 @@ def inline_discussion(request, course_id, discussion_id): course = get_course_with_access(request.user, course_id, 'load_forum') - try: - threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE) - cc_user = cc.User.from_django_user(request.user) - user_info = cc_user.to_dict() - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): - log.error("Error loading inline discussion threads.") - raise + threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE) + cc_user = cc.User.from_django_user(request.user) + user_info = cc_user.to_dict() with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) @@ -181,9 +177,6 @@ def forum_form_discussion(request, course_id): except cc.utils.CommentClientMaintenanceError: log.warning("Forum is in maintenance mode") return render_to_response('discussion/maintenance.html', {}) - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: - log.error("Error loading forum discussion threads: %s", str(err)) - raise user = cc.User.from_django_user(request.user) user_info = user.to_dict() @@ -252,11 +245,7 @@ def single_thread(request, course_id, discussion_id, thread_id): cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() - try: - thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id) - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): - log.error("Error loading single thread.") - raise + thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id) if request.is_ajax(): with newrelic.agent.FunctionTrace(nr_transaction, "get_courseware_context"): @@ -279,12 +268,8 @@ def single_thread(request, course_id, discussion_id, thread_id): with newrelic.agent.FunctionTrace(nr_transaction, "get_discussion_category_map"): category_map = utils.get_discussion_category_map(course) - try: - threads, query_params = get_threads(request, course_id) - threads.append(thread.to_dict()) - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): - log.error("Error loading single thread.") - raise + threads, query_params = get_threads(request, course_id) + threads.append(thread.to_dict()) course = get_course_with_access(request.user, course_id, 'load_forum') diff --git a/lms/djangoapps/django_comment_client/middleware.py b/lms/djangoapps/django_comment_client/middleware.py index 7876631d9e..3a4031f871 100644 --- a/lms/djangoapps/django_comment_client/middleware.py +++ b/lms/djangoapps/django_comment_client/middleware.py @@ -1,4 +1,4 @@ -from comment_client import CommentClientError +from comment_client import CommentClientRequestError from django_comment_client.utils import JsonError import json import logging @@ -8,17 +8,17 @@ log = logging.getLogger(__name__) class AjaxExceptionMiddleware(object): """ - Middleware that captures CommentClientErrors during ajax requests + Middleware that captures CommentClientRequestErrors during ajax requests and tranforms them into json responses """ def process_exception(self, request, exception): """ - Processes CommentClientErrors in ajax requests. If the request is an ajax request, + Processes CommentClientRequestErrors in ajax requests. If the request is an ajax request, returns a http response that encodes the error as json """ - if isinstance(exception, CommentClientError) and request.is_ajax(): + if isinstance(exception, CommentClientRequestError) and request.is_ajax(): try: - return JsonError(json.loads(exception.message), 500) + return JsonError(json.loads(exception.message), exception.status_code) except ValueError: - return JsonError(exception.message, 500) + return JsonError(exception.message, exception.status_code) return None diff --git a/lms/djangoapps/django_comment_client/tests/test_middleware.py b/lms/djangoapps/django_comment_client/tests/test_middleware.py index 54fcbd3c58..6b21fe056d 100644 --- a/lms/djangoapps/django_comment_client/tests/test_middleware.py +++ b/lms/djangoapps/django_comment_client/tests/test_middleware.py @@ -1,32 +1,38 @@ +import django.http from django.test import TestCase +import json import comment_client -import django.http import django_comment_client.middleware as middleware class AjaxExceptionTestCase(TestCase): - -# TODO: check whether the correct error message is produced. -# The error message should be the same as the argument to CommentClientError def setUp(self): self.a = middleware.AjaxExceptionMiddleware() self.request1 = django.http.HttpRequest() self.request0 = django.http.HttpRequest() - self.exception1 = comment_client.CommentClientError('{}') - self.exception2 = comment_client.CommentClientError('Foo!') - self.exception0 = ValueError() + self.exception1 = comment_client.CommentClientRequestError('{}', 401) + self.exception2 = comment_client.CommentClientRequestError('Foo!', 404) + self.exception0 = comment_client.CommentClient500Error("Holy crap the server broke!") self.request1.META['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX" def test_process_exception(self): response1 = self.a.process_exception(self.request1, self.exception1) self.assertIsInstance(response1, middleware.JsonError) - self.assertEqual(500, response1.status_code) + self.assertEqual(self.exception1.status_code, response1.status_code) + self.assertEqual( + {"errors": json.loads(self.exception1.message)}, + json.loads(response1.content) + ) response2 = self.a.process_exception(self.request1, self.exception2) self.assertIsInstance(response2, middleware.JsonError) - self.assertEqual(500, response2.status_code) + self.assertEqual(self.exception2.status_code, response2.status_code) + self.assertEqual( + {"errors": [self.exception2.message]}, + json.loads(response2.content) + ) self.assertIsNone(self.a.process_exception(self.request1, self.exception0)) self.assertIsNone(self.a.process_exception(self.request0, self.exception1)) diff --git a/lms/lib/comment_client/__init__.py b/lms/lib/comment_client/__init__.py index 6228c32024..bd8187ecfc 100644 --- a/lms/lib/comment_client/__init__.py +++ b/lms/lib/comment_client/__init__.py @@ -1,2 +1,5 @@ from .comment_client import * -from .utils import CommentClientError, CommentClientUnknownError +from .utils import ( + CommentClientError, CommentClientRequestError, + CommentClient500Error, CommentClientMaintenanceError +) diff --git a/lms/lib/comment_client/comment.py b/lms/lib/comment_client/comment.py index fd68d5cdeb..2b0d77b15f 100644 --- a/lms/lib/comment_client/comment.py +++ b/lms/lib/comment_client/comment.py @@ -1,4 +1,4 @@ -from .utils import CommentClientError, perform_request +from .utils import CommentClientRequestError, perform_request from .thread import Thread, _url_for_flag_abuse_thread, _url_for_unflag_abuse_thread import models @@ -48,7 +48,7 @@ class Comment(models.Model): elif voteable.type == 'comment': url = _url_for_flag_abuse_comment(voteable.id) else: - raise CommentClientError("Can only flag/unflag threads or comments") + raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} request = perform_request('put', url, params) voteable.update_attributes(request) @@ -59,7 +59,7 @@ class Comment(models.Model): elif voteable.type == 'comment': url = _url_for_unflag_abuse_comment(voteable.id) else: - raise CommentClientError("Can flag/unflag for threads or comments") + raise CommentClientRequestError("Can flag/unflag for threads or comments") params = {'user_id': user.id} if removeAll: diff --git a/lms/lib/comment_client/models.py b/lms/lib/comment_client/models.py index 97734c36e8..7296151c80 100644 --- a/lms/lib/comment_client/models.py +++ b/lms/lib/comment_client/models.py @@ -119,13 +119,13 @@ class Model(object): @classmethod def url(cls, action, params={}): if cls.base_url is None: - raise CommentClientError("Must provide base_url when using default url function") + raise CommentClientRequestError("Must provide base_url when using default url function") if action not in cls.DEFAULT_ACTIONS: raise ValueError("Invalid action {0}. The supported action must be in {1}".format(action, str(cls.DEFAULT_ACTIONS))) elif action in cls.DEFAULT_ACTIONS_WITH_ID: try: return cls.url_with_id(params) except KeyError: - raise CommentClientError("Cannot perform action {0} without id".format(action)) + raise CommentClientRequestError("Cannot perform action {0} without id".format(action)) else: # action must be in DEFAULT_ACTIONS_WITHOUT_ID now return cls.url_without_id() diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 00d5f01814..5cc14aa18f 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -1,5 +1,5 @@ from .utils import merge_dict, strip_blank, strip_none, extract, perform_request -from .utils import CommentClientError +from .utils import CommentClientRequestError import models import settings @@ -88,7 +88,7 @@ class Thread(models.Model): elif voteable.type == 'comment': url = _url_for_flag_comment(voteable.id) else: - raise CommentClientError("Can only flag/unflag threads or comments") + raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} request = perform_request('put', url, params) voteable.update_attributes(request) @@ -99,7 +99,7 @@ class Thread(models.Model): elif voteable.type == 'comment': url = _url_for_unflag_comment(voteable.id) else: - raise CommentClientError("Can only flag/unflag for threads or comments") + raise CommentClientRequestError("Can only flag/unflag for threads or comments") params = {'user_id': user.id} #if you're an admin, when you unflag, remove ALL flags if removeAll: diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 1af64cc18c..014fe0a810 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -1,4 +1,4 @@ -from .utils import merge_dict, perform_request, CommentClientError +from .utils import merge_dict, perform_request, CommentClientRequestError import models import settings @@ -41,7 +41,7 @@ class User(models.Model): elif voteable.type == 'comment': url = _url_for_vote_comment(voteable.id) else: - raise CommentClientError("Can only vote / unvote for threads or comments") + raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id, 'value': value} request = perform_request('put', url, params) voteable.update_attributes(request) @@ -52,14 +52,14 @@ class User(models.Model): elif voteable.type == 'comment': url = _url_for_vote_comment(voteable.id) else: - raise CommentClientError("Can only vote / unvote for threads or comments") + raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id} request = perform_request('delete', url, params) voteable.update_attributes(request) def active_threads(self, query_params={}): if not self.course_id: - raise CommentClientError("Must provide course_id when retrieving active threads for the user") + raise CommentClientRequestError("Must provide course_id when retrieving active threads for the user") url = _url_for_user_active_threads(self.id) params = {'course_id': self.course_id} params = merge_dict(params, query_params) @@ -68,7 +68,7 @@ class User(models.Model): def subscribed_threads(self, query_params={}): if not self.course_id: - raise CommentClientError("Must provide course_id when retrieving subscribed threads for the user") + raise CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user") url = _url_for_user_subscribed_threads(self.id) params = {'course_id': self.course_id} params = merge_dict(params, query_params) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index aef4476406..c24dac8121 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -55,35 +55,30 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): headers = {'X-Edx-Api-Key': settings.API_KEY} request_id = uuid4() request_id_dict = {'request_id': request_id} - try: - if method in ['post', 'put', 'patch']: - data = data_or_params - params = request_id_dict - else: - data = None - params = merge_dict(data_or_params, request_id_dict) - with request_timer(request_id, method, url): - response = requests.request( - method, - url, - data=data, - params=params, - headers=headers, - timeout=5 - ) - except Exception as err: - log.exception("Trying to call {method} on {url} with params {params}".format( - method=method, url=url, params=data_or_params)) - # Reraise with a single exception type - raise CommentClientError(str(err)) + + if method in ['post', 'put', 'patch']: + data = data_or_params + params = request_id_dict + else: + data = None + params = merge_dict(data_or_params, request_id_dict) + with request_timer(request_id, method, url): + response = requests.request( + method, + url, + data=data, + params=params, + headers=headers, + timeout=5 + ) if 200 < response.status_code < 500: - raise CommentClientError(response.text) + raise CommentClientRequestError(response.text, response.status_code) # Heroku returns a 503 when an application is in maintenance mode elif response.status_code == 503: raise CommentClientMaintenanceError(response.text) elif response.status_code == 500: - raise CommentClientUnknownError(response.text) + raise CommentClient500Error(response.text) else: if kwargs.get("raw", False): return response.text @@ -99,9 +94,15 @@ class CommentClientError(Exception): return repr(self.message) +class CommentClientRequestError(CommentClientError): + def __init__(self, msg, status_code=400): + super(CommentClientRequestError, self).__init__(msg) + self.status_code = status_code + + +class CommentClient500Error(CommentClientError): + pass + + class CommentClientMaintenanceError(CommentClientError): pass - - -class CommentClientUnknownError(CommentClientError): - pass