weakening http method constraint to allow PUT or POST for uploads. #24

Open
wants to merge 31 commits into
from

Conversation

Projects
None yet
8 participants
@talg

talg commented Aug 18, 2011

I needed to support upload acceleration for an existing system that used PUT to send files. weakening the upload method check seemed like the cleanest way to go. notice a few other folks have requested this feature as well.

vkholodkov and others added some commits Jul 21, 2008

Fixed bug: missing CRLF at the end of resulting body; Added feature: …
…limiting file size and resulting body size via upload_max_file_size and upload_max_output_body_len.
@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Feb 10, 2012

any chances in seeing this one merged?

mvrhov commented Feb 10, 2012

any chances in seeing this one merged?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov Feb 10, 2012

Collaborator

Could you make it configurable with PUTs off by default?

Collaborator

vkholodkov commented Feb 10, 2012

Could you make it configurable with PUTs off by default?

@talg

This comment has been minimized.

Show comment
Hide comment
@talg

talg Feb 10, 2012

I'm open to that.

Can you explain your thoughts around why this is would be the way to go vs. just allowing PUT or POST?

talg commented Feb 10, 2012

I'm open to that.

Can you explain your thoughts around why this is would be the way to go vs. just allowing PUT or POST?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov Feb 11, 2012

Collaborator

Because it does not comply with standarts.

Collaborator

vkholodkov commented Feb 11, 2012

Because it does not comply with standarts.

@talg

This comment has been minimized.

Show comment
Hide comment
@talg

talg Feb 13, 2012

I can make it configurable if it will make it into the lastest version.

I notice that what is currently in the git repo is way out of date with the latest version... whats the deal?

talg commented Feb 13, 2012

I can make it configurable if it will make it into the lastest version.

I notice that what is currently in the git repo is way out of date with the latest version... whats the deal?

@edevil

This comment has been minimized.

Show comment
Hide comment
@edevil

edevil Mar 6, 2012

Has it been merged?

edevil commented Mar 6, 2012

Has it been merged?

@d

This comment has been minimized.

Show comment
Hide comment
@d

d Sep 12, 2012

@vkholodkov which part of http standards isn't PUT request compliant with?

d commented Sep 12, 2012

@vkholodkov which part of http standards isn't PUT request compliant with?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov Sep 13, 2012

Collaborator

You phrase it incorrectly. PUT request is compliant with HTTP standard, but simply changing POST to PUT makes the module violate the definition of PUT method: "The PUT method requests that the enclosed entity be stored under the supplied Request-URI." In other words, if a PUT request is made for certain Request-URI, then the client expects this resource be available under specified Request-URI, while for POST it doesn't. Request-URI in POST request is just a name of an entry point.

Collaborator

vkholodkov commented Sep 13, 2012

You phrase it incorrectly. PUT request is compliant with HTTP standard, but simply changing POST to PUT makes the module violate the definition of PUT method: "The PUT method requests that the enclosed entity be stored under the supplied Request-URI." In other words, if a PUT request is made for certain Request-URI, then the client expects this resource be available under specified Request-URI, while for POST it doesn't. Request-URI in POST request is just a name of an entry point.

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov Sep 13, 2012

Collaborator

Sorry for being so uncooperative -- lot of things going on

Collaborator

vkholodkov commented Sep 13, 2012

Sorry for being so uncooperative -- lot of things going on

@wbond

This comment has been minimized.

Show comment
Hide comment
@wbond

wbond May 30, 2013

@vkholodkov So in terms of PUT requiring that a resource be available under a URL, there is no reason a GET request to the same URL would not return the file, right?

I'm having a hard time understanding why supporting PUT and PATCH (without a configuration) would not be a logical choice? I mean someone could choose to implement them in a non-pure-HTTP way, but is that really what the upload module should be worried about? Isn't that the job of the developer and their nginx config?

wbond commented May 30, 2013

@vkholodkov So in terms of PUT requiring that a resource be available under a URL, there is no reason a GET request to the same URL would not return the file, right?

I'm having a hard time understanding why supporting PUT and PATCH (without a configuration) would not be a logical choice? I mean someone could choose to implement them in a non-pure-HTTP way, but is that really what the upload module should be worried about? Isn't that the job of the developer and their nginx config?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov May 30, 2013

Collaborator

@wbond PUT is implemented in the way it is supposed to be implemented in DAV module. Of course, I can reimplement PUT in upload module and grab people's attention:) But it doesn't seem to be worth it...

It's not that GET to the same URL would not return the same file, but for PUT Request-URI identifies the resource being upload and for POST it is just an entry point.

Collaborator

vkholodkov commented May 30, 2013

@wbond PUT is implemented in the way it is supposed to be implemented in DAV module. Of course, I can reimplement PUT in upload module and grab people's attention:) But it doesn't seem to be worth it...

