Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage queue linting, builder refactor, tests #4383

Merged
merged 17 commits into from
Jul 22, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@
<Or>
<Class name="com.azure.storage.blob.BlobInputStream"/>
<Class name="com.azure.storage.blob.BlobOutputStream"/>
<Class name="com.azure.storage.queue.QueueServiceClient"/>
</Or>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>
Expand Down
6 changes: 6 additions & 0 deletions eng/jacoco-test-coverage/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
<azure-identity.version>1.0.0-preview.1</azure-identity.version>
<azure-keyvault.version>4.0.0-preview.1</azure-keyvault.version>
<azure-messaging-eventhubs.version>5.0.0-preview.2</azure-messaging-eventhubs.version>
<azure-storage-queue.version>12.0.0-preview.1</azure-storage-queue.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these should go in alphabetical order, so 'queue' comes after 'blob'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

</properties>

<distributionManagement>
Expand Down Expand Up @@ -84,6 +85,11 @@
<artifactId>azure-messaging-eventhubs</artifactId>
<version>${azure-messaging-eventhubs.version}</version>
</dependency>
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-queue</artifactId>
<version>${azure-storage-queue.version}</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetical here (based on artifactId) - push to after blob below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


<!-- Tracing will be built and released separately. Removing tracing dependency
until we finalize dependency composition -->
Expand Down
7 changes: 7 additions & 0 deletions eng/spotbugs-aggregate-report/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<azure-identity.version>1.0.0-preview.1</azure-identity.version>
<azure-keyvault.version>4.0.0-preview.1</azure-keyvault.version>
<azure-messaging-eventhubs.version>5.0.0-preview.2</azure-messaging-eventhubs.version>
<azure-storage-queue.version>12.0.0-preview.1</azure-storage-queue.version>
</properties>
<distributionManagement>
<site>
Expand Down Expand Up @@ -58,6 +59,7 @@
<source>..\..\core\azure-core-test\src\main\java</source>
<source>..\..\eventhubs\client\azure-eventhubs\src\main\java</source>
<source>..\..\eventhubs\client\azure-eventhubs\src\samples\java</source>
<source>..\..\storage\client\queue\src\samples\java</source>
</sources>
</configuration>
</execution>
Expand Down Expand Up @@ -142,6 +144,11 @@
<artifactId>azure-data-appconfiguration</artifactId>
<version>${azure-data-appconfiguration.version}</version>
</dependency>
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-queue</artifactId>
<version>${azure-storage-queue.version}</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-messaging-eventhubs</artifactId>
Expand Down
6 changes: 6 additions & 0 deletions pom.client.xml
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,10 @@
<title>Azure Storage - Blobs</title>
<packages>com.azure.storage.blob*</packages>
</group>
<group>
<title>Azure Storage - Queues</title>
<packages>com.azure.storage.client.queue*</packages>
</group>
JonathanGiles marked this conversation as resolved.
Show resolved Hide resolved
</groups>
<links>
<link>https://docs.oracle.com/javase/8/docs/api/</link>
Expand Down Expand Up @@ -542,6 +546,7 @@
-snippetpath ${project.basedir}/eventhubs/client/azure-eventhubs/src/samples/java
-snippetpath ${project.basedir}/keyvault/client/keys/src/samples/java
-snippetpath ${project.basedir}/keyvault/client/secrets/src/samples/java
-snippetpath ${project.basedir}/storage/client/queue/src/samples/java
JonathanGiles marked this conversation as resolved.
Show resolved Hide resolved
</additionalOptions>
</configuration>
</reportSet>
Expand Down Expand Up @@ -745,5 +750,6 @@
<!-- <module>./sdk/tracing</module>-->
<module>./sdk/identity/azure-identity</module>
<module>./storage/client/blob</module>
<module>./storage/client/queue</module>
</modules>
</project>
7 changes: 7 additions & 0 deletions storage/client/queue/package-info.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

/**
* This package contains the classes to perform actions on Azure Storage Queue.
*/
package com.azure.storage.queue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package-info.java file is in the entirely wrong directory. Again, generate JavaDoc and you would see that there are missing descriptions for packages in your project. This should also fail CheckStyle rules, so please be sure you've run CheckStyle, SpotBugs, and JavaDoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with move package-info to right place.

