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 >> >> >>>> >> >> >>
