Skip to content

Streamline and unmagicify view.DATA / view.CONTENT #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
flashingpumpkin opened this issue Feb 10, 2012 · 8 comments
Closed

Streamline and unmagicify view.DATA / view.CONTENT #158

flashingpumpkin opened this issue Feb 10, 2012 · 8 comments

Comments

@flashingpumpkin
Copy link
Contributor

Hi

I find it awkward that the DATA attribute on views does not provide a consistent interface to request data, both in the request body and as url query params.

Sample code:

class OAuth2Authentication(BaseAuthentication):
    def authenticate(self, request):
        if request.method.lower() in ["get", "delete"]:
            form = OAuthTokenForm(request.GET)
        else:
            form = OAuthTokenForm(self.view.DATA)
        if form.is_valid():
            return form.cleaned_data.get('oauth_token').user

Even though it is discouraged in the Django core, I'd much rather have something along the lines of the REQUEST attribute that consistantly returns all the data, no matter from where. The current DATA documentation is misleading in that it suggests that it will also return GET and DELETE data, which is not the case.

Also - I find it awkward that DATA and CONTENT are properties and would strongly prefer methods like the following on the views:

def get_(raw_)data(self):
    """ Returns uncleaned data straight from the parser """

def clean / is_valid / full_clean / clean_data(self):
    """ Basically mimic the form api to clean the raw data and create a `cleaned_data` attribute """

Maybe something to think about while @sebpiq is doing all of the backwards incompatible refactoring work on his mainline.

You'll lose one or two more lines of code to the views dealing with data but gain explicitness.

@tomchristie
Copy link
Member

I find it awkward that the DATA attribute on views does not provide a consistent interface to request data, both in the request body and as url query params.

I think it will make a lot more sense once it's request.DATA / request.FILES, not view.DATA / view.FILES.
And, yes, it needs better documentation.
I'd strongly argue against including url parameters in .DATA for get and delete requests.
Right now it does a very specific thing - return the content of the request body.

I think it's more an issue of:

  1. It needs to be on the request, not on the view.
  2. It needs better, more explicit documentation.

Also - I find it awkward that DATA and CONTENT are properties and would strongly prefer methods like the following...

I totally agree with CONTENT, and yup, mimicking the forms API seems like a better approach.

@sebpiq
Copy link
Contributor

sebpiq commented Feb 10, 2012

I totally agree with the clean stuff - forms API-like, instead of .CONTENT. But I guess it'll come after.

@flashingpumpkin
Copy link
Contributor Author

Cool. I've also just now spotted some changes in #141 that address some points already, so I might just be lagging behind. (+1 on putting the data on the request ;))

I'll have a look if I can fork off @sebpiq's changes and add validation reflecting the forms API soonish.

Doing one specific thing is good... What about collapsing delete and get params into another single attribute on the request?

@flashingpumpkin
Copy link
Contributor Author

Well, they are already on the same attribute. Guess what I'd wondered about is what about adding something along the lines of request.has_query() to check if there are any query parameters.

@tomchristie
Copy link
Member

Doing one specific thing is good... What about collapsing delete and get params into another single attribute on the request?

Possibly, possibly, possibly.

I'm not convinced, because I think it conflates two totally different aspects of the HTTP request. (I'm not convinced that request.REQUEST should be in Django core.)

If we do go down that route, tho, using request.REQUEST might be the thing to do. It'd still have the same behavior for form data (assuming you're using the form parser), but it'd support other content types too.

@tomchristie
Copy link
Member

Guess what I'd wondered about is what about adding something along the lines of request.has_query() to check if there are any query parameters.

Isn't that just the same as doing if request.GET?

@flashingpumpkin
Copy link
Contributor Author

Isn't that just the same as doing if request.GET?

Essentially yes. But it leaves ambiguity - one might think that it's only checking for get requests. Semantics at play, but shouldn't matter if documented and reasonably experienced with Django.

@tomchristie
Copy link
Member

REST framework 2.0 uses request.DATA and request.QUERY_PARAMS.
Request parsing is also much simplified as per issue title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants