django.core.serializers.get_serializer() should raise a more specific exception on invalid argument

[RoySmith]

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 :-)

[Alex]

Let's name the bikeshed SerializerNotFound :)

[RoySmith]

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.

[Alex]

Oh sorry I just meant the bikeshed of what to name the exception, I'm +1 on giving this on a more specific exception.

[jpic]

The uploaded patch is trivial. I couldn't find out in which file to code a test for that.

[magopian]

Also, patch needs improvements: there's a missing "not" in the test if format not in get_serializer_formats()

[magopian]

patch + tests + docs

[magopian]

Here's my attempt at the patch, with regression tests and documentation.

[Alex]

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.

[jacob]

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.

[magopian]

Integrated Alex's and Jacob's feedbacks and improvements requests

[magopian]

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?)

[Alex]

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.

[magopian]

integrated Alex's new feedbacks

[magopian]

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 !

[magopian]

My name and email, as requested by Alex: Mathieu Agopian, mathieu.agopian@… (fame! it's coming!)

[Alex]

In [16104]:

Fixed #15889 -- when trying to access to access a serializer that doesn't exist, raise a new SerializerDoesNotExist exception. Thanks to Mathieu Agopian for the patch.