-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from 2 commits
bf6a460
a5a8a87
bc590b5
52a8de0
4205b7e
d3de0c3
4091695
433dfbe
45e5fa4
1338dc7
844c0ff
1369721
9db31b9
4788a13
73f0b89
5b540c6
c440e58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done with move package-info to right place. |
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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is queues going to release as preview 1? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slf4j shouldn't be required here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nor here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is reactor-test or adal4j used? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is not correct. Use |
||
} | ||
} | ||
|
||
|
@@ -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)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.