[patch] Default OPTIONS implementation in base View class

[Steven Cummings]

I wanted to introduce a couple of relatively minor but useful changes to the base View class.

First, I want to add a default options implementation that simply returns an HTTP 204 response with an Allow header, just like that of the 405 METHOD NOT ALLOWED response. This method could easily be overrided to provide additional or different info in the options response.

Second, I noticed that the default head method assumes that a get method exists. I'm removing the default head and instead setting it to simply be the get method, if it exists, in the view method created in as_view. This was apparently the original proposed implementation of head in ticket #15668. It's unclear why it ended up as it currently is, where an AttributeError can occur if a View does not response to GET.

I brought these changes up on this dev list ​post.

Patch forthcoming shortly.

[Steven Cummings]

Claiming

[Aymeric Augustin]

HTTP verbs should be returned in uppercase in the Allow header. Besides this, the patch looks very good.

It reverts r16105, but that's the correct thing to do.

[Steven Cummings]

Replying to aaugustin:

HTTP verbs should be returned in uppercase in the Allow header.

I agree with this, but then for consistency I would want to upper-case the methods returned by http_method_not_allowed as well. Then the question becomes, is this a passivity issue? Anyway, I'll go ahead and upload the updated patch.

[Steven Cummings]

Forgot to unset needs-improvement after the latest patch...

[Claude Paroz]

Small bikeshedding: all HttpResponse subclasses begin with HttpResponse, that's why I'd rather name the class HttpResponseOptions, unless you have a specific reason not to adopt the model.

Then I think it should be mentioned on ​https://docs.djangoproject.com/en/1.3/ref/request-response/#httpresponse-subclasses

[Steven Cummings]

Replying to claudep:

Small bikeshedding: all HttpResponse subclasses begin with HttpResponse, that's why I'd rather name the class HttpResponseOptions, unless you have a specific reason not to adopt the model.

Then I think it should be mentioned on ​https://docs.djangoproject.com/en/1.3/ref/request-response/#httpresponse-subclasses

Both valid points. Updated patch forthcoming.

[Steven Cummings]

Updated patch for improved HEAD, and default OPTIONS

[Steven Cummings]

Doc was added in that last patch.

[Claude Paroz]

Looks good. As a side note, I prefer when you don't replace previous patches by new ones, as it prevents to compare what's changed in the new patch.

[Steven Cummings]

Replying to claudep:

Looks good. As a side note, I prefer when you don't replace previous patches by new ones, as it prevents to compare what's changed in the new patch.

Gotcha. I'll stop doing that then!

[anonymous]

It seems it's better to return a 200 OK response, see ​http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2

[Steven Cummings]

Replying to anonymous:

It seems it's better to return a 200 OK response, see ​http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2

204 seems to make more sense specifically indicating no body and still falling in the range of success statuses. That part of the spec doesn't read (at least to me) as requiring 200. However, looking around a bit at some existing server implementations, I'm seeing all 200s, so I can make that change.

BTW, who's comment was that?

[Steven Cummings]

Updated patch using status code 200 instead of 204

[Jamie Matthews]

How can we push this forward and make sure it gets into 1.4?

The problem seems to be that this ticket covers two issues:

1. the bug in the default HEAD implementation
2. Improving the behaviour of OPTIONS

It seems to me that (1) is a simple (but potentially serious) bug which has been fixed in the attached patches, with no disagreements. (2) appears to be more contentious, and I can't tell from the above discussion if the problems with the patch have been resolved.

Would it help if I moved (1) into a separate issue and split the patch to just include the fix for that? Or is this patch good enough to be checked in as-is?

[Steven Cummings]

Replying to j4mie:

How can we push this forward and make sure it gets into 1.4?

The problem seems to be that this ticket covers two issues:

1. the bug in the default HEAD implementation
2. Improving the behaviour of OPTIONS

It seems to me that (1) is a simple (but potentially serious) bug which has been fixed in the attached patches, with no disagreements. (2) appears to be more contentious, and I can't tell from the above discussion if the problems with the patch have been resolved.

Would it help if I moved (1) into a separate issue and split the patch to just include the fix for that? Or is this patch good enough to be checked in as-is?

I would think that maybe we hadn't landed on (2) as well except that a core-dev marked this bug ready-for-checking. As for how it gets into 1.4, we need one of those core devs to weigh in here.

[Aymeric Augustin]

In [17545]:

Prevented the generic views from automatically creating a HEAD method when there is no GET. Reverts r16105, refs #17449.

[Aymeric Augustin]

Like j4mie said, this ticket covers two different ideas, which is a bit impractical.

I just fixed the bug, the regression in the HEAD implementation.

So we're only left with the feature request: a default OPTIONS implementation. This is a new feature, so I'm re-classifying the ticket, and it will have to wait until 1.4 is released. Since I've only applied parts of the patch, it no longer applies cleanly, so I have to set "patch needs improvement" and bump it back to "Accepted". This is just a procedural matter; I don't see the patch as particularly debatable; it just arrived a bit late in the 1.4 release cycle. I'm interested in this feature, please ping me if it stagnates after the final 1.4 release.

[Jamie Matthews]

Great, thanks aaugustin!

[Steven Cummings]

Replying to aaugustin:

I'm interested in this feature, please ping me if it stagnates after the final 1.4 release.

I'll re-clean the patch soon so I'll be ready to go after 1.4 when core devs hopefully get freed up and something simple like this can just go in. Thanks!

[Steven Cummings]

Removing mention of HEAD bug in summary now that it's fixed per r17545.

[Steven Cummings]

Patch adjusted to only implement default OPTIONS

[Steven Cummings]

Ready to go.

[anonymous]

Would it help if I re-created this as a pull request now that Django is on GitHub?

[Steven Cummings]

Replying to anonymous:

Would it help if I re-created this as a pull request now that Django is on GitHub?

Forgot to login first. That was me :/

[Aymeric Augustin]

Currently, each HttpResponse subclass implements an HTTP response code. To avoid breaking this design, I'm going to replace HttpResponseOptions with a plain HttpResponse. Otherwise the patch looks good, I'm going to commit it in a few minutes. Thanks!

[Aymeric Augustin <aymeric.augustin@…>]

In [c4996df16c58b46844d2e046bca5a3d41dfcc122]:

Fixed #17449 -- Added OPTIONS to generic views.

Thanks estebistec for the report and patch.

[Steven Cummings]

Awesome! Thanks for applying it!