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

[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