You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Chaithanya Ganta <no...@github.com> on 2017/07/17 11:24:51 UTC

[jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Without request body, there's no point in asking for 100-continue.

With [JavaUrlHttpCommandExecutor](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java), HttpUrlConnection is throwing an [IOException](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L96) if we expect 100-continue for an PUT/POST request with empty body although it is returning 201 response code, it is resulting in another network call when we do [getHeaderFields](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L113) on the connection.

This issue can be easily replicated by invoking putBlob operation with zero-length blob.
```
ByteSource payload = ByteSource.empty();
Blob blob = blobStore.blobBuilder(blobName)
            .payload(payload)
            .contentLength(payload.size())
            .build();
blobStore.putBlob(containerName, blob);
```
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1120

-- Commit Summary --

  * Remove Expect header for PUT and POST reqs with content-length=0

-- File Changes --

    M apis/s3/src/test/java/org/jclouds/s3/S3ClientMockTest.java (2)
    M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (16)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1120.patch
https://github.com/jclouds/jclouds/pull/1120.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) {
       if (request.getPayload() != null) {
          contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), headers);
       }
+
+      boolean isEmptyBody = false;
+      if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
+         if (request.getPayload() != null) {
+            Long length = request.getPayload().getContentMetadata().getContentLength();
+            if (length != null && length == 0) {
+               isEmptyBody = true;
+            }
+         } else {
+            isEmptyBody = true;
+         }
+      }
+      if (isEmptyBody) {
+         request = request.toBuilder().removeHeader("Expect").build();
+      }

@nacx Refactored the code to a separate method and added a unit test case to check the functionality. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127988381

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@ChaithanyaGK pushed 1 commit.

467eecc  Refactored the code and added a unit test


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/e12e21821dd3c54bdbae0954216d7d0a1f8514d9..467eeccb71b1e041301b0b4d720c6b22ec37a401

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -73,7 +73,7 @@ public void testZeroLengthPutHasContentLengthHeader() throws IOException, Interr
       RecordedRequest request = server.takeRequest();
       assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
       assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-      assertEquals(request.getHeaders(EXPECT), ImmutableList.of("100-continue"));
+      assert request.getHeaders(EXPECT).isEmpty();

Done

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127988452

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
> > could you explain the motivation for this change? I don't understand what this addresses except optimizing the rare case of a zero-length PUT.
> 
> @andrewgaul Every time I initiate a multipart upload to azure using S3Proxy it creates an empty blob, where I've encountered this issue.

So you want to optimize a single round-trip out of a set of thousands or millions with large multi-part uploads?  Seems like a curious pursuit...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316674058

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

@nacx jclouds don't set the content length in case of `chunked` transfer, that is the reason I'm not considering the body to be empty if content length is not set. Please check [this ](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/http/HttpUtils.java#L242)


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128180992

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
Thanks for investigating the failure which I addressed in 9e73bbec16cbf91c5f228d4c3ae99c6c526081f8.  You will need to rebase your commit since I included your integration tests to validate my fix.  I guess this is fine to merge although I still do not understand why you are so motivated to optimize an uncommon path.  I have asked this twice before and did not received an answer -- which benchmark do you have which shows that this is a performance issue?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320532517

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
> As such, unless we can verify that those conditions no longer exist, I'd be very hesitant to changing more than is absolutely necessary here to get whatever is currently broken working.

@ChaithanyaGK could you explain the motivation for this change?  I don't understand what this addresses except optimizing the uncommon case of a zero-length PUT.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316463817

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
> could you explain the motivation for this change? I don't understand what this addresses except optimizing the rare case of a zero-length PUT.

@andrewgaul  Every time I initiate a multipart upload to azure using S3Proxy it creates an [empty blob](https://github.com/andrewgaul/s3proxy/blob/master/src/main/java/org/gaul/s3proxy/S3ProxyHandler.java#L1905), where I've encountered this issue.

> Please open an JIRA issue and include the issue in the commit.

Updated the PR with JIRA issue-id

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316614770

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) {
       if (request.getPayload() != null) {
          contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), headers);
       }
+
+      boolean isEmptyBody = false;
+      if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {

@nacx Mostly, I came across Expect header being set for either POST or PUT request. But, I couldn't find any mention of that Expect header is restricted to some Http methods. I think we can remove this check altogether. WDYT ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127745941

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
To clear up any confusion, the proposed change introduces a regression in google-cloud-storage and must be addressed before merging.  Previously zero-byte writes succeeded and with this pull request they fail.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320527190

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@andrewgaul  Currently, I don't have any performance numbers with me. As I have mentioned before, we were comparing the no. of requests being made for a multi-part upload using S3Proxy vs using Azure directly, where I've encountered this redundant request and raised the PR to fix the same.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320612582

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

This version also makes it explicit to set the body to `empty` if the `content-length` is `null`. It is unlikely and I'd say jclouds takes care of always setting the content length for payloads with content, but in any case, I think it is more correct (unlike the current implementation) to mark the body as empty if there content-length is not present

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128165640

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
andrewgaul requested changes on this pull request.

The change looks good and I successfully ran integration tests against atmos, aws-s3, azureblob, b2, filesystem, cloudfiles-us, dreamobjects, and transient.  However, google-cloud-storage fails with:

```
testPutZeroLengthInputStream(org.jclouds.googlecloudstorage.blobstore.integration.GoogleCloudStorageBlobIntegrationLiveTest)  Time elapsed: 11.748 sec  <<< FAILURE!
org.jclouds.http.HttpResponseException: command: POST https://www.googleapis.com/storage/v1/b/gaul-blobstore3/o/multipart-upload/compose HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content: [{
 "error": {
  "errors": [
   {
    "domain": "global",
    "reason": "required",
    "message": "You must provide at least one source component."
   }
  ],
  "code": 400,
  "message": "You must provide at least one source component."
 }
}
]
```

This is due to the hacky GCS `InputStream` support added in [JCLOUDS-912](https://issues.apache.org/jira/browse/JCLOUDS-912).  Please investigate this.

I still do not understand the motivation for this change.  It poses high risk and offers low reward to optimize a corner case that should comprise less than one percentage point in run-time.  Can you expand on your use case so I can understand how you measured this as a hot spot?

> @@ -1559,6 +1559,17 @@ protected void configure() {
       assertEquals(request.getFilters().get(1).getClass(), ConnectionCloseHeader.class);
    }
 
+   @Test
+   public void testZeroContentStripExpectHeader() {
+      Invokable<?, ?> method = method(TestRequestFilter.class, "post");
+      Invocation invocation = Invocation.create(method,
+              ImmutableList.<Object>of(HttpRequest.builder().method("POST").endpoint("http://localhost")
+                      .payload(new byte[]{}).addHeader(HttpHeaders.EXPECT, "100-Continue")
+                      .build()));
+      GeneratedHttpRequest request = processor.apply(invocation);
+      assertEquals(request.getFirstHeaderOrNull(HttpHeaders.EXPECT), null);

Prefer `assertThat(request.getFirstHeaderOrNull(HttpHeaders.EXPECT)).isNull()`.

> @@ -1559,6 +1559,17 @@ protected void configure() {
       assertEquals(request.getFilters().get(1).getClass(), ConnectionCloseHeader.class);
    }
 
+   @Test
+   public void testZeroContentStripExpectHeader() {
+      Invokable<?, ?> method = method(TestRequestFilter.class, "post");
+      Invocation invocation = Invocation.create(method,
+              ImmutableList.<Object>of(HttpRequest.builder().method("POST").endpoint("http://localhost")
+                      .payload(new byte[]{}).addHeader(HttpHeaders.EXPECT, "100-Continue")

Prefer `new byte[0]`.

> @@ -624,6 +625,28 @@ public void testPutInputStream() throws Exception {
    }
 
    @Test(groups = { "integration", "live" })
+   public void testPutZeroLengthString() throws Exception {

Remove as `testPutZeroLengthByteSource` duplicates this.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-53941848

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.



> @@ -73,7 +74,7 @@ public void testZeroLengthPutHasContentLengthHeader() throws IOException, Interr
       RecordedRequest request = server.takeRequest();
       assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
       assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-      assertEquals(request.getHeaders(EXPECT), ImmutableList.of("100-continue"));
+      assertThat(request.getHeaders(EXPECT).isEmpty());

This is written incorrectly.  Instead:

```java
assertThat(request.getHeaders(EXPECT)).isEmpty();
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50986305

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
Please open an [JIRA issue](https://issues.apache.org/jira/projects/JCLOUDS) and include the issue in the commit.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316463065

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Andrew Phillips <no...@github.com>.
I recall a lot of discussion and trial and error in the past trying to get this to work across providers and versions - from what I recall, there were API bugs to work around etc. etc.

As such, unless we can verify that those conditions no longer exist, I'd be very hesitant to changing more than is absolutely necessary here to get whatever is currently broken working.

@andrewgaul @nacx Do either of you two happen to recall some more details? I'll also go looking a bit through the archives...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-315925669

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Andrew Gaul <no...@github.com>.
andrewgaul requested changes on this pull request.

Which existing integration test exercises this code path?  If none exists, please add one.  Also post the results from some real providers.

> @@ -73,7 +73,7 @@ public void testZeroLengthPutHasContentLengthHeader() throws IOException, Interr
       RecordedRequest request = server.takeRequest();
       assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
       assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-      assertEquals(request.getHeaders(EXPECT), ImmutableList.of("100-continue"));
+      assert request.getHeaders(EXPECT).isEmpty();

Call `assertThat(request.getHeaders(EXPECT).isEmpty())`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50510182

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Andrew Phillips <no...@github.com>.
> I'll also go looking a bit through the archives...

Here's a few:

* https://groups.google.com/forum/#!topic/jclouds-dev/H6Q5mhpCq0c
* https://github.com/rackspace/jclouds/commit/d897c5e44717e3d66062cd1275033e414f6a2de2
* https://issues.apache.org/jira/browse/JCLOUDS-181

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-315926003

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
Closed #1120.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#event-1215196971

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

I tested removing Content-Length from `BaseBlobIntegrationTest.testPutInputStream` and found:

requires C-L:

* Atmos
* AWS-S3, [reference](https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html)
* Azure Blob, [reference](https://docs.microsoft.com/en-us/rest/api/storageservices/put-blob)
* B2, [reference](https://www.backblaze.com/b2/docs/b2_upload_file.html)

requires C-L but should not:

* GCS, [reference](https://cloud.google.com/storage/docs/json_api/v1/objects/insert)

does not require C-L but should:

* Rackspace Cloud Files/OpenStack Swift, [reference](https://developer.openstack.org/api-ref/object-storage/?expanded=create-or-replace-object-detail#create-or-replace-object)

I do not understand jclouds chunked encoding support well enough to comment but otherwise our portable call sites should set Content-Length.  For what it is worth I prefer the original code which does not assume a Content-Length and not the suggested change.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128358733

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@ChaithanyaGK pushed 1 commit.

22692e6  Code cleanup and added integration tests


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/467eeccb71b1e041301b0b4d720c6b22ec37a401..22692e66b2ddc8adec4f55a74cbe9f1f987da28c

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@andrewgaul Please have a look at the Charles screenshot attached, where two calls are made in case of azure putBlob operation with zero length blob.

![charles1](https://user-images.githubusercontent.com/28896513/28322642-3d1b2142-6bf4-11e7-856e-1d3c00d6d9e2.png)

I've added a unit test case to check whether Expect header is stripped in the case of the empty request body.  Not sure how to add an integration test to actually check that only one network call is being made. Any leads on this would be very helpful. 


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316083949

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

@nacx I've debugged this further and below are my findings:

1. `content-length` is always set for non-stream payloads like [String](https://github.com/jclouds/jclouds/blob/0bc935dd57dc8009731d05c533edd831c8642664/core/src/main/java/org/jclouds/io/payloads/StringPayload.java#L38),  [ByteArray](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/ByteArrayPayload.java#L31) etc.,
2. The only scenario I'm worried is in the case of `chunked` transfer with Stream payloads (like [InputStream](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/InputStreamPayload.java#L23), [ByteSource ](https://github.com/jclouds/jclouds/blob/4bbca9edf943852ce1ea5aa579fa0554f770a3ea/core/src/main/java/org/jclouds/io/payloads/ByteSourcePayload.java#L30)etc., ) how to check the body is empty?

Just to verify the case, I tried to do azure putBlob without setting the content-length for ByteSource payload:

```
ByteSource payload = ByteSource.wrap("testdata".getBytes(Charsets.UTF_8));
Blob blob = blobStore.blobBuilder(blobName)
          		.payload(payload)
       			//.contentLength(payload.size())
           		.build();
blobStore.putBlob(containerName, blob);
```

I'm getting the below error saying I've to specify the content-length (which is not the case with non-stream payloads):
```
Exception in thread "main" java.lang.IllegalArgumentException: size must be set
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:125)
	at org.jclouds.azureblob.binders.BindAzureBlobMetadataToRequest.bindToRequest(BindAzureBlobMetadataToRequest.java:56)
	at org.jclouds.rest.internal.RestAnnotationProcessor.decorateRequest(RestAnnotationProcessor.java:660)
	at org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:354)
	at org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:137)
```
So, I'm wondering whether there would be a scenario where content-length is not set ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128252872

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
> You should add a test for putting a zero-length blob to BaseBlobStoreIntegrationTest and test this against a few providers. We probably want a second test for streaming zero-length InputStream payloads as well.

@andrewgaul I've added integration tests for zero-length `String`, `ByteSource` and `InputStream` payloads,  and successfully tested against Azure blob and aws-s3

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316718635

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@andrewgaul This PR doesn't introduce any regression. `putBlob` with zero-length inputstream is failing before as well on google-cloud. The thing is we don't have a test case for that scenario before, so you didn't see any test failure.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-320528041

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -73,7 +74,7 @@ public void testZeroLengthPutHasContentLengthHeader() throws IOException, Interr
       RecordedRequest request = server.takeRequest();
       assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
       assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
-      assertEquals(request.getHeaders(EXPECT), ImmutableList.of("100-continue"));
+      assertThat(request.getHeaders(EXPECT).isEmpty());

Good spot,  will update.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128437618

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

Could we better add explicitly the `chunked` case here? I think we should be explicit about which exact cases represent empty bodies.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128188359

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
Pushed to master as e331a000d17d36043165cf79c505a7c0a7370f05.  I still do not understand the usefulness but there is no technical argument against this change.  If you want to improve on this commit you could investigate which providers actually need/want 100-continue and provide configuration to disable 100-continue for small, non-zero payloads.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-323914940

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@ChaithanyaGK pushed 1 commit.

dad7d0a  Code review changes


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1120/files/22692e66b2ddc8adec4f55a74cbe9f1f987da28c..dad7d0a8f95829b8b96e54e1a3337f4cf7d585e3

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.

Thanks, @ChaithanyaGK!
Just one minor style comment. The changes LGTM. I'm OK to merge this as soon as @demobox and @andrewgaul are happy with the change too.

> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

This could be a bit cleaner:
```java
boolean isBodyEmpty = true;
if (request.getPayload() != null) {
   Long length = request.getPayload().getContentMetadata().getContentLength();
   isBodyEmpty = (length == null || length == 0);
}
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50815389

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Andrew Gaul <no...@github.com>.
> Which existing integration test exercises this code path? If none exists, please add one. Also post the results from some real providers.

You did not address my comment.  You should add a test for putting a zero-length blob to `BaseBlobStoreIntegrationTest` and test this against a few providers.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-316462549

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

Thanks for the analysis @andrewgaul Will stick to the original code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128438490

Re: [jclouds/jclouds] JCLOUDS-1322: Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
@andrewgaul Could you please let me know is there anything else that needs to be addressed here.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#issuecomment-317779896

Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)

Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.

Thanks @ChaithanyaGK!
@andrewgaul Mind having a look?

> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) {
       if (request.getPayload() != null) {
          contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), headers);
       }
+
+      boolean isEmptyBody = false;
+      if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {

Should we consider "PATCH" operations too?

> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation) {
       if (request.getPayload() != null) {
          contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(), headers);
       }
+
+      boolean isEmptyBody = false;
+      if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
+         if (request.getPayload() != null) {
+            Long length = request.getPayload().getContentMetadata().getContentLength();
+            if (length != null && length == 0) {
+               isEmptyBody = true;
+            }
+         } else {
+            isEmptyBody = true;
+         }
+      }
+      if (isEmptyBody) {
+         request = request.toBuilder().removeHeader("Expect").build();
+      }

Let's refactor this to a method that gets a request and returns the modified one so it can be properly unit-tested.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50308589

Re: [jclouds/jclouds] Remove Expect header for requests with empty body (#1120)

Posted by Chaithanya Ganta <no...@github.com>.
ChaithanyaGK commented on this pull request.



> @@ -779,6 +782,22 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       return parts.build();
    }
 
+   private static GeneratedHttpRequest stripExpectHeaderIfContentZero(GeneratedHttpRequest request) {
+      boolean isBodyEmpty = false;
+      if (request.getPayload() != null) {
+         Long length = request.getPayload().getContentMetadata().getContentLength();
+         if (length != null && length == 0) {
+            isBodyEmpty = true;
+         }
+      } else {
+         isBodyEmpty = true;
+      }

Thanks @nacx , will update the PR.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r128164859