Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#17449 closed New feature (fixed)

[patch] Default OPTIONS implementation in base View class

Reported by: Steven Cummings Owned by: Aymeric Augustin
Component: Generic views Version: 1.3
Severity: Normal Keywords:
Cc: jamie.matthews@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (3)

patch17449.diff (5.3 KB ) - added by Steven Cummings 12 years ago.
Updated patch for improved HEAD, and default OPTIONS
patch17449-status200.diff (5.3 KB ) - added by Steven Cummings 12 years ago.
Updated patch using status code 200 instead of 204
ticket-17449-default-options.patch (4.8 KB ) - added by Steven Cummings 12 years ago.
Patch adjusted to only implement default OPTIONS

Download all attachments as: .zip

Change History (30)

comment:1 by Steven Cummings, 12 years ago

Owner: changed from nobody to Steven Cummings

Claiming

comment:2 by Steven Cummings, 12 years ago

Has patch: set

comment:3 by Aymeric Augustin, 12 years ago

Component: HTTP handlingGeneric views
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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.

comment:4 by Steven Cummings, 12 years ago

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.

comment:5 by Jamie Matthews, 12 years ago

Cc: jamie.matthews@… added

comment:6 by Steven Cummings, 12 years ago

Patch needs improvement: unset

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

comment:7 by Claude Paroz, 12 years ago

Needs documentation: set

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

in reply to:  7 comment:8 by Steven Cummings, 12 years ago

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.

by Steven Cummings, 12 years ago

Attachment: patch17449.diff added

Updated patch for improved HEAD, and default OPTIONS

comment:9 by Steven Cummings, 12 years ago

Needs documentation: unset

Doc was added in that last patch.

comment:10 by Claude Paroz, 12 years ago

Triage Stage: AcceptedReady for checkin

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.

in reply to:  10 comment:11 by Steven Cummings, 12 years ago

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!

comment:12 by anonymous, 12 years ago

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

in reply to:  12 comment:13 by Steven Cummings, 12 years ago

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?

by Steven Cummings, 12 years ago

Attachment: patch17449-status200.diff added

Updated patch using status code 200 instead of 204

comment:14 by Jamie Matthews, 12 years ago

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?

in reply to:  14 comment:15 by Steven Cummings, 12 years ago

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-checkin. As for how it gets into 1.4, we need one of those core devs to weigh in here.

Last edited 12 years ago by Steven Cummings (previous) (diff)

comment:16 by Aymeric Augustin, 12 years ago

In [17545]:

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

comment:17 by Aymeric Augustin, 12 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted
Type: BugNew feature

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.

comment:18 by Jamie Matthews, 12 years ago

Great, thanks aaugustin!

in reply to:  17 comment:19 by Steven Cummings, 12 years ago

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!

comment:20 by Steven Cummings, 12 years ago

Summary: [patch] Default OPTIONS and improved HEAD in base View class[patch] Default OPTIONS implementation in base View class

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

by Steven Cummings, 12 years ago

Patch adjusted to only implement default OPTIONS

comment:21 by Steven Cummings, 12 years ago

Patch needs improvement: unset

Ready to go.

comment:22 by Aymeric Augustin, 12 years ago

Owner: changed from Steven Cummings to Aymeric Augustin

comment:23 by anonymous, 12 years ago

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

in reply to:  23 comment:24 by Steven Cummings, 12 years ago

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

comment:25 by Aymeric Augustin, 12 years ago

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!

comment:26 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [c4996df16c58b46844d2e046bca5a3d41dfcc122]:

Fixed #17449 -- Added OPTIONS to generic views.

Thanks estebistec for the report and patch.

comment:27 by Steven Cummings, 12 years ago

Awesome! Thanks for applying it!

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