Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#17449 closed New feature (fixed)

[patch] Default OPTIONS implementation in base View class

Reported by: estebistec Owned by: aaugustin
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 estebistec 2 years ago.
Updated patch for improved HEAD, and default OPTIONS
patch17449-status200.diff (5.3 KB) - added by estebistec 2 years ago.
Updated patch using status code 200 instead of 204
ticket-17449-default-options.patch (4.8 KB) - added by estebistec 2 years ago.
Patch adjusted to only implement default OPTIONS

Download all attachments as: .zip

Change History (30)

comment:1 Changed 2 years ago by estebistec

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to estebistec
  • Patch needs improvement unset

Claiming

comment:2 Changed 2 years ago by estebistec

  • Has patch set

comment:3 Changed 2 years ago by aaugustin

  • Component changed from HTTP handling to Generic views
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

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 Changed 2 years ago by estebistec

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 Changed 2 years ago by j4mie

  • Cc jamie.matthews@… added

comment:6 Changed 2 years ago by estebistec

  • Patch needs improvement unset

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

comment:7 follow-up: Changed 2 years ago by claudep

  • 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

comment:8 in reply to: ↑ 7 Changed 2 years ago by estebistec

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.

Changed 2 years ago by estebistec

Updated patch for improved HEAD, and default OPTIONS

comment:9 Changed 2 years ago by estebistec

  • Needs documentation unset

Doc was added in that last patch.

comment:10 follow-up: Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready 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.

comment:11 in reply to: ↑ 10 Changed 2 years ago by estebistec

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 follow-up: Changed 2 years ago by anonymous

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

comment:13 in reply to: ↑ 12 Changed 2 years ago by estebistec

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?

Changed 2 years ago by estebistec

Updated patch using status code 200 instead of 204

comment:14 follow-up: Changed 2 years ago by 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?

comment:15 in reply to: ↑ 14 Changed 2 years ago by estebistec

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 2 years ago by estebistec (previous) (diff)

comment:16 Changed 2 years ago by aaugustin

In [17545]:

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

comment:17 follow-up: Changed 2 years ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted
  • Type changed from Bug to New 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 Changed 2 years ago by j4mie

Great, thanks aaugustin!

comment:19 in reply to: ↑ 17 Changed 2 years ago by estebistec

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 Changed 2 years ago by estebistec

  • Summary changed from [patch] Default OPTIONS and improved HEAD in base View class to [patch] Default OPTIONS implementation in base View class

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

Changed 2 years ago by estebistec

Patch adjusted to only implement default OPTIONS

comment:21 Changed 2 years ago by estebistec

  • Patch needs improvement unset

Ready to go.

comment:22 Changed 2 years ago by aaugustin

  • Owner changed from estebistec to aaugustin

comment:23 follow-up: Changed 2 years ago by anonymous

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

comment:24 in reply to: ↑ 23 Changed 2 years ago by estebistec

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 Changed 2 years ago by aaugustin

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 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

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

In [c4996df16c58b46844d2e046bca5a3d41dfcc122]:

Fixed #17449 -- Added OPTIONS to generic views.

Thanks estebistec for the report and patch.

comment:27 Changed 2 years ago by estebistec

Awesome! Thanks for applying it!

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.