Opened 12 years ago
Closed 12 years ago
#15889 closed New feature (fixed)
django.core.serializers.get_serializer() should raise a more specific exception on invalid argument
Reported by: | Roy Smith | Owned by: | nobody |
---|---|---|---|
Component: | Core (Serialization) | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | mathieu.agopian@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
If you call get_serializer("foo"), where "foo" is not a valid serializer type, it raises KeyError (see Ticket #15886). It should raise something more specific, such as UnknownSerializerFormat.
We're trying to build a route structure something like:
url(r'/foo/object\.(?P<format>\w+)', api.get_object)
with a view that looks like:
def get_object(request, format):
serializer = django.core.serializers.get_serializer(format)
[...]
Then you can GET /foo/object.xml, /foo/object/json, etc and it all just works. The problem is, if you GET /foo/object.xyz, the get_serializer call will throw the generic KeyError, which means you need to deal with it right there (and the exception catching logic would have to be replicated in every view that used this) If it threw a more specific UnknownSerializerFormat, that could be caught in a middleware process_exception() handler, which could return HttpResponseNotFound or HttpResponseBadRequest or something like that (with an appropriate explanatory message in the body).
You don't want to catch KeyError in middleware; that's much too generic and could mask all sorts of coding errors.
Even if you wanted to keep it generic, ValueError seems more appropriate than KeyError. And it should be documented :-)
Attachments (4)
Change History (20)
comment:1 Changed 12 years ago by
Easy pickings: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 Changed 12 years ago by
I don't think it's fair calling this a bikeshed. Defining domain-specific exceptions is common practice. It allows users to write cleaner code, and it helps with debugging. I'm not claiming this is the most critical thing the Django project has in its queue, but that doesn't mean it's a bikeshed.
comment:3 Changed 12 years ago by
Oh sorry I just meant the bikeshed of what to name the exception, I'm +1 on giving this on a more specific exception.
Changed 12 years ago by
Attachment: | django_ticket_15889.patch added |
---|
comment:4 Changed 12 years ago by
The uploaded patch is trivial. I couldn't find out in which file to code a test for that.
comment:5 Changed 12 years ago by
Cc: | mathieu.agopian@… added |
---|
comment:6 Changed 12 years ago by
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
comment:7 Changed 12 years ago by
Patch needs improvement: | set |
---|
Also, patch needs improvements: there's a missing "not" in the test if format not in get_serializer_formats()
Changed 12 years ago by
Attachment: | 15889-get_serializer-specific-exception.diff added |
---|
patch + tests + docs
comment:8 Changed 12 years ago by
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Type: | Uncategorized → New feature |
Here's my attempt at the patch, with regression tests and documentation.
comment:9 Changed 12 years ago by
Patch needs improvement: | set |
---|
Couple things: a) you've got a random TestCase class with nothing on it, presumabley this can be removed, b) I'd simplify the check to if format not in _serializers
after the check if they're loaded, c) I'd like it if the SerializerDoesNotExist
exception was exposed at django.core.serializers
, d) The exception object should be instantiated with the non-existant serializer.
comment:10 Changed 12 years ago by
On more beyond Alex's todo list: I think SerializerNotFound
should subclass KeyError
to maintain backwards compatibility for folks expecting get_serializer
to raise KeyError
.
Changed 12 years ago by
Attachment: | 15889-get_serializer-specific-exception_2.diff added |
---|
Integrated Alex's and Jacob's feedbacks and improvements requests
comment:11 Changed 12 years ago by
Patch needs improvement: | unset |
---|
Second attempt : i integrated the feedbacks and modifications request from both Alex and Jacob, and added a few tests.
Please, let me know where/how to improve (specifically regarding the tests, as i feel it's far from KISS and "the one way to do it")
(As i updated the patch, i removed the "needs improvement" flag, that's the correct flow right?)
comment:12 Changed 12 years ago by
One last gripe with the tests, the as
syntax for exceptions is new in 2.6, let's use a context manager!
with self.assertRaises(SerializerDoesNotExist) as exc_info: serializers.get_serializer("nonsense") self.assertEqual(exc_info.args, ("nonsense",))
and you can remove the other test, it's already extensively tested that you can load a serializer.
Changed 12 years ago by
Attachment: | 15889-get_serializer-specific-exception_3.diff added |
---|
integrated Alex's new feedbacks
comment:13 Changed 12 years ago by
Third attempt, this time also renamed all the SerializerNotFound
to SerializerDoesNotExist
following conversation on irc with Alex
thanks for all the feedbacks, keep 'em coming !
comment:14 Changed 12 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:15 Changed 12 years ago by
My name and email, as requested by Alex: Mathieu Agopian, mathieu.agopian@… (fame! it's coming!)
Let's name the bikeshed
SerializerNotFound
:)