Code

Opened 3 years ago

Closed 3 years ago

#15889 closed New feature (fixed)

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

Reported by: RoySmith 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 jpic 3 years ago.
15889-get_serializer-specific-exception.diff (3.3 KB) - added by magopian 3 years ago.
patch + tests + docs
15889-get_serializer-specific-exception_2.diff (3.7 KB) - added by magopian 3 years ago.
Integrated Alex's and Jacob's feedbacks and improvements requests
15889-get_serializer-specific-exception_3.diff (3.5 KB) - added by magopian 3 years ago.
integrated Alex's new feedbacks

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by Alex

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Let's name the bikeshed SerializerNotFound :)

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by 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.

Changed 3 years ago by jpic

comment:4 Changed 3 years ago by jpic

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

Last edited 3 years ago by jpic (previous) (diff)

comment:5 Changed 3 years ago by magopian

  • Cc mathieu.agopian@… added

comment:6 Changed 3 years ago by magopian

  • Has patch set
  • Needs documentation set
  • Needs tests set

comment:7 Changed 3 years ago by magopian

  • Patch needs improvement set

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

Changed 3 years ago by magopian

patch + tests + docs

comment:8 Changed 3 years ago by magopian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to New feature

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

comment:9 Changed 3 years ago by Alex

  • 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 3 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 3 years ago by magopian

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

comment:11 Changed 3 years ago by magopian

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

Changed 3 years ago by magopian

integrated Alex's new feedbacks

comment:13 Changed 3 years ago by 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 !

comment:14 Changed 3 years ago by Alex

  • Triage Stage changed from Accepted to Ready for checkin

comment:15 Changed 3 years ago by magopian

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

comment:16 Changed 3 years ago by Alex

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.