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

Azure Storage Turn on Fixed Tests #4422

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
66be278
Removed raw clients
alzimmermsft Jul 10, 2019
5493c38
Fixed PageBlob getRange and getRangeDiff, turned on all unit tests
alzimmermsft Jul 11, 2019
64643ab
Turned on more tests
alzimmermsft Jul 11, 2019
c88a4c3
Partial work for implementing and testing reliable download options
alzimmermsft Jul 11, 2019
1ec9438
Storage SAS implementation (#4404)
gapra-msft Jul 12, 2019
40a8ce2
Turned on download retry test
alzimmermsft Jul 12, 2019
78db8d4
Merge branch 'master' into storage-post-review1-dev
alzimmermsft Jul 12, 2019
69026ba
Merge branch 'master' into storage-post-review1-dev
alzimmermsft Jul 16, 2019
be429c3
Remove RawClients from Blobs (#4375)
alzimmermsft Jul 16, 2019
e3f43d6
Merged upstream
alzimmermsft Jul 16, 2019
b00cdc6
Add deleteContainer to StorageClient and getBlobClient with Snapshot …
alzimmermsft Jul 19, 2019
da7ebb6
Merge branch 'master' into storage-post-review1-dev
alzimmermsft Jul 19, 2019
78ea66b
Storage queue linting, builder refactor, tests (#4383)
sima-zhu Jul 22, 2019
f65653f
Merge branch 'storage-post-review1-dev' into AzStorage_TurnOnTests
alzimmermsft Jul 24, 2019
248b3a9
merging in master
alzimmermsft Jul 25, 2019
f101d67
Fixed call to changed method
alzimmermsft Jul 26, 2019
bf411bf
Merge branch 'master' into AzStorage_TurnOnTests
alzimmermsft Jul 31, 2019
2e4ed28
Merged in master and fixed merge conflict
alzimmermsft Jul 31, 2019
81421d8
Fixed merging issues
alzimmermsft Jul 31, 2019
4bd07e6
Fixed method parameter order
alzimmermsft Jul 31, 2019
ef2daa4
Merge branch 'master' into AzStorage_TurnOnTests
alzimmermsft Aug 1, 2019
6cdd2d8
Merge branch 'master' into AzStorage_TurnOnTests
alzimmermsft Aug 2, 2019
623611e
Merge branch 'master' into AzStorage_TurnOnTests
alzimmermsft Aug 2, 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
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ public final class DownloadAsyncResponse {


// The constructor is package-private because customers should not be creating their own responses.
// TODO (unknown): resolve comment vs code mismatch
DownloadAsyncResponse(ResponseBase<BlobDownloadHeaders, Flux<ByteBuf>> response,
HTTPGetterInfo info, Function<HTTPGetterInfo, Mono<DownloadAsyncResponse>> getter) {
HTTPGetterInfo info, Function<HTTPGetterInfo, Mono<DownloadAsyncResponse>> getter) {
Utility.assertNotNull("getter", getter);
Utility.assertNotNull("info", info);
Utility.assertNotNull("info.eTag", info.eTag());
Expand All @@ -55,10 +54,8 @@ public final class DownloadAsyncResponse {
* {@code options.maxRetryRequests > 0}. If retries are enabled, if a connection fails while reading, the stream
* will make additional requests to reestablish a connection and continue reading.
*
* @param options
* {@link ReliableDownloadOptions}
*
* @return A {@code Flux} which emits the data as {@code ByteBuffer}s.
* @param options {@link ReliableDownloadOptions}
* @return A {@link Flux} which emits the data as {@link ByteBuf ByteBufs}
*/
public Flux<ByteBuf> body(ReliableDownloadOptions options) {
ReliableDownloadOptions optionsReal = options == null ? new ReliableDownloadOptions() : options;
Expand Down Expand Up @@ -86,38 +83,32 @@ possible the method call that returns a Single is what throws (like how our apis
call time rather than at subscription time.
*/
try {
// Get a new response and try reading from it.
return getter.apply(this.info)
.flatMapMany(response ->
/*
Do not compound the number of retries by passing in another set of downloadOptions; just get
the raw body.
*/
this.applyReliableDownload(this.rawResponse.value(), retryCount, options));
/*Get a new response and try reading from it.
Do not compound the number of retries by passing in another set of downloadOptions; just get
the raw body.
*/
return getter.apply(this.info).flatMapMany(response -> this.applyReliableDownload(this.rawResponse.value(), retryCount, options));
} catch (Exception e) {
// If the getter fails, return the getter failure to the user.
return Flux.error(e);
}
}
}

private Flux<ByteBuf> applyReliableDownload(Flux<ByteBuf> data,
int currentRetryCount, ReliableDownloadOptions options) {
return data
.doOnNext(buffer -> {
/*
Update how much data we have received in case we need to retry and propagate to the user the data we
have received.
*/
this.info.offset(this.info.offset() + buffer.readableBytes()); // was `remaining()` in Rx world
if (this.info.count() != null) {
this.info.count(this.info.count() - buffer.readableBytes()); // was `remaining()` in Rx world
}
})
.onErrorResume(t2 -> {
// Increment the retry count and try again with the new exception.
return tryContinueFlux(t2, currentRetryCount + 1, options);
});
private Flux<ByteBuf> applyReliableDownload(Flux<ByteBuf> data, int currentRetryCount, ReliableDownloadOptions options) {
return data.doOnNext(buffer -> {
/*
Update how much data we have received in case we need to retry and propagate to the user the data we
have received.
*/
this.info.offset(this.info.offset() + buffer.readableBytes()); // was `remaining()` in Rx world
if (this.info.count() != null) {
this.info.count(this.info.count() - buffer.readableBytes()); // was `remaining()` in Rx world
}
}).onErrorResume(t2 -> {
// Increment the retry count and try again with the new exception.
return tryContinueFlux(t2, currentRetryCount + 1, options);
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.azure.storage.blob.models.ModifiedAccessConditions;
import com.azure.storage.blob.models.PageBlobAccessConditions;
import com.azure.storage.blob.models.PageBlobItem;
import com.azure.storage.blob.models.PageList;
import com.azure.storage.blob.models.PageRange;
import com.azure.storage.blob.models.SequenceNumberActionType;
import com.azure.storage.blob.models.SourceModifiedAccessConditions;
Expand Down Expand Up @@ -336,7 +337,7 @@ public Mono<Response<PageBlobItem>> clearPages(PageRange pageRange,
* @return
* A reactive response containing the information of the cleared pages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Make return one line.

*/
public Flux<PageRange> getPageRanges(BlobRange blobRange) {
public Mono<Response<PageList>> getPageRanges(BlobRange blobRange) {
return this.getPageRanges(blobRange, null);
}

Expand All @@ -352,15 +353,15 @@ public Flux<PageRange> getPageRanges(BlobRange blobRange) {
* @return
* A reactive response emitting all the page ranges.
*/
public Flux<PageRange> getPageRanges(BlobRange blobRange, BlobAccessConditions accessConditions) {
public Mono<Response<PageList>> getPageRanges(BlobRange blobRange, BlobAccessConditions accessConditions) {
blobRange = blobRange == null ? new BlobRange(0) : blobRange;
accessConditions = accessConditions == null ? new BlobAccessConditions() : accessConditions;

return postProcessResponse(this.azureBlobStorage.pageBlobs().getPageRangesWithRestResponseAsync(
null, null, snapshot, null, null, blobRange.toHeaderValue(),
null, accessConditions.leaseAccessConditions(), accessConditions.modifiedAccessConditions(),
Context.NONE))
.flatMapMany(response -> Flux.fromIterable(response.value().pageRange()));
.map(response -> new SimpleResponse<>(response, response.value()));
}

/**
Expand All @@ -377,7 +378,7 @@ public Flux<PageRange> getPageRanges(BlobRange blobRange, BlobAccessConditions a
* @return
* A reactive response emitting all the different page ranges.
*/
public Flux<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapshot) {
public Mono<Response<PageList>> getPageRangesDiff(BlobRange blobRange, String prevSnapshot) {
return this.getPageRangesDiff(blobRange, prevSnapshot, null);
}

Expand All @@ -397,7 +398,7 @@ public Flux<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapsho
* @return A reactive response emitting all the different page ranges.
* @throws IllegalArgumentException If {@code prevSnapshot} is {@code null}
*/
public Flux<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapshot, BlobAccessConditions accessConditions) {
public Mono<Response<PageList>> getPageRangesDiff(BlobRange blobRange, String prevSnapshot, BlobAccessConditions accessConditions) {
blobRange = blobRange == null ? new BlobRange(0) : blobRange;
accessConditions = accessConditions == null ? new BlobAccessConditions() : accessConditions;

Expand All @@ -409,7 +410,7 @@ public Flux<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapsho
null, null, snapshot, null, null, prevSnapshot,
blobRange.toHeaderValue(), null, accessConditions.leaseAccessConditions(),
accessConditions.modifiedAccessConditions(), Context.NONE))
.flatMapMany(response -> Flux.fromIterable(response.value().pageRange()));
.map(response -> new SimpleResponse<>(response, response.value()));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.azure.storage.blob.models.ModifiedAccessConditions;
import com.azure.storage.blob.models.PageBlobAccessConditions;
import com.azure.storage.blob.models.PageBlobItem;
import com.azure.storage.blob.models.PageList;
import com.azure.storage.blob.models.PageRange;
import com.azure.storage.blob.models.SequenceNumberActionType;
import com.azure.storage.blob.models.SourceModifiedAccessConditions;
Expand Down Expand Up @@ -325,7 +326,7 @@ public Response<PageBlobItem> clearPages(PageRange pageRange,
* @return
* The information of the cleared pages.
*/
public Iterable<PageRange> getPageRanges(BlobRange blobRange) {
public Response<PageList> getPageRanges(BlobRange blobRange) {
return this.getPageRanges(blobRange, null, null);
}

Expand All @@ -343,10 +344,8 @@ public Iterable<PageRange> getPageRanges(BlobRange blobRange) {
* @return
* All the page ranges.
*/
public Iterable<PageRange> getPageRanges(BlobRange blobRange,
BlobAccessConditions accessConditions, Duration timeout) {
Flux<PageRange> response = pageBlobAsyncClient.getPageRanges(blobRange, accessConditions);
return timeout == null ? response.toIterable() : response.timeout(timeout).toIterable();
public Response<PageList> getPageRanges(BlobRange blobRange, BlobAccessConditions accessConditions, Duration timeout) {
return Utility.blockWithOptionalTimeout(pageBlobAsyncClient.getPageRanges(blobRange, accessConditions), timeout);
}

/**
Expand All @@ -363,7 +362,7 @@ public Iterable<PageRange> getPageRanges(BlobRange blobRange,
* @return
* All the different page ranges.
*/
public Iterable<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapshot) {
public Response<PageList> getPageRangesDiff(BlobRange blobRange, String prevSnapshot) {
return this.getPageRangesDiff(blobRange, prevSnapshot, null, null);
}

Expand All @@ -385,10 +384,8 @@ public Iterable<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSna
* @return
* All the different page ranges.
*/
public Iterable<PageRange> getPageRangesDiff(BlobRange blobRange, String prevSnapshot,
BlobAccessConditions accessConditions, Duration timeout) {
Flux<PageRange> response = pageBlobAsyncClient.getPageRangesDiff(blobRange, prevSnapshot, accessConditions);
return timeout == null ? response.toIterable() : response.timeout(timeout).toIterable();
public Response<PageList> getPageRangesDiff(BlobRange blobRange, String prevSnapshot, BlobAccessConditions accessConditions, Duration timeout) {
return Utility.blockWithOptionalTimeout(pageBlobAsyncClient.getPageRangesDiff(blobRange, prevSnapshot, accessConditions), timeout);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.azure.core.util.configuration.ConfigurationManager
import com.azure.identity.credential.EnvironmentCredential
import com.azure.storage.blob.models.*
import com.azure.storage.common.credentials.SharedKeyCredential
import io.netty.buffer.ByteBuf
import org.junit.Assume
import org.spockframework.lang.ISpecificationContext
import reactor.core.publisher.Flux
Expand Down Expand Up @@ -409,7 +410,7 @@ class APISpec extends Specification {
response.value().contentEncoding() == contentEncoding &&
response.value().contentLanguage() == contentLanguage &&
response.value().contentMD5() == contentMD5 &&
response.headers().value("Content-Type") == (contentType == null ? "application/octet-stream" : contentType)
response.headers().value("Content-Type") == contentType
}

static Metadata getMetadataFromHeaders(HttpHeaders headers) {
Expand Down Expand Up @@ -502,43 +503,51 @@ class APISpec extends Specification {
to play too nicely with mocked objects and the complex reflection stuff on both ends made it more difficult to work
with than was worth it. Because this type is just for BlobDownload, we don't need to accept a header type.
*/
def getStubResponseForBlobDownload(int code, Flux<ByteBuffer> body, String etag) {
return new HttpResponse() {
static class MockDownloadHttpResponse extends HttpResponse {
private final int statusCode
private final HttpHeaders headers
private final Flux<ByteBuf> body

MockDownloadHttpResponse(HttpResponse response, int statusCode, Flux<ByteBuf> body) {
this.request(response.request())
this.statusCode = statusCode
this.headers = response.headers()
this.body = body
}

@Override
int statusCode() {
return code
}
@Override
int statusCode() {
return statusCode
}

@Override
String headerValue(String s) {
return null
}
@Override
String headerValue(String s) {
return headers.value(s)
}

@Override
HttpHeaders headers() {
return new HttpHeaders()
}
@Override
HttpHeaders headers() {
return headers
}

@Override
Flux<ByteBuffer> body() {
return body
}
@Override
Flux<ByteBuf> body() {
return body
}

@Override
Mono<byte[]> bodyAsByteArray() {
return null
}
@Override
Mono<byte[]> bodyAsByteArray() {
return Mono.error(new IOException())
}

@Override
Mono<String> bodyAsString() {
return null
}
@Override
Mono<String> bodyAsString() {
return Mono.error(new IOException())
}

@Override
Mono<String> bodyAsString(Charset charset) {
return null
}
@Override
Mono<String> bodyAsString(Charset charset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary to have, it looks like a duplicate to the one above? Since it is a mock response, we can only include the necessary part.

Copy link
Member Author

Choose a reason for hiding this comment

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

You have to include everything as it's implementing an interface, I should change extends -> implements

return Mono.error(new IOException())
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package com.azure.storage.blob;

import com.azure.core.util.configuration.ConfigurationManager;
import com.azure.identity.credential.EnvironmentCredential;
import com.azure.storage.blob.models.ContainerItem;
import org.junit.BeforeClass;
Expand All @@ -15,8 +16,9 @@ public class AadLoginTest {

@BeforeClass
public static void setup() {
String endpoint = String.format("https://%s.blob.core.windows.net", ConfigurationManager.getConfiguration().get("PRIMARY_STORAGE_ACCOUNT_KEY"));
Copy link
Contributor

Choose a reason for hiding this comment

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

PRIMARY_STORAGE_ACCOUNT_KEY. It should be account name

Copy link
Member Author

Choose a reason for hiding this comment

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

This class is being deleted in another PR

storageClient = new BlobServiceClientBuilder()
.endpoint("https://" + System.getenv("ACCOUNT_NAME") + ".blob.core.windows.net")
.endpoint(endpoint)
.credential(new EnvironmentCredential())
// .httpClient(HttpClient.createDefault().proxy(() -> new ProxyOptions(Type.HTTP, new InetSocketAddress("localhost", 8888))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

This class is being deleted in another PR

.buildClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ class AppendBlobAPITest extends APISpec {
bu.create(headers, null, null, null)
Response<BlobProperties> response = bu.getProperties()

// If the value isn't set the service will automatically set it
contentType = (contentType == null) ? "application/octet-stream" : contentType
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason you remove it from APISpec but added here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't true for all cases that used the method


then:
validateBlobProperties(response, cacheControl, contentDisposition, contentEncoding, contentLanguage, contentMD5, contentType)

Expand Down
Loading