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.

Sign up for GitHub

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

Storage queue linting, builder refactor, tests #4383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bf6a460
Linting, builder refactor and playback tests
sima-zhu Jul 10, 2019
a5a8a87
Remove unnecssary changes
sima-zhu Jul 10, 2019
bc590b5
Address Alan's feedback.
sima-zhu Jul 11, 2019
52a8de0
Remove unused fields
sima-zhu Jul 11, 2019
4205b7e
Remove unused imports
sima-zhu Jul 11, 2019
d3de0c3
Merge branch 'storage-post-review1-dev' into storage_queue
sima-zhu Jul 12, 2019
4091695
Fixed the missing tag
sima-zhu Jul 13, 2019
433dfbe
Fixed the missing group id
sima-zhu Jul 13, 2019
45e5fa4
Added the queue service javadoc code snippet.
sima-zhu Jul 15, 2019
1338dc7
Added the queue service builder javadoc code snippet.
sima-zhu Jul 15, 2019
844c0ff
Added the queue builder javadoc code snippet.
sima-zhu Jul 15, 2019
1369721
Queue client java doc code snippet
sima-zhu Jul 16, 2019
9db31b9
Make minor changes and fix checkstyle.
sima-zhu Jul 16, 2019
4788a13
Addressed comments.
sima-zhu Jul 18, 2019
73f0b89
Make changes to queue configuration
sima-zhu Jul 18, 2019
5b540c6
Minor changes for queue java doc and readme code snippet.
sima-zhu Jul 20, 2019
c440e58
fix checkstyle
sima-zhu Jul 20, 2019
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
1 change: 1 addition & 0 deletions eng/code-quality-reports/src/main/resources/spotbugs/spotbugs-exclude.xml
View file
Open in desktop
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
7 changes: 6 additions & 1 deletion eng/jacoco-test-coverage/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<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-blob.version>12.0.0-preview.1</azure-storage-blob.version>
<azure-storage-queue.version>12.0.0-preview.1</azure-storage-queue.version>
</properties>

<distributionManagement>
Expand Down Expand Up @@ -93,7 +94,11 @@
<!-- <artifactId>azure-storage-blob</artifactId>-->
<!-- <version>${azure-storage-blob.version}</version>-->
<!-- </dependency>-->

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-queue</artifactId>
<version>${azure-storage-queue.version}</version>
</dependency>
<!-- Tracing will be built and released separately. Removing tracing dependency
until we finalize dependency composition -->
<!-- <dependency>-->
Expand Down
7 changes: 6 additions & 1 deletion eng/spotbugs-aggregate-report/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
<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.version>12.0.0-preview.1</azure-storage.version>
JonathanGiles marked this conversation as resolved.
Show resolved Hide resolved
<azure-storage-queue.version>12.0.0-preview.1</azure-storage-queue.version>
</properties>
<distributionManagement>
<site>
Expand Down Expand Up @@ -182,7 +183,11 @@
<!-- <artifactId>azure-storage-blob</artifactId>-->
<!-- <version>${azure-storage.version}</version>-->
<!-- </dependency>-->

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-queue</artifactId>
<version>${azure-storage-queue.version}</version>
</dependency>
<!-- Added this dependency to include necessary annotations used by reactor core.
Without this dependency, javadoc throws a warning as it cannot find enum When.MAYBE
which is used in @Nullable annotation in reactor core classes -->
Expand Down
8 changes: 7 additions & 1 deletion pom.client.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,10 @@
<title>Azure Storage - Blobs</title>
<packages>com.azure.storage.blob*</packages>
</group>
<group>
<title>Azure Storage - Queues</title>
<packages>com.azure.storage.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 @@ -546,7 +550,8 @@
-snippetpath ${project.basedir}/sdk/eventhubs/azure-eventhubs/src/samples/java
-snippetpath ${project.basedir}/sdk/keyvault/azure-keyvault-keys/src/samples/java
-snippetpath ${project.basedir}/sdk/keyvault/azure-keyvault-secrets/src/samples/java
<!-- -snippetpath ${project.basedir}/storage/client/blob/src/samples/java-->
<!-- -snippetpath ${project.basedir}/storage/client/blob/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 @@ -748,5 +753,6 @@
<!-- <module>./sdk/tracing</module>-->
<module>./sdk/identity/azure-identity</module>
<module>./storage/client/blob</module>
<module>./storage/client/queue</module>
</modules>
</project>
135 changes: 85 additions & 50 deletions storage/client/queue/README.md
View file
Open in desktop

Large diffs are not rendered by default.

78 changes: 78 additions & 0 deletions storage/client/queue/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?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.

</dependencies>
</project>
39 changes: 24 additions & 15 deletions .../client/queue/src/main/java/com/azure/storage/common/credentials/SharedKeyCredential.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package com.azure.storage.common.credentials;

import com.azure.core.implementation.util.ImplUtils;
import com.azure.core.util.logging.ClientLogger;
import io.netty.handler.codec.http.QueryStringDecoder;

import javax.crypto.Mac;
Expand All @@ -26,11 +27,12 @@
* SharedKey credential policy that is put into a header to authorize requests.
*/
public final class SharedKeyCredential {
private static final ClientLogger LOGGER = new ClientLogger(SharedKeyCredential.class);
Copy link
Member

Choose a reason for hiding this comment

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

This should never be static!

Copy link
Contributor Author

@sima-zhu sima-zhu Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

private final ClientLogger logger = new ClientLogger(someclass)

Is it the right way of initializing logger?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the logAndThrow API, it is a change in preview 3. I have no visibility as we depend on preview 2.

File an issue, and will address that after our dependency bump to preview 3.
#4551

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 +61,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 +100,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 +115,12 @@ 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) {
LOGGER.asError().log(e.getMessage());
throw new RuntimeException(e);
} catch (InvalidKeyException e) {
LOGGER.asError().log("Please double check the account key. Error details: " + e.getMessage());
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 +129,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 Expand Up @@ -224,6 +232,7 @@ private String computeHMACSHA256(String stringToSign) {
String signature = Base64.getEncoder().encodeToString(hmacSha256.doFinal(utf8Bytes));
return String.format(AUTHORIZATION_HEADER_FORMAT, accountName, signature);
} catch (NoSuchAlgorithmException | InvalidKeyException ex) {
LOGGER.asError().log(ex.getMessage());
throw new Error(ex);
}
}
Expand Down
6 changes: 5 additions & 1 deletion storage/client/queue/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions storage/client/queue/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java
View file
Open in desktop
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