edicl / drakma

HTTP client written in Common Lisp
http://edicl.github.io/drakma/
249 stars 58 forks source link

Possible to make Drakma less picky on put vs. post? #130

Closed mdbergmann closed 1 year ago

mdbergmann commented 1 year ago

I have a web service that actually wants a multipart-form via PUT. Since the service creates a new resource it should actually be POST. So this is not correctly implemented on the server side, but Drakma doesn't allow me to specify PUT when a request contains form-data.

However, also a PUT request should accept form-data when a resource should just be updated on the server side. So specifying both POST and PUT with form-parameters is fully valid.

Could we just remove this:

    (when (and file-parameters-p (not (eq method :post)))
      (parameter-error "Don't know how to handle parameters in ~S, as this is not a POST request."
                       parameters))
phoe commented 1 year ago

In order to preserve legacy error-signaling behavior, I'd suggest a parameter-cerror so a handler can explicitly #'continue to send non-standard HTTP.

phoe commented 1 year ago

Would you mind submitting a PR with a unit test case for this?

mdbergmann commented 1 year ago

Yep, can do. Just wanted to check if this were accepted as a change.

phoe commented 1 year ago

I'd be OK with merging this since invalid HTTP requests (in particular, form data in non-POST requests) are something that I know has been occurring in the wild and they may form a valid use case to #'continue from.

Unless someone else steps in and says it's a bad idea for some other reason, I'd accept a PR that cerrors this particular case and allows the user to roll with their choice.

mdbergmann commented 1 year ago

Not entirely sure what you meant with #’continue. To allow PUT for x-www-urlencoded and multipart form-data basically just two predicates had to be changes to allow the put to be processed in the same way as a post. On hunchentoot side one can actually add PUT to the *methods-for-post-parameters* to have a put be handled as a post. But please see PR: #131

phoe commented 1 year ago

Not entirely sure what you meant with #’continue.

I thought of signaling a continuable error instead of a normal one, but I see what your PR does. I have no idea if allowing this new behavior "by default" would be a welcome change, because it would change legacy behavior; adding a new continue restart to an already existing error would preserve legacy behavior but also allow users to opt out of it and going into the non-standard territory.

mdbergmann commented 1 year ago

OK, I think I know what you mean. The user should use handler-bind and auto-choose a restart that continues. Alright, let me play a bit with this. I fear though that this will make the already quite unreadable http-request function more unreadable.

hanshuebner commented 1 year ago

I'd rather not overcomplicate things. Using a restart seems quite heavy handed. Making PUT behave like POST may not be welcome to everyone and somewhat against the spec, but I think we can be liberal in what we accept here.

mdbergmann commented 1 year ago

OK, so we can keep the 'fix' as is?

hanshuebner commented 1 year ago

OK, so we can keep the 'fix' as is?

In my opinion, yes. @phoe can you take it from here?

phoe commented 1 year ago

Sure - let me review and try merging.

phoe commented 1 year ago

Merged, thanks.

Are the differences since 2.0.9 enough to warrant a new release, so that Quicklisp can pull the new version?