Following Adrian's advice on Twitter, I've made a proof of concept using OkHttp [1]. The initial tests worked, but need to use their latest SNAPSHOT and will need this patch merged too [2].
If this client works, I'll create a proper driver for it, so the projects that need PATCH over HTTP can use it. That will be a pretty straightforward driver, but would require work, so I don't expect to have it for 1.7.0. I think it is OK to leave the current patch as-is, to at least support HTTPS. Do you agree? [1] http://square.github.io/okhttp/ [2] https://github.com/square/okhttp/pull/368 On 13 December 2013 09:37, Ignasi Barrera <[email protected]> wrote: > TL;DR: Cannot use PATCH requests with a body in HTTP. What do we do for 1.7.0 > ? > > > After some time yesterday trying with Everett to make this work, we > couldn't find a suitable solution. > > The problem is the java.net.HttpURLConnection class and its default > JRE implementation sun.net.www.protocol.http.HttpURLConnection: > > Its "setRequestMethod" fails if the method is not one of the initial > HTTP/1.1 spec, and PATCH is not one of them. To bypass that > limitation, the JavaUrlHttpCommandExecutorService.java tried to > manually set the field using reflection [1] if the "setRequestMethod" > failed. That worked fine, except for HTTPS connections, so we added > this patch [2] to properly set the method field when using HTTPS too. > So far so good. > > The problem comes when getting the output stream to write the payload > of the request [3]. At that point, the default JRE implementation > verifies that the request method is "POST" or "PUT", and fails > otherwise [4] (but only in HTTP, not when using HTTPS!). That makes it > impossible to send PATCH requests with a body. Tonight with Everett, > we tried to workaround this (in the only ugly way we could come up > with) [5], but we got at the point where many ugly workarounds were > required. Furthermore, those workarounds would require coupling our > code to specific implementation details of the > sun.net.www.protocol.http.HttpURLConnection class, which is not good > at all. > > So I think, going this way is not an option, and we won't be able to > use the default JRE HttpUrlConnection to perform PATCH requests. Have > someone else succeeded at this? (Adrian? :D). > > The other suggested options was to use the Apache HTTP Client driver > [6]. I've added it to Everett's branch with the Openstack Marconi APi > that uses the PATCH method, but the results were worse. Almost all > expect tests were failing due to several errors, but the PATCH request > also failed reporting an unsupported HTTP method. > > > At this point, neither using the default JRE implementation is an > option, nor using the apachehc driver. Perhaps we have to build a > proper driver that uses a library that properly supports the modern > HTTP methods, but that would be quite an amount of work, and > definitely not something to be done for 1.7.0. > > > Given this, do you think we should revert the patch [2]? It only > "extends" the reflection stuff to properly set the method in HTTPS > connections, so in practice I think the code could remain. If we keep > the patch, at least we would be able to use PATCH requests with a body > over HTTPS, as the sun.net.www.protocol.http.HttpURLConnection only > fails then the protocol is HTTP. > > > WDYT? > > > Ignasi > > > [1] > https://github.com/jclouds/jclouds/blob/828d8790c2c1e87e4db3a2dd650a8fd2d7f3a790/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L180-L185 > [2] https://github.com/jclouds/jclouds/pull/230 > [3] > https://github.com/jclouds/jclouds/blob/828d8790c2c1e87e4db3a2dd650a8fd2d7f3a790/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L252 > [4] > http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/net/www/protocol/http/HttpURLConnection.java#HttpURLConnection.getOutputStream%28%29 > [5] > https://github.com/nacx/jclouds/commit/926254e8512ecfbd7c1fcd6c0e4dd3ad4b2120ee > [6] https://github.com/jclouds/jclouds/tree/master/drivers/apachehc > > On 12 December 2013 17:57, Ignacio Mulas Viela <[email protected]> wrote: >>> The main reason for this is that there are several apis that (for >>> example) accept PUT requests without a payload (even if that makes >>> little sense, being purists), meaning we should assume not all apis >>> are 100% RESTful. Given this, I'd rather be flexible here, and don't >>> require PATCH requests to have a payload. >> >> Yeah I totally agree that put validation on the payload by default is not a >> good idea as the flexibility is more important in here. >> >> Thanks for the comment Ignasi :) >> >> 2013/12/12 Ignasi Barrera <[email protected]> >> >>> > I tried PATCH annotation and it is not fully working. What I got is that >>> I >>> > can create a HTTP PATCH request if no payload is attached (which doesn't >>> > make much sense since it always need some body to specify what you want >>> to >>> > update >>> >>> The payload is set in the request in this method [1], when parsing the >>> invoked method annotations, reading its parameters and calling the >>> binders. >>> >>> There is currently no validation that requires a payload to be >>> included in POST/PUT methods, and I think we shouldn't validate by >>> default that a payload is present when sending PATCH requests (it is >>> up to the one making the api call to properly build the request). >>> >>> The main reason for this is that there are several apis that (for >>> example) accept PUT requests without a payload (even if that makes >>> little sense, being purists), meaning we should assume not all apis >>> are 100% RESTful. Given this, I'd rather be flexible here, and don't >>> require PATCH requests to have a payload. >>> >>> >>> >>> >>> [1] >>> https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java#L181 >>> >>> >>> On 12 December 2013 16:59, Ignacio Mulas Viela <[email protected]> >>> wrote: >>> > Hi All, >>> > I tried PATCH annotation and it is not fully working. What I got is that >>> I >>> > can create a HTTP PATCH request if no payload is attached (which doesn't >>> > make much sense since it always need some body to specify what you want >>> to >>> > update). So my problem was mainly how allow the PATCH annotation to >>> attach >>> > a body to the request. >>> > I saw that the payload attachment is handled in [1] but could not go into >>> > more details. :) >>> > >>> > I have tried with HTTP but not with HTTPS so I do not know if there is >>> > special extra requirements in there. >>> > >>> > [1] >>> > >>> https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java >>> > >>> > >>> > 2013/12/12 Ignasi Barrera <[email protected]> >>> > >>> >> For the record: https://issues.apache.org/jira/browse/JCLOUDS-405 >>> >> >>> >> On 12 December 2013 16:23, Ignasi Barrera <[email protected]> >>> >> wrote: >>> >> > Verified with the Charles proxy that the fix works. Will send the PR >>> >> > as soon as tests finish :) >>> >> > >>> >> > On 12 December 2013 15:58, Ignasi Barrera <[email protected]> >>> >> wrote: >>> >> >> Trying it right now... Will send a PR if it works >>> >> >> >>> >> >> On 12 December 2013 15:36, Ignasi Barrera <[email protected]> >>> >> wrote: >>> >> >>> I found this comment [1] in the Jersey issue, explaining that in SSL >>> >> >>> there is an HTTPS URL connection wrapping the actual >>> HttpURLConnection >>> >> >>> where the method should be set. >>> >> >>> Could you do a quick test and add the appropriate bits from the code >>> >> >>> in the Jersey issue and see if that does the trick for HTTPS >>> >> >>> connections? >>> >> >>> >>> >> >>> >>> >> >>> [1] >>> >> >>> https://java.net/jira/browse/JERSEY-639?focusedCommentId=370981&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_370981 >>> >> >>> >>> >> >>> On 12 December 2013 15:30, Everett Toews < >>> [email protected]> >>> >> wrote: >>> >> >>>> Yes. The request is going to >>> >> https://ord.queues.api.rackspacecloud.com. >>> >> >>>> >>> >> >>>> There's some special handling for HTTPS [1] but the part for >>> setting >>> >> the request method seems to be common for both HTTP and HTTPS. >>> >> >>>> >>> >> >>>> What did you have in mind? >>> >> >>>> >>> >> >>>> Everett >>> >> >>>> >>> >> >>>> [1] >>> >> >>> https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L158 >>> >> >>>> >>> >> >>>> >>> >> >>>> On Dec 12, 2013, at 2:29 AM, Ignasi Barrera wrote: >>> >> >>>> >>> >> >>>>> This is strange. The reflection fix is the same used in the Jersey >>> >> >>>>> client [1], and it is supposed to work. >>> >> >>>>> Looking at the jclouds code, there is no special handling for >>> HTTPS >>> >> >>>>> connections. Everett, are you using an HTTPS one? >>> >> >>>>> >>> >> >>>>> >>> >> >>>>> >>> >> >>>>> [1] https://java.net/jira/browse/JERSEY-639 >>> >> >>>>> >>> >> >>>>> On 12 December 2013 09:09, Andrew Phillips <[email protected] >>> > >>> >> wrote: >>> >> >>>>>>> I know that HttpURLConnection.setRequest() [2] doesn't allow >>> >> PATCH. I >>> >> >>>>>>> also know that we work around this in jclouds by setting the >>> field >>> >> by >>> >> >>>>>>> reflection [3]. >>> >> >>>>>> >>> >> >>>>>> >>> >> >>>>>> Have you been able to try this with the apachehc [1] driver? Does >>> >> that make >>> >> >>>>>> any difference? >>> >> >>>>>> >>> >> >>>>>> ap >>> >> >>>>>> >>> >> >>>>>> [1] >>> https://github.com/jclouds/jclouds/tree/master/drivers/apachehc >>> >> >>>> >>> >> >>>
