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

Implement unexpectlengthexception in azure core #5108

Merged
merged 51 commits into from
Sep 5, 2019
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
8a3e7dc
Implement unexpectlengthexception in azure core
sima-zhu Aug 26, 2019
1d9f1cf
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Aug 26, 2019
fffb585
merge from mainline
sima-zhu Aug 26, 2019
89039fc
Remove unnessary files.
sima-zhu Aug 26, 2019
ee165be
Added more playback jsons
sima-zhu Aug 26, 2019
d3d8c04
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Aug 26, 2019
96f3b79
Push more changes to fix bytebuffer and file creation
sima-zhu Aug 26, 2019
623d6e7
Added more sync and async tests.
sima-zhu Aug 27, 2019
43ab40e
Remove extra blocks
sima-zhu Aug 27, 2019
d162cd1
enable blob
sima-zhu Aug 28, 2019
c71ceb4
Make changes to lazy verifing the length
sima-zhu Aug 29, 2019
0666ca4
add unexpectedlengthexception.
sima-zhu Aug 30, 2019
823999d
Removed the one in blob.
sima-zhu Aug 30, 2019
b4b0282
Checkstyle fix.
sima-zhu Aug 30, 2019
a80346c
Addressed comments.
sima-zhu Aug 30, 2019
d91091b
some changes
sima-zhu Aug 30, 2019
25a1f7f
Rename error msg
sima-zhu Aug 30, 2019
4ba94ba
Remove flux util changes.
sima-zhu Aug 30, 2019
05f3bb9
Moved exception under exception folder.
sima-zhu Aug 30, 2019
aae6689
Minor chanes
sima-zhu Aug 30, 2019
f45166a
merge from mainline
sima-zhu Aug 30, 2019
59211eb
Merge from mainline
sima-zhu Aug 30, 2019
98c7202
Fixed failed playback tests. Remove unnessary changes.
sima-zhu Aug 31, 2019
7ef2653
More improvements and fix spotbugs.
sima-zhu Aug 31, 2019
d8a819f
Common pkg checkstyle changes.
sima-zhu Aug 31, 2019
72d7075
Fixed checkstyle
sima-zhu Aug 31, 2019
9395346
Added unexpectedLengthException
sima-zhu Sep 2, 2019
8937136
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Sep 2, 2019
e09b7ed
Address checkstyle changes.
sima-zhu Sep 3, 2019
af3ef5a
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Sep 3, 2019
72eff56
Make changes on the total length implementation.
sima-zhu Sep 3, 2019
ec9f417
Changes in Utility
sima-zhu Sep 3, 2019
609e0b3
Clean up the excepted thrown exception.
sima-zhu Sep 3, 2019
088eaeb
Update the json files for azure-storage-file
sima-zhu Sep 3, 2019
e0b1d6e
Fixed async playback jsons
sima-zhu Sep 3, 2019
55a1b53
Added blank lines.
sima-zhu Sep 3, 2019
7a802f5
Reverted some changes in blob.
sima-zhu Sep 3, 2019
165a0b2
Use logger in helper methods.
sima-zhu Sep 3, 2019
c63f931
Remove extra lines
sima-zhu Sep 3, 2019
f599930
Clean up more tests.
sima-zhu Sep 3, 2019
3c34beb
Move exception to public exception folder.
sima-zhu Sep 4, 2019
dd1dbab
Make exception into public.
sima-zhu Sep 4, 2019
b88014e
Fixed javadoc complaint
sima-zhu Sep 4, 2019
942d7bc
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Sep 4, 2019
9a9ed7c
Addressed JavaDoc issue.
sima-zhu Sep 4, 2019
9ee4bb7
merge from mainline
sima-zhu Sep 4, 2019
78fff12
Fixed json after merge
sima-zhu Sep 4, 2019
5e16f5a
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Sep 4, 2019
645da4a
Added final
sima-zhu Sep 4, 2019
43b5581
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
sima-zhu Sep 4, 2019
8a38331
Added playback jsons
sima-zhu Sep 4, 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 @@ -155,6 +155,9 @@

<!-- ClientLogger class suppression -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="ClientLogger.java"/>
<!-- Use the logger in Utility static method. -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="com.azure.storage.common.Utility.java"/>

<!-- Azure Core Test -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files=".*[/\\]core[/\\]test[/\\].*\.java"/>
<!-- Class has static methods which using static logger instance, issue link: https://github.com/Azure/azure-sdk-for-java/issues/5137-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package com.azure.core.test.models;

import com.azure.core.exception.UnexpectedLengthException;
import com.fasterxml.jackson.annotation.JsonProperty;

import java.net.UnknownHostException;
Expand All @@ -12,12 +13,14 @@
* during the pipeline and deserialize them back into their actual throwable class when running in playback mode.
*/
public class NetworkCallError {
@JsonProperty("Throwable")
private Throwable throwable;

@JsonProperty("ClassName")
private String className;

@JsonProperty("ErrorMessage")
private String errorMessage;

private Throwable throwable;

/**
* Empty constructor used by deserialization.
*/
Expand All @@ -32,6 +35,7 @@ public NetworkCallError() {
public NetworkCallError(Throwable throwable) {
this.throwable = throwable;
this.className = throwable.getClass().getName();
this.errorMessage = throwable.getMessage();
}

/**
Expand All @@ -40,13 +44,16 @@ public NetworkCallError(Throwable throwable) {
public Throwable get() {
switch (className) {
case "java.lang.NullPointerException":
return new NullPointerException(throwable.getMessage());
return new NullPointerException(this.errorMessage);

case "java.lang.IndexOutOfBoundsException":
return new IndexOutOfBoundsException(throwable.getMessage());
return new IndexOutOfBoundsException(this.errorMessage);

case "java.net.UnknownHostException":
return new UnknownHostException(throwable.getMessage());
return new UnknownHostException(this.errorMessage);

case "com.azure.core.exception.UnexpectedLengthException":
return new UnexpectedLengthException(this.errorMessage, 0L, 0L);

default:
return throwable;
Expand All @@ -71,4 +78,14 @@ public void throwable(Throwable throwable) {
public void className(String className) {
this.className = className;
}

/**
* Sets the error message of the class of the throwable. This is used during deserialization the construct the
* throwable as the actual class that was thrown.
*
* @param errorMessage Error msg from the exception.
*/
public void errorMessage(String errorMessage) {
this.errorMessage = errorMessage;
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.storage.blob;
package com.azure.core.exception;

/**
* This exception class represents an error when the specified input length doesn't match the data length.
Expand All @@ -10,7 +10,13 @@ public final class UnexpectedLengthException extends IllegalStateException {
private final long bytesRead;
private final long bytesExpected;

UnexpectedLengthException(String message, long bytesRead, long bytesExpected) {
/**
* Constructor of the UnexpectedLengthException.
* @param message The message for the exception.
* @param bytesRead The number of bytes read from resource.
* @param bytesExpected The number of bytes expected from the receiver.
*/
public UnexpectedLengthException(String message, long bytesRead, long bytesExpected) {
super(message);
this.bytesRead = bytesRead;
this.bytesExpected = bytesExpected;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.azure.core.http.rest.PagedResponse;
import com.azure.core.http.rest.Response;
import com.azure.core.http.rest.ResponseBase;
import com.azure.core.exception.UnexpectedLengthException;
import com.azure.core.implementation.http.ContentType;
import com.azure.core.implementation.http.PagedResponseBase;
import com.azure.core.implementation.http.UrlBuilder;
Expand Down Expand Up @@ -142,10 +143,14 @@ public Object invoke(Object proxy, final Method method, Object[] args) {
Context context = methodParser.context(args).addData("caller-method", methodParser.fullyQualifiedMethodName());
context = startTracingSpan(method, context);

if (request.body() != null) {
request.body(validateLength(request));
}

final Mono<HttpResponse> asyncResponse = send(request, context);
//

Mono<HttpDecodedResponse> asyncDecodedResponse = this.decoder.decode(asyncResponse, methodParser);
//

return handleHttpResponse(request, asyncDecodedResponse, methodParser, methodParser.returnType(), context);
}

Expand All @@ -154,6 +159,36 @@ public Object invoke(Object proxy, final Method method, Object[] args) {
}
}

private Flux<ByteBuffer> validateLength(final HttpRequest request) {
final Flux<ByteBuffer> bbFlux = request.body();
if (bbFlux == null) {
return Flux.empty();
}

return Flux.defer(() -> {
Long expectedLength = Long.valueOf(request.headers().value("Content-Length"));
final long[] currentTotalLength = new long[1];
Copy link
Member

Choose a reason for hiding this comment

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

I guess what we're doing here is correct. I asked the question in SOF and got a response from David (the main contributor of rx) - https://stackoverflow.com/questions/57776012/mutating-array-elements-in-reactive-publisher-onnext-doonnext-methods/
The last thing to rule out is whether to use volatile keyword to ensure immediate update visibility across thread, waiting for his response.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Anu for taking care of this! Learned a lot from the thread.

return bbFlux.doOnEach(s -> {
if (s.isOnNext()) {
ByteBuffer byteBuffer = s.get();
int currentLength = (byteBuffer == null) ? 0 : byteBuffer.remaining();
currentTotalLength[0] += currentLength;
if (currentTotalLength[0] > expectedLength) {
throw logger.logExceptionAsError(new UnexpectedLengthException(
String.format("Request body emitted %d bytes more than the expected %d bytes.",
currentTotalLength[0], expectedLength), currentTotalLength[0], expectedLength));
}
} else if (s.isOnComplete()) {
if (expectedLength.compareTo(currentTotalLength[0]) != 0) {
throw logger.logExceptionAsError(new UnexpectedLengthException(
String.format("Request body emitted %d bytes less than the expected %d bytes.",
currentTotalLength[0], expectedLength), currentTotalLength[0], expectedLength));
}
}
});
});
}

private Method determineResumeMethod(Method method, String resumeMethodName) {
for (Method potentialResumeMethod : method.getDeclaringClass().getMethods()) {
if (potentialResumeMethod.getName().equals(resumeMethodName)) {
Expand Down Expand Up @@ -290,6 +325,8 @@ private HttpRequest configRequest(HttpRequest request, SwaggerMethodParser metho
if (!bodyContentString.isEmpty()) {
request.body(bodyContentString);
}
} else if (bodyContentObject instanceof ByteBuffer) {
request.body(Flux.just((ByteBuffer) bodyContentObject));
} else {
final String bodyContentString = serializer.serialize(bodyContentObject, SerializerEncoding.fromHeaders(request.headers()));
request.body(bodyContentString);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public static byte[] byteBufferToArray(ByteBuffer byteBuffer) {
byteBuffer.get(byteArray);
return byteArray;
}

/**
* This method converts the incoming {@code subscriberContext} from {@link reactor.util.context.Context Reactor
* Context} to {@link Context Azure Context} and calls the given lambda function with this context and returns a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,17 @@
import com.azure.core.http.rest.ResponseBase;
import com.azure.core.http.rest.StreamResponse;
import com.azure.core.http.rest.VoidResponse;
import com.azure.core.exception.UnexpectedLengthException;
import com.azure.core.implementation.http.ContentType;
import com.azure.core.implementation.serializer.SerializerAdapter;
import com.azure.core.implementation.serializer.jackson.JacksonAdapter;
import com.azure.core.implementation.util.FluxUtil;
import java.nio.charset.StandardCharsets;
import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;

Expand All @@ -57,6 +61,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import reactor.test.StepVerifier;

import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
Expand All @@ -74,6 +79,9 @@ public abstract class RestProxyTests {
*/
protected abstract HttpClient createHttpClient();

@Rule
public ExpectedException thrown = ExpectedException.none();

@Host("http://httpbin.org")
@ServiceInterface(name = "Service1")
private interface Service1 {
Expand Down Expand Up @@ -430,6 +438,16 @@ private interface Service9 {
@ExpectedResponses({200})
Mono<HttpBinJSON> putAsync(@BodyParam(ContentType.APPLICATION_OCTET_STREAM) int putBody);

@Put("put")
@ExpectedResponses({200})
@UnexpectedResponseExceptionType(MyRestException.class)
HttpBinJSON putBodyAndContentLength(@BodyParam("application/octet-stream") ByteBuffer body, @HeaderParam("Content-Length") long contentLength);

@Put("put")
@ExpectedResponses({200})
@UnexpectedResponseExceptionType(MyRestException.class)
Mono<HttpBinJSON> putAsyncBodyAndContentLength(@BodyParam("application/octet-stream") Flux<ByteBuffer> body, @HeaderParam("Content-Length") long contentLength);

@Put("put")
@ExpectedResponses({201})
HttpBinJSON putWithUnexpectedResponse(@BodyParam(ContentType.APPLICATION_OCTET_STREAM) String putBody);
Expand Down Expand Up @@ -500,6 +518,79 @@ public void asyncPutRequestWithIntBody() {
assertEquals("42", json.data());
}

// Test all scenarios for the body length and content length comparison for sync API
@Test
public void syncPutRequestWithBodyAndEqualContentLength() {
ByteBuffer body = ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8));
final HttpBinJSON json = createService(Service9.class)
.putBodyAndContentLength(body, 4L);
assertEquals("test", json.data());
assertEquals("application/octet-stream", json.headers().get(("Content-Type")));
assertEquals("4", json.headers().get(("Content-Length")));
body.clear();
}

@Test
public void syncPutRequestWithBodyLessThanContentLength() {
ByteBuffer body = ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8));
thrown.expect(UnexpectedLengthException.class);
thrown.expectMessage("less than");
createService(Service9.class)
.putBodyAndContentLength(body, 5L);
body.clear();
}

@Test
public void syncPutRequestWithBodyMoreThanContentLength() {
ByteBuffer body = ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8));
thrown.expect(UnexpectedLengthException.class);
thrown.expectMessage("more than");
createService(Service9.class)
.putBodyAndContentLength(body, 3L);
body.clear();
}

// Test all scenarios for the body length and content length comparison for Async API
@Test
public void asyncPutRequestWithBodyAndEqualContentLength() {
Flux<ByteBuffer> body = Flux.just(ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8)));
StepVerifier.create(createService(Service9.class)
.putAsyncBodyAndContentLength(body, 4L))
.assertNext(
json -> {
assertEquals("test", json.data());
assertEquals("application/octet-stream", json.headers().get(("Content-Type")));
assertEquals("4", json.headers().get(("Content-Length")));
}
).verifyComplete();
}

@Test
public void asyncPutRequestWithBodyAndLessThanContentLength() {
Flux<ByteBuffer> body = Flux.just(ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8)));
StepVerifier.create(createService(Service9.class)
.putAsyncBodyAndContentLength(body, 5L))
.verifyErrorSatisfies(
exception -> {
assertTrue(exception instanceof UnexpectedLengthException);
assertTrue(exception.getMessage().contains("less than"));
}
);
}

@Test
public void asyncPutRequestWithBodyAndMoreThanContentLength() {
Flux<ByteBuffer> body = Flux.just(ByteBuffer.wrap("test".getBytes(StandardCharsets.UTF_8)));
StepVerifier.create(createService(Service9.class)
.putAsyncBodyAndContentLength(body, 3L))
.verifyErrorSatisfies(
exception -> {
assertTrue(exception instanceof UnexpectedLengthException);
assertTrue(exception.getMessage().contains("more than"));
}
);
}

@Test
public void syncPutRequestWithUnexpectedResponse() {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package com.azure.storage.blob;

import com.azure.core.http.rest.Response;
import com.azure.core.exception.UnexpectedLengthException;
import com.azure.core.util.Context;
import com.azure.storage.blob.models.AppendBlobAccessConditions;
import com.azure.storage.blob.models.AppendBlobItem;
Expand All @@ -14,6 +15,7 @@
import com.azure.storage.blob.models.SourceModifiedAccessConditions;
import com.azure.storage.blob.models.StorageException;
import com.azure.storage.common.Utility;
import java.util.Objects;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.core.scheduler.Schedulers;
Expand Down Expand Up @@ -159,24 +161,14 @@ public AppendBlobItem appendBlock(InputStream data, long length) {
* @param context Additional context that is passed through the Http pipeline during the service call.
*
* @return A {@link Response} whose {@link Response#value() value} contains the append blob operation.
* @throws UnexpectedLengthException when the length of data does not match the input {@code length}.
Copy link
Member

Choose a reason for hiding this comment

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

You cannot throw UnexpectedLengthException here as it is not public API. You either need to make it public API, or not throw it from public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put the exception in public exception folder

* @throws NullPointerException if the input data is null.
*/
public Response<AppendBlobItem> appendBlockWithResponse(InputStream data, long length,
AppendBlobAccessConditions appendBlobAccessConditions, Duration timeout, Context context) {
Flux<ByteBuffer> fbb = Flux.range(0, (int) Math.ceil((double) length / (double) MAX_APPEND_BLOCK_BYTES))
.map(i -> i * MAX_APPEND_BLOCK_BYTES)
.concatMap(pos -> Mono.fromCallable(() -> {
long count = pos + MAX_APPEND_BLOCK_BYTES > length ? length - pos : MAX_APPEND_BLOCK_BYTES;
byte[] cache = new byte[(int) count];
int read = 0;
while (read < count) {
read += data.read(cache, read, (int) count - read);
}

return ByteBuffer.wrap(cache);
}));

Mono<Response<AppendBlobItem>> response = appendBlobAsyncClient.appendBlockWithResponse(
fbb.subscribeOn(Schedulers.elastic()), length, appendBlobAccessConditions, context);
Objects.requireNonNull(data);
Flux<ByteBuffer> fbb = Utility.convertStreamToByteBuffer(data, length, MAX_APPEND_BLOCK_BYTES);
Mono<Response<AppendBlobItem>> response = appendBlobAsyncClient.appendBlockWithResponse(fbb.subscribeOn(Schedulers.elastic()), length, appendBlobAccessConditions, context);
return Utility.blockWithOptionalTimeout(response, timeout);
}

Expand Down
Loading