It's not that GET to the same URL would not return the same file, but for PUT Request-URI identifies the resource being upload and for POST it is just an entry point.

@wbond

This comment has been minimized.

Show comment
Hide comment
@wbond

wbond May 30, 2013

Hmm, so perhaps you are only considering the situation where the upload module is handling just a file upload?

For instance, with our usage, we have a REST API accepting a "File" record that contains many fields, one of which is an actual file upload. So when the user wants to update some of the fields in this "File" record they can PATCH if they want to change just a few specific fields.

So unless I am misunderstanding something, in our situation, PUT and PATCH support makes sense. The Request-URI we are PUTing to is for a resource that is more than just the file contents.

Does this make sense?

wbond commented May 30, 2013

Hmm, so perhaps you are only considering the situation where the upload module is handling just a file upload?

For instance, with our usage, we have a REST API accepting a "File" record that contains many fields, one of which is an actual file upload. So when the user wants to update some of the fields in this "File" record they can PATCH if they want to change just a few specific fields.

So unless I am misunderstanding something, in our situation, PUT and PATCH support makes sense. The Request-URI we are PUTing to is for a resource that is more than just the file contents.

Does this make sense?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov May 30, 2013

Collaborator

I must admit, I don't understand you. This could be because you have some unusual setup or because of some other reason. Provide more context if you want your questions answered.

Collaborator

vkholodkov commented May 30, 2013

I must admit, I don't understand you. This could be because you have some unusual setup or because of some other reason. Provide more context if you want your questions answered.

@wbond

This comment has been minimized.

Show comment
Hide comment
@wbond

wbond May 30, 2013

Ok, so we have a database table that looks like this:

create table File (
    id   INT,
    url  VARCHAR,
    mime_type VARCHAR,
    size INT,
    description VARCHAR
)

Then we have a JSON REST API that allows interacting with it, with URLs such as:

POST /files
GET /files/1
PUT /files/1
PATCH /files/1
DELETE /files/1

Now, when a new File record is created, we use POST to send the file contents and the nginx upload module works awesome (thank you!).

However, when we want to update the File record with new file contents, we want to do it via PUT or PATCH. However, we can't use the upload module since it restricts functionality to just POST.

Let me know if anything is still unclear.

wbond commented May 30, 2013

Ok, so we have a database table that looks like this:

create table File (
    id   INT,
    url  VARCHAR,
    mime_type VARCHAR,
    size INT,
    description VARCHAR
)

Then we have a JSON REST API that allows interacting with it, with URLs such as:

POST /files
GET /files/1
PUT /files/1
PATCH /files/1
DELETE /files/1

Now, when a new File record is created, we use POST to send the file contents and the nginx upload module works awesome (thank you!).

However, when we want to update the File record with new file contents, we want to do it via PUT or PATCH. However, we can't use the upload module since it restricts functionality to just POST.

Let me know if anything is still unclear.

@wbond

This comment has been minimized.

Show comment
Hide comment
@wbond

wbond Jun 11, 2013

@vkholodkov Does this make sense now?

wbond commented Jun 11, 2013

@vkholodkov Does this make sense now?

@vkholodkov

This comment has been minimized.

Show comment
Hide comment
@vkholodkov

vkholodkov Jun 11, 2013

Collaborator

It does. However, upload module simply does not implement PUT and PATCH. That's it.

But you can extend it if you want.

Collaborator

vkholodkov commented Jun 11, 2013

It does. However, upload module simply does not implement PUT and PATCH. That's it.

But you can extend it if you want.

@wbond

This comment has been minimized.

Show comment
Hide comment
@wbond

wbond Jun 11, 2013

No worries.

I have started maintaining a fork of the nginx-upload-upload module that allows it to function in an environment where GET, OPTIONS, PUT and PATCH methods are all used. For anyone interested, they can follow along at https://github.com/veracross/nginx-upload-module.

wbond commented Jun 11, 2013

No worries.

I have started maintaining a fork of the nginx-upload-upload module that allows it to function in an environment where GET, OPTIONS, PUT and PATCH methods are all used. For anyone interested, they can follow along at https://github.com/veracross/nginx-upload-module.

@gencer

This comment has been minimized.

Show comment
Hide comment
@gencer

gencer Sep 8, 2016

@wbond your repo gives 405 either. (Itried with my own environment and fresh install and config). No way to over 405 error.

gencer commented Sep 8, 2016

@wbond your repo gives 405 either. (Itried with my own environment and fresh install and config). No way to over 405 error.

@khavishbhundoo

This comment has been minimized.

Show comment
Hide comment
@khavishbhundoo

khavishbhundoo Apr 2, 2018

Contributor

@fdintino

I believe this pull request can be closed as it won't be relevant to the latest version.It could be made a feature request if it is still needed.

Contributor

khavishbhundoo commented Apr 2, 2018

@fdintino

I believe this pull request can be closed as it won't be relevant to the latest version.It could be made a feature request if it is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment