Update to address various review comments

- error out of getUserBucketUsages() if bucket param is set but userid is not
- added unit tests for the same.
- split some copy/paste code into a new function
- added unit tests for new function which required moving the test to
  the right package to test a protected function.
- use the base class logger
- misc tidy ups.

Author: tpodowd

plugins/integrations/cloudian/src/main/java/org/apache/cloudstack/cloudian/client/CloudianClient.java

@@ -18,6 +18,7 @@
 package org.apache.cloudstack.cloudian.client;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.PushbackInputStream;
 import java.net.SocketTimeoutException;
 import java.security.KeyManagementException;
@@ -156,6 +157,30 @@ private void throwTimeoutOrServerException(final IOException e) {
         }
     }
 
+    /**
+     * Return the body content stream only if the body has bytes.
+     *
+     * Unfortunately, some of the responses such as listGroups() or listUsers() return
+     * an empty body instead of returning and empty list. The only way to detect this is
+     * to try read from the body. This method handles this and will return null if the
+     * body was empty or a valid stream with the body content otherwise.
+     *
+     * @param response the response to check for the body contents.
+     * @return a valid InputStream or null if the body was empty.
+     *
+     * @throws IOException some error reading from the body such as timeout etc.
+     */
+    protected InputStream getNonEmptyContentStream(HttpResponse response) throws IOException {
+        PushbackInputStream iStream = new PushbackInputStream(response.getEntity().getContent());
+        int firstByte=iStream.read();
+        if (firstByte == -1) {
+            return null;
+        } else {
+            iStream.unread(firstByte);
+            return iStream;
+        }
+    }
+
     private HttpResponse delete(final String path) throws IOException {
         final HttpResponse response = httpClient.execute(new HttpDelete(adminApiUrl + path), httpContext);
         checkAuthFailure(response);
@@ -235,14 +260,17 @@ public String getServerVersion() {
      *
      * @param groupId the groupId is required (and must exist)
      * @param userId the userId is optional (null) and if not set all group users are returned.
-     * @param bucket the bucket is optional (null). If set the userId must also be set.
+     * @param bucket the bucket is optional (null). If set, the userId must also be set.
      * @return a list of bucket usages (possibly empty).
      * @throws ServerApiException on non-200 response such as unknown groupId etc or response issue.
      */
     public List<CloudianUserBucketUsage> getUserBucketUsages(final String groupId, final String userId, final String bucket) {
-        if (StringUtils.isBlank(groupId)) {
-            return new ArrayList<>();
+        if (StringUtils.isBlank(groupId) || (StringUtils.isBlank(userId) && !StringUtils.isBlank(bucket))) {
+            String msg = String.format("Bad parameters groupId=%s userId=%s bucket=%s", groupId, userId, bucket);
+            logger.error(msg);
+            throw new ServerApiException(ApiErrorCode.PARAM_ERROR, msg);
         }
+
         logger.debug("Getting bucket usages for groupId={} userId={} bucket={}", groupId, userId, bucket);
         StringBuilder cmd = new StringBuilder("/system/bucketusage?groupId=");
         cmd.append(groupId);
@@ -251,7 +279,6 @@ public List<CloudianUserBucketUsage> getUserBucketUsages(final String groupId, f
             cmd.append(userId);
         }
         if (! StringUtils.isBlank(bucket)) {
-            // Assume userId is also set (or request fails).
             cmd.append("&bucket=");
             cmd.append(bucket);
         }
@@ -341,15 +368,10 @@ public List<CloudianUser> listUsers(final String groupId) {
             if (noResponseEntity(response)) {
                 throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "API error");
             }
-            // The empty list case is badly behaved and returns as 200 with an empty body.
-            // We'll try detect this by reading the first byte and then putting it back if required.
-            PushbackInputStream iStream = new PushbackInputStream(response.getEntity().getContent());
-            int firstByte=iStream.read();
-            if (firstByte == -1) {
-                return new ArrayList<>(); // EOF => empty list
+            InputStream iStream = getNonEmptyContentStream(response);
+            if (iStream == null) {
+                return new ArrayList<>(); // empty body => empty list
             }
-            // unread that first byte and process the JSON
-            iStream.unread(firstByte);
             final ObjectMapper mapper = new ObjectMapper();
             return Arrays.asList(mapper.readValue(iStream, CloudianUser[].class));
         } catch (final IOException e) {
@@ -512,15 +534,10 @@ public List<CloudianGroup> listGroups() {
             if (noResponseEntity(response)) {
                 throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "API error");
             }
-            // The empty list case is badly behaved and returns as 200 with an empty body.
-            // We'll try detect this by reading the first byte and then putting it back if required.
-            PushbackInputStream iStream = new PushbackInputStream(response.getEntity().getContent());
-            int firstByte=iStream.read();
-            if (firstByte == -1) {
-                return new ArrayList<>(); // EOF => empty list
+            InputStream iStream = getNonEmptyContentStream(response);
+            if (iStream == null) {
+                return new ArrayList<>(); // Empty body => empty list
             }
-            // unread that first byte and process the JSON
-            iStream.unread(firstByte);
             final ObjectMapper mapper = new ObjectMapper();
             return Arrays.asList(mapper.readValue(iStream, CloudianGroup[].class));
         } catch (final IOException e) {

plugins/integrations/cloudian/src/test/java/org/apache/cloudstack/cloudian/CloudianClientTest.java renamed to plugins/integrations/cloudian/src/test/java/org/apache/cloudstack/cloudian/client/CloudianClientTest.java

@@ -15,7 +15,7 @@
 // specific language governing permissions and limitations
 // under the License.
 
-package org.apache.cloudstack.cloudian;
+package org.apache.cloudstack.cloudian.client;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
 import static com.github.tomakehurst.wiremock.client.WireMock.containing;
@@ -31,26 +31,33 @@
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching;
 import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.http.HttpEntity;
+import org.apache.http.HttpResponse;
+
+import org.apache.cloudstack.api.ApiErrorCode;
 import org.apache.cloudstack.api.ServerApiException;
-import org.apache.cloudstack.cloudian.client.CloudianClient;
-import org.apache.cloudstack.cloudian.client.CloudianCredential;
-import org.apache.cloudstack.cloudian.client.CloudianGroup;
-import org.apache.cloudstack.cloudian.client.CloudianUser;
-import org.apache.cloudstack.cloudian.client.CloudianUserBucketUsage;
 import org.apache.cloudstack.cloudian.client.CloudianUserBucketUsage.CloudianBucketUsage;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
 
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.github.tomakehurst.wiremock.client.BasicCredentials;
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
 
+@RunWith(MockitoJUnitRunner.class)
 public class CloudianClientTest {
     private final int port = 14333;
     private final int timeout = 2;
@@ -119,6 +126,38 @@ public void testBasicAuthFailure() {
         client.listUser("someUserId", "somegGroupId");
     }
 
+    @Test
+    public void getNonEmptyContentStreamEmpty() {
+        InputStream emptyStream = new ByteArrayInputStream(new byte[]{});
+        HttpEntity entity = mock(HttpEntity.class);
+        HttpResponse response = mock(HttpResponse.class);
+        when(response.getEntity()).thenReturn(entity);
+        try {
+                when(entity.getContent()).thenReturn(emptyStream);
+                Assert.assertNull(client.getNonEmptyContentStream(response));
+        } catch (IOException e) {
+                Assert.fail("Should not be any exception here");
+        }
+    }
+
+    @Test
+    public void getNonEmptyContentStreamWithContent() {
+        InputStream nonEmptyStream = new ByteArrayInputStream(new byte[]{9, 8});
+        HttpEntity entity = mock(HttpEntity.class);
+        HttpResponse response = mock(HttpResponse.class);
+        when(response.getEntity()).thenReturn(entity);
+        try {
+                when(entity.getContent()).thenReturn(nonEmptyStream);
+                InputStream is = client.getNonEmptyContentStream(response);
+                Assert.assertNotNull(is);
+                Assert.assertEquals(9, is.read());
+                Assert.assertEquals(8, is.read());
+                Assert.assertEquals(-1, is.read());
+        } catch (IOException e) {
+                Assert.fail("Should not be any exception here");
+        }
+    }
+
     /////////////////////////////////////////////////////
     //////////////// System API tests ///////////////////
     /////////////////////////////////////////////////////
@@ -135,6 +174,20 @@ public void getServerVersion() {
         Assert.assertEquals(expect, version);
     }
 
+    @Test
+    public void getUserBucketUsagesBadUsageBlankGroup() {
+        ServerApiException thrown = Assert.assertThrows(ServerApiException.class, () -> client.getUserBucketUsages(null, null, null));
+        Assert.assertNotNull(thrown);
+        Assert.assertEquals(ApiErrorCode.PARAM_ERROR, thrown.getErrorCode());
+    }
+
+    @Test
+    public void getUserBucketUsagesBadUsageBlankUserWithBucket() {
+        ServerApiException thrown = Assert.assertThrows(ServerApiException.class, () -> client.getUserBucketUsages("group", "", "bucket"));
+        Assert.assertNotNull(thrown);
+        Assert.assertEquals(ApiErrorCode.PARAM_ERROR, thrown.getErrorCode());
+    }
+
     @Test
     public void getUserBucketUsagesEmptyGroup() {
         wireMockRule.stubFor(get(urlEqualTo("/system/bucketusage?groupId=mygroup"))

plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImpl.java

@@ -41,8 +41,6 @@
 import org.apache.cloudstack.storage.object.BaseObjectStoreDriverImpl;
 import org.apache.cloudstack.storage.object.Bucket;
 import org.apache.cloudstack.storage.object.BucketObject;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 
 import com.amazonaws.AmazonClientException;
 import com.amazonaws.services.identitymanagement.AmazonIdentityManagement;
@@ -82,8 +80,6 @@
 import com.cloud.utils.exception.CloudRuntimeException;
 
 public class CloudianHyperStoreObjectStoreDriverImpl extends BaseObjectStoreDriverImpl {
-    protected Logger logger = LogManager.getLogger(CloudianHyperStoreObjectStoreDriverImpl.class);
-
     @Inject
     AccountDao _accountDao;
 

plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtil.java

@@ -64,33 +64,28 @@ public class CloudianHyperStoreUtil {
 
     public static final String IAM_USER_USERNAME = "CloudStack";
     public static final String IAM_USER_POLICY_NAME = "CloudStackPolicy";
-    public static final String IAM_USER_POLICY;
-    static {
-        StringBuilder sb = new StringBuilder();
-        sb.append("{\n");
-        sb.append("  \"Version\": \"2012-10-17\",\n");
-        sb.append("  \"Statement\": [\n");
-        sb.append("    {\n");
-        sb.append("      \"Sid\": \"AllowFullS3Access\",\n");
-        sb.append("      \"Effect\": \"Allow\",\n");
-        sb.append("      \"Action\": [\n");
-        sb.append("        \"s3:*\"\n");
-        sb.append("      ],\n");
-        sb.append("      \"Resource\": \"*\"\n");
-        sb.append("    },\n");
-        sb.append("    {\n");
-        sb.append("      \"Sid\": \"ExceptBucketCreationOrDeletion\",\n");
-        sb.append("      \"Effect\": \"Deny\",\n");
-        sb.append("      \"Action\": [\n");
-        sb.append("        \"s3:createBucket\",\n");
-        sb.append("        \"s3:deleteBucket\"\n");
-        sb.append("      ],\n");
-        sb.append("      \"Resource\": \"*\"\n");
-        sb.append("    }\n");
-        sb.append("  ]\n");
-        sb.append("}\n");
-        IAM_USER_POLICY = sb.toString();
-    }
+    public static final String IAM_USER_POLICY = "{\n" +
+        "  \"Version\": \"2012-10-17\",\n" +
+        "  \"Statement\": [\n" +
+        "    {\n" +
+        "      \"Sid\": \"AllowFullS3Access\",\n" +
+        "      \"Effect\": \"Allow\",\n" +
+        "      \"Action\": [\n" +
+        "        \"s3:*\"\n" +
+        "      ],\n" +
+        "      \"Resource\": \"*\"\n" +
+        "    },\n" +
+        "    {\n" +
+        "      \"Sid\": \"ExceptBucketCreationOrDeletion\",\n" +
+        "      \"Effect\": \"Deny\",\n" +
+        "      \"Action\": [\n" +
+        "        \"s3:createBucket\",\n" +
+        "        \"s3: deleteBucket\"\n" +
+        "      ],\n" +
+        "      \"Resource\": \"*\"\n" +
+        "    }\n" +
+        "  ]\n" +
+        "}\n";
 
     /**
      * This method is solely for test purposes so that we can mock the timeout.
@@ -120,9 +115,7 @@ public static CloudianClient getCloudianClient(String url, String user, String p
                 port = DEFAULT_ADMIN_PORT;
             }
             return new CloudianClient(host, port, scheme, user, pass, validateSSL, getAdminTimeoutSeconds());
-        } catch (MalformedURLException e) {
-            throw new CloudRuntimeException(e);
-        } catch (KeyStoreException | NoSuchAlgorithmException | KeyManagementException e) {
+        } catch (MalformedURLException | KeyStoreException | NoSuchAlgorithmException | KeyManagementException e) {
             throw new CloudRuntimeException(e);
         }
     }