Opened 5 years ago

Closed 5 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:

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 5 years ago.
15889-get_serializer-specific-exception.diff (3.3 KB) - added by Mathieu Agopian 5 years ago.
patch + tests + docs
15889-get_serializer-specific-exception_2.diff (3.7 KB) - added by Mathieu Agopian 5 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 5 years ago.
integrated Alex's new feedbacks

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Alex Gaynor

Easy pickings: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Let's name the bikeshed SerializerNotFound :)

comment:2 Changed 5 years ago by Roy Smith

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

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 5 years ago by James Pic

Attachment: django_ticket_15889.patch added

comment:4 Changed 5 years ago by James Pic

I uploaded a trivial patch ... I'm unsure how/where to test it.

Version 0, edited 5 years ago by James Pic (next)

comment:5 Changed 5 years ago by Mathieu Agopian

Cc: mathieu.agopian@… added

comment:6 Changed 5 years ago by Mathieu Agopian

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

comment:7 Changed 5 years ago by Mathieu Agopian

Patch needs improvement: set

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

Changed 5 years ago by Mathieu Agopian

patch + tests + docs

comment:8 Changed 5 years ago by Mathieu Agopian

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 Changed 5 years ago by Alex Gaynor

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 5 years ago by 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.

Changed 5 years ago by Mathieu Agopian

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

comment:11 Changed 5 years ago by Mathieu Agopian

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

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 5 years ago by Mathieu Agopian

integrated Alex's new feedbacks

comment:13 Changed 5 years ago by Mathieu Agopian

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

Triage Stage: AcceptedReady for checkin

comment:15 Changed 5 years ago by Mathieu Agopian

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

comment:16 Changed 5 years ago by Alex Gaynor

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