Skip to content
Open
4 changes: 2 additions & 2 deletions contentcuration/contentcuration/utils/pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ def paginate_queryset(self, queryset, request, view=None):
self.page = paginator.page(page_number)
except InvalidPage as exc:
msg = self.invalid_page_message.format(
page_number=page_number, message=str(exc)
page_number=page_number, message="Invalid page"
)
raise NotFound(msg)
raise NotFound(msg) from exc

if paginator.num_pages > 1 and self.template is not None:
# The browsable API should display pagination controls.
Expand Down
10 changes: 7 additions & 3 deletions contentcuration/contentcuration/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def base(request):
def health(request):
c = Channel.objects.first()
if c:
return HttpResponse(c.name)
return HttpResponse(c.name, content_type="text/plain")
return HttpResponse("No channels created yet!")


Expand Down Expand Up @@ -404,13 +404,17 @@ def set_language(request):
next_url_split = urlsplit(next_url) if next_url else None
if next_url and not is_valid_path(next_url_split.path):
next_url = translate_url(reverse("base"), lang_code)
response = HttpResponse(next_url) if next_url else HttpResponse(status=204)
response = (
HttpResponse(next_url, content_type="text/plain")
if next_url
else HttpResponse(status=204)
)
if request.method == "POST":
if lang_code and check_for_language(lang_code):
if next_url:
next_trans = translate_url(next_url, lang_code)
if next_trans != next_url:
response = HttpResponse(next_trans)
response = HttpResponse(next_trans, content_type="text/plain")
if hasattr(request, "session"):
# Storing the language in the session is deprecated.
# (RemovedInDjango40Warning)
Expand Down
101 changes: 67 additions & 34 deletions contentcuration/contentcuration/views/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
from contentcuration.viewsets.sync.utils import generate_update_event


logger = logging.getLogger(__name__)

VersionStatus = namedtuple("VersionStatus", ["version", "status", "message"])
VERSION_OK = VersionStatus(
version=rc.VERSION_OK, status=0, message=rc.VERSION_OK_MESSAGE
Expand Down Expand Up @@ -138,7 +140,10 @@ def check_version(request):
}
)
except Exception as e:
return HttpResponseServerError(content=str(e), reason=str(e))
handle_server_error(e, request)
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand All @@ -163,7 +168,10 @@ def file_diff(request):

return Response(to_return)
except Exception as e:
return HttpResponseServerError(content=str(e), reason=str(e))
handle_server_error(e, request)
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand All @@ -180,12 +188,14 @@ def api_file_upload(request):
try:
request.user.check_staged_space(fobj.size, checksum)
except Exception as e:
return HttpResponseForbidden(str(e))
handle_server_error(e, request)
return HttpResponseForbidden("Permission denied", content_type="text/plain")

try:
write_file_to_storage(fobj, check_valid=True)
except SuspiciousOperation as e:
return HttpResponseBadRequest(str(e))
except SuspiciousOperation:
logger.warning("SuspiciousOperation in api_file_upload", exc_info=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not know about the exc_info kwarg, handy!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Glad it's useful — it's the standard way to include the traceback in a warning-level log without using logger.exception (which logs at ERROR).

return HttpResponseBadRequest("Invalid file", content_type="text/plain")

StagedFile.objects.get_or_create(
checksum=checksum, file_size=fobj.size, uploaded_by=request.user
Expand All @@ -196,7 +206,9 @@ def api_file_upload(request):
return HttpResponseBadRequest("Invalid file upload request")
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand All @@ -222,13 +234,18 @@ def api_create_channel_endpoint(request):
"channel_id": obj.pk,
}
)
except KeyError as e:
except KeyError:
logger.warning(
"api_create_channel_endpoint missing required field", exc_info=True
)
return HttpResponseBadRequest(
"Required attribute missing from data | {}".format(str(e))
"Required attribute missing from data", content_type="text/plain"
)
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand Down Expand Up @@ -294,14 +311,17 @@ def api_commit_channel(request):
}
)
except (Channel.DoesNotExist, PermissionDenied):
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
except KeyError as e:
return HttpResponseNotFound("Channel not found", content_type="text/plain")
except KeyError:
logger.warning("api_commit_channel missing required field", exc_info=True)
return HttpResponseBadRequest(
"Required attribute missing from data | {}".format(str(e))
"Required attribute missing from data", content_type="text/plain"
)
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand Down Expand Up @@ -349,18 +369,23 @@ def api_add_nodes_to_tree(request):
}
)
except ContentNode.DoesNotExist:
return HttpResponseNotFound("No content matching: {}".format(parent_id))
except ValidationError as e:
return HttpResponseBadRequest(content=str(e))
except KeyError as e:
return HttpResponseNotFound("Content not found", content_type="text/plain")
except ValidationError:
logger.warning("api_add_nodes_to_tree ValidationError", exc_info=True)
return HttpResponseBadRequest("Validation error", content_type="text/plain")
except KeyError:
logger.warning("api_add_nodes_to_tree missing required field", exc_info=True)
return HttpResponseBadRequest(
"Required attribute missing from data | {}".format(str(e))
"Required attribute missing from data", content_type="text/plain"
)
except NodeValidationError as e:
return HttpResponseBadRequest(str(e))
except NodeValidationError:
logger.warning("api_add_nodes_to_tree NodeValidationError", exc_info=True)
return HttpResponseBadRequest("Invalid node data", content_type="text/plain")
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand Down Expand Up @@ -395,10 +420,12 @@ def api_publish_channel(request):
}
)
except (KeyError, Channel.DoesNotExist):
return HttpResponseNotFound("No channel matching: {}".format(data))
return HttpResponseNotFound("Channel not found", content_type="text/plain")
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand All @@ -418,10 +445,12 @@ def check_user_is_editor(request):
Channel.get_editable(request.user, channel_id)
return Response({"success": True})
except Channel.DoesNotExist:
return HttpResponseNotFound("Channel not found {}".format(channel_id))
return HttpResponseNotFound("Channel not found", content_type="text/plain")

except KeyError:
raise HttpResponseBadRequest("Missing attribute from data: {}".format(data))
return HttpResponseBadRequest(
"Missing attribute from data", content_type="text/plain"
)


@api_view(["POST"])
Expand Down Expand Up @@ -452,12 +481,14 @@ def get_tree_data(request):
children_data = tree_data.get("children", [])
return Response({"success": True, "tree": children_data})
except (Channel.DoesNotExist, PermissionDenied):
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
return HttpResponseNotFound("Channel not found", content_type="text/plain")
except ValueError:
return HttpResponseNotFound("No tree name matching: {}".format(tree_name))
return HttpResponseNotFound("Invalid tree name", content_type="text/plain")
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand Down Expand Up @@ -499,10 +530,12 @@ def get_node_tree_data(request):
}
return Response(response_data)
except Channel.DoesNotExist:
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
return HttpResponseNotFound("Channel not found", content_type="text/plain")
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


@api_view(["POST"])
Expand All @@ -529,14 +562,14 @@ def get_channel_status_bulk(request):

return Response({"success": True, "statuses": statuses})
except (Channel.DoesNotExist, PermissionDenied):
return HttpResponseNotFound(
"No complete set of channels matching: {}".format(",".join(channel_ids))
)
return HttpResponseNotFound("Channel not found", content_type="text/plain")
except KeyError:
raise ObjectDoesNotExist("Missing attribute from data: {}".format(data))
except Exception as e:
handle_server_error(e, request)
return HttpResponseServerError(content=str(e), reason=str(e))
return HttpResponseServerError(
"Internal server error", content_type="text/plain"
)


def get_status(channel_id):
Expand Down
2 changes: 1 addition & 1 deletion contentcuration/contentcuration/views/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def get_node_details(request, node_id):
node = ContentNode.objects.get(pk=node_id)
channel = node.get_channel()
if channel and not channel.public:
return HttpResponseNotFound("No topic found for {}".format(node_id))
return HttpResponseNotFound("Node not found", content_type="text/plain")
data = get_node_details_cached(request.user, node, channel)
return HttpResponse(json.dumps(data))

Expand Down
3 changes: 2 additions & 1 deletion contentcuration/contentcuration/views/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ def send_invitation_email(request):
message = render_to_string("permissions/permissions_email.txt", ctx_dict)
send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [user_email])
except KeyError:
logger.warning("send_invitation_email missing required field", exc_info=True)
return HttpResponseBadRequest(
"Missing attribute from data: {}".format(request.data)
"Missing attribute from data", content_type="text/plain"
)

return Response(InvitationSerializer(invitation).data)
Expand Down
19 changes: 12 additions & 7 deletions contentcuration/contentcuration/viewsets/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import json
import logging
import traceback
import uuid
from contextlib import contextmanager
Expand Down Expand Up @@ -38,6 +39,9 @@
from contentcuration.viewsets.sync.utils import log_sync_exception


logger = logging.getLogger(__name__)


class SimpleReprMixin(object):
def __repr__(self):
"""
Expand Down Expand Up @@ -709,7 +713,7 @@ def create_from_changes(self, changes):
errors.append(change)
except Exception as e:
log_sync_exception(e, user=self.request.user, change=change)
change["errors"] = [str(e)]
change["errors"] = ["Internal server error"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For all of these exceptions, I think this is probably the right call - and actual serialization validation errors would not fall through to this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — DRF ValidationError carries programmer-authored messages (not raw exception text), so propagating its detail is the right call for validation failures vs generic server errors.

errors.append(change)

return errors
Expand All @@ -723,8 +727,9 @@ def create(self, request, *args, **kwargs):
try:
self.perform_create(serializer)

except IntegrityError as e:
return Response({"error": str(e)}, status=409)
except IntegrityError:
logger.exception("RESTCreateModelMixin.create IntegrityError")
return Response({"error": "Internal server error"}, status=409)
instance = serializer.instance
return Response(self.serialize_object(pk=instance.pk), status=HTTP_201_CREATED)

Expand Down Expand Up @@ -754,7 +759,7 @@ def delete_from_changes(self, changes):
pass
except Exception as e:
log_sync_exception(e, user=self.request.user, change=change)
change["errors"] = [str(e)]
change["errors"] = ["Internal server error"]
errors.append(change)
return errors

Expand Down Expand Up @@ -796,7 +801,7 @@ def update_from_changes(self, changes):
errors.append(change)
except Exception as e:
log_sync_exception(e, user=self.request.user, change=change)
change["errors"] = [str(e)]
change["errors"] = ["Internal server error"]
errors.append(change)
return errors

Expand Down Expand Up @@ -836,7 +841,7 @@ def create_from_changes(self, changes):
except Exception as e:
log_sync_exception(e, user=self.request.user, changes=changes)
for change in changes:
change["errors"] = [str(e)]
change["errors"] = ["Internal server error"]
errors.extend(changes)
else:
valid_data = []
Expand Down Expand Up @@ -875,7 +880,7 @@ def update_from_changes(self, changes):
except Exception as e:
log_sync_exception(e, user=self.request.user, changes=changes)
for change in changes:
change["errors"] = [str(e)]
change["errors"] = ["Internal server error"]
errors.extend(changes)
if serializer.missing_keys:
# add errors for any changes that were specified but no object
Expand Down
Loading
Loading