93 changes: 93 additions & 0 deletions storage/client/queue/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<parent>
<groupId>com.azure</groupId>
<artifactId>azure-client-sdk-parent</artifactId>
<version>1.1.0</version>
<relativePath>../../../pom.client.xml</relativePath>
</parent>

<modelVersion>4.0.0</modelVersion>

<groupId>com.azure</groupId>
<artifactId>azure-storage-queue</artifactId>
<version>12.0.0-preview.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is queues going to release as preview 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alzimmermsft said we can have the version number 12.0.0-preview.1

I did not aware of any other proposal.

Let me know if we need to change to something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed to Preview 2, File, Queues, and the Common package will all skip Preview 1 to be consistent with Blobs and the other languages.


<name>azure-storage-queue</name>
<url>https://github.com/Azure/azure-sdk-for-java</url>

<distributionManagement>
<site>
<id>azure-java-build-docs</id>
<url>${site.url}/site/${project.artifactId}</url>
</site>
</distributionManagement>

<scm>
<url>scm:git:https://github.com/Azure/azure-sdk-for-java</url>
<connection>scm:git:[email protected]:Azure/azure-sdk-for-java.git</connection>
<tag>HEAD</tag>
</scm>

<dependencies>
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-core</artifactId>
<version>1.0.0-preview.2</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slf4j shouldn't be required here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-core-test</artifactId>
<version>1.0.0-preview.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-identity</artifactId>
<version>1.0.0-preview.1</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

<dependency>
<groupId>io.projectreactor</groupId>
<artifactId>reactor-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>adal4j</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is reactor-test or adal4j used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed adal4j

Reator-test is necessary for async tests.

<dependency>
<groupId>org.spockframework</groupId>
<artifactId>spock-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>cglib</groupId>
<artifactId>cglib-nodep</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>uk.org.lidalia</groupId>
<artifactId>slf4j-test</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm dubious all of these dependencies are required. Can you confirm that they all are, and remove any that are not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

</dependencies>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ public final class SharedKeyCredential {
private static final String AUTHORIZATION_HEADER_FORMAT = "SharedKey %s:%s";

// Pieces of the connection string that are needed.
private static final String ACCOUNT_NAME = "AccountName".toLowerCase();
private static final String ACCOUNT_KEY = "AccountKey".toLowerCase();
private static final String ACCOUNT_NAME = "accountname";
private static final String ACCOUNT_KEY = "accountkey";

private final String accountName;
private final byte[] accountKey;
Expand Down Expand Up @@ -59,7 +59,7 @@ public static SharedKeyCredential fromConnectionString(String connectionString)
HashMap<String, String> connectionStringPieces = new HashMap<>();
for (String connectionStringPiece : connectionString.split(";")) {
String[] kvp = connectionStringPiece.split("=", 2);
connectionStringPieces.put(kvp[0].toLowerCase(), kvp[1]);
connectionStringPieces.put(kvp[0].toLowerCase(Locale.ROOT), kvp[1]);
}

String accountName = connectionStringPieces.get(ACCOUNT_NAME);
Expand Down Expand Up @@ -98,9 +98,11 @@ public String generateAuthorizationHeader(URL requestURL, String httpMethod, Map
*
* @param stringToSign The UTF-8-encoded string to sign.
* @return A {@code String} that contains the HMAC-SHA256-encoded signature.
* @throws InvalidKeyException If the accountKey is not a valid Base64-encoded string.
* @throws RuntimeException for one of the following cases:
* - If the HMAC-SHA256 signature for {@code sharedKeyCredentials} fails to generate.
* - If the an invalid key has been given to the client.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generate the JavaDoc and review this - it won't look good most probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sharedKey is in common. Alan and I prefer to have the queue and file merged in first, and then have another PR for common specifically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc should be fixed before it is merged - it is just a formatting issue, not a logic issue.

*/
public String computeHmac256(final String stringToSign) throws InvalidKeyException {
public String computeHmac256(final String stringToSign) {
try {
/*
We must get a new instance of the Mac calculator for each signature calculated because the instances are
Expand All @@ -111,8 +113,10 @@ public String computeHmac256(final String stringToSign) throws InvalidKeyExcepti
hmacSha256.init(new SecretKeySpec(this.accountKey, "HmacSHA256"));
byte[] utf8Bytes = stringToSign.getBytes(StandardCharsets.UTF_8);
return Base64.getEncoder().encodeToString(hmacSha256.doFinal(utf8Bytes));
} catch (final NoSuchAlgorithmException e) {
throw new Error(e);
} catch (final NoSuchAlgorithmException e) {
throw new RuntimeException(e);
} catch (InvalidKeyException e) {
throw new RuntimeException("Please double check the account key. Error details: " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new ClientLogger.logAndThrow API whenever you throw an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not correct. Use logAndThrow

}
}

Expand All @@ -121,20 +125,20 @@ private String buildStringToSign(URL requestURL, String httpMethod, Map<String,
contentLength = contentLength.equals("0") ? "" : contentLength;

// If the x-ms-header exists ignore the Date header
String dateHeader = (headers.containsKey("x-ms-date")) ? "" : getStandardHeaderValue(headers,"Date");
String dateHeader = (headers.containsKey("x-ms-date")) ? "" : getStandardHeaderValue(headers, "Date");

return String.join("\n",
httpMethod,
getStandardHeaderValue(headers,"Content-Encoding"),
getStandardHeaderValue(headers,"Content-Language"),
getStandardHeaderValue(headers, "Content-Encoding"),
getStandardHeaderValue(headers, "Content-Language"),
contentLength,
getStandardHeaderValue(headers,"Content-MD5"),
getStandardHeaderValue(headers,"Content-Type"),
getStandardHeaderValue(headers, "Content-MD5"),
getStandardHeaderValue(headers, "Content-Type"),
dateHeader,
getStandardHeaderValue(headers,"If-Modified-Since"),
getStandardHeaderValue(headers,"If-Match"),
getStandardHeaderValue(headers, "If-Modified-Since"),
getStandardHeaderValue(headers, "If-Match"),
getStandardHeaderValue(headers, "If-None-Match"),
getStandardHeaderValue(headers,"If-Unmodified-Since"),
getStandardHeaderValue(headers, "If-Unmodified-Since"),
getStandardHeaderValue(headers, "Range"),
getAdditionalXmsHeaders(headers),
getCanonicalizedResource(requestURL));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.storage.common.policy;

import com.azure.core.http.HttpPipelineCallContext;
import com.azure.core.http.HttpPipelineNextPolicy;
import com.azure.core.http.HttpResponse;
import com.azure.core.http.policy.HttpPipelinePolicy;
import com.azure.storage.queue.QueueClient;
import com.azure.storage.queue.QueueServiceClient;
import reactor.core.publisher.Mono;

/**
* Anonymous credentials are to be used with with HTTP(S) requests that read blobs from public containers or requests
* that use a Shared Access Signature (SAS). This is because Anonymous credentials will not set an Authorization header.
* Pass an instance of this class as the credentials parameter when creating a new pipeline (typically with
* {@link QueueServiceClient}, {@link QueueClient}).
*/
public final class AnonymousCredentialPolicy implements HttpPipelinePolicy {

/**
* Returns an empty instance of {@code AnonymousCredentials}.
*/
public AnonymousCredentialPolicy() {
}



Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary new lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from Blob. There is no usage for queue. I would like to delete it.

@Override
public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
return next.process();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ public RequestRetryOptions() {
* <p><strong>Sample Code</strong></p>
*
* <p>For more samples, please see the <a href="https://github.com/Azure/azure-storage-java/blob/master/src/test/java/com/microsoft/azure/storage/Samples.java">samples file</a></p>
* @throws IllegalArgumentException If one of the following case exists:
* <ul>
* <li> There is only one null value for retryDelay and maxRetryDelay.<li/>
* <li> Unrecognized retry policy type.<li/>
* </ul>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better - but the indentation is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I aligned with the start point of the description. Follow the format above. Could you explain it more where these line should be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it all the way to the left with just a space (or a few of them at most).

*/
public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, Integer tryTimeout,
Long retryDelayInMs, Long maxRetryDelayInMs, String secondaryHost) {
Expand Down Expand Up @@ -105,7 +110,6 @@ public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, In
}
this.maxRetryDelayInMs = TimeUnit.SECONDS.toMillis(120);
}

this.secondaryHost = secondaryHost;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
public final class RequestRetryPolicy implements HttpPipelinePolicy {
private final RequestRetryOptions requestRetryOptions;

/**
* Create a policy of retrying a given HTTP request.
* @param requestRetryOptions Options for configuring the RequestRetryPolicy.
*/
public RequestRetryPolicy(RequestRetryOptions requestRetryOptions) {
this.requestRetryOptions = requestRetryOptions;
}
Expand Down
Loading