Opened 13 years ago

Closed 13 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)

django_ticket_15889.patch (1.0 KB ) - added by James Pic 13 years ago.
15889-get_serializer-specific-exception.diff (3.3 KB ) - added by Mathieu Agopian 13 years ago.
patch + tests + docs
15889-get_serializer-specific-exception_2.diff (3.7 KB ) - added by Mathieu Agopian 13 years ago.
Integrated Alex's and Jacob's feedbacks and improvements requests
15889-get_serializer-specific-exception_3.diff (3.5 KB ) - added by Mathieu Agopian 13 years ago.
integrated Alex's new feedbacks

Download all attachments as: .zip

Change History (20)

comment:1 by Alex Gaynor, 13 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted

Let's name the bikeshed SerializerNotFound :)

comment:2 by Roy Smith, 13 years ago

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 by Alex Gaynor, 13 years ago

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

by James Pic, 13 years ago

Attachment: django_ticket_15889.patch added

comment:4 by James Pic, 13 years ago

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

Last edited 13 years ago by James Pic (previous) (diff)

comment:5 by Mathieu Agopian, 13 years ago

Cc: mathieu.agopian@… added

comment:6 by Mathieu Agopian, 13 years ago

Has patch: set
Needs documentation: set
Needs tests: set

comment:7 by Mathieu Agopian, 13 years ago

Patch needs improvement: set

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

by Mathieu Agopian, 13 years ago

patch + tests + docs

comment:8 by Mathieu Agopian, 13 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Type: UncategorizedNew feature

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

comment:9 by Alex Gaynor, 13 years ago

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 by Jacob, 13 years ago

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.

by Mathieu Agopian, 13 years ago

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

comment:11 by Mathieu Agopian, 13 years ago

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 by Alex Gaynor, 13 years ago

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.

by Mathieu Agopian, 13 years ago

integrated Alex's new feedbacks

comment:13 by Mathieu Agopian, 13 years ago

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 by Alex Gaynor, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by Mathieu Agopian, 13 years ago

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

comment:16 by Alex Gaynor, 13 years ago

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top