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

Jump to bottom

Fix GrpcStatus for InvalidProtocolBufferException #5554

Merged
merged 8 commits into from
Apr 8, 2024
Merged

Fix GrpcStatus for InvalidProtocolBufferException #5554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fdd6fa5
fix: add case for InvalidProtocolBufferException in statusFromThrowab…
jaeseung-bae Apr 2, 2024
1062336
chore: lint fix(LINE OSS style)
jaeseung-bae Apr 2, 2024
59ec98a
test: add negative test for type-mismatched integer json value
jaeseung-bae Apr 2, 2024
a68a9d7
Merge remote-tracking branch 'upstream/main' into fix/handling-invali…
jaeseung-bae Apr 2, 2024
8ae6aef
Merge branch 'main' into fix/handling-invalid-protocol-buffer-exception
jaeseung-bae Apr 2, 2024
0f2f8e9
Merge branch 'main' into fix/handling-invalid-protocol-buffer-exception
jaeseung-bae Apr 4, 2024
f513257
Merge branch 'main' into fix/handling-invalid-protocol-buffer-exception
jaeseung-bae Apr 4, 2024
d3495d7
Merge branch 'main' into fix/handling-invalid-protocol-buffer-exception
jaeseung-bae Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ private static Status statusFromThrowable(Throwable t) {
if (t instanceof ClosedStreamException || t instanceof RequestTimeoutException) {
return Status.CANCELLED.withCause(t);
}
if (t instanceof InvalidProtocolBufferException) {
return Status.INVALID_ARGUMENT.withCause(t);
}
if (t instanceof UnprocessedRequestException ||
t instanceof IOException ||
t instanceof FailFastException) {
Expand Down
8 changes: 8 additions & 0 deletions grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/GrpcStatusTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.junit.jupiter.api.Test;

import com.google.api.gax.grpc.GrpcStatusCode;
import com.google.protobuf.InvalidProtocolBufferException;

import com.linecorp.armeria.client.circuitbreaker.CircuitBreaker;
import com.linecorp.armeria.client.circuitbreaker.FailFastException;
Expand All @@ -58,4 +59,11 @@ void failFastExceptionToUnavailableCode() {
.getCode())
.isEqualTo(Status.Code.UNAVAILABLE);
}

@Test
void invalidProtocolBufferExceptionToInvalidArgumentCode() {
assertThat(GrpcStatus.fromThrowable(new InvalidProtocolBufferException("Failed to parse message"))
.getCode())
.isEqualTo(Status.Code.INVALID_ARGUMENT);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add an integration test that sends invalid data described in the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll

}
16 changes: 16 additions & 0 deletions grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import com.linecorp.armeria.server.grpc.HttpJsonTranscodingQueryParamMatchRule;
import com.linecorp.armeria.testing.junit5.server.ServerExtension;

import io.grpc.Status.Code;
import io.grpc.stub.StreamObserver;
import testing.grpc.HttpJsonTranscodingTestServiceGrpc.HttpJsonTranscodingTestServiceBlockingStub;
import testing.grpc.HttpJsonTranscodingTestServiceGrpc.HttpJsonTranscodingTestServiceImplBase;
Expand Down Expand Up @@ -816,6 +817,21 @@ void shouldDenyMalformedJson() throws JsonProcessingException {
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@ParameterizedTest
@ValueSource(strings = {
"{\"int32Val\": 1.1}",
"{\"int64Val\": 2.2}",
"{\"uint32Val\": 3.3}",
"{\"uint64Val\": 4.4}"
})
void shouldDenyTypeMismatchedValue(String jsonContent)
throws JsonProcessingException {
final AggregatedHttpResponse response = jsonPostRequest(webClient, "/v1/echo/wrappers", jsonContent);
final JsonNode root = mapper.readTree(response.contentUtf8());
assertThat(response.headers().status()).isEqualTo(HttpStatus.BAD_REQUEST);
assertThat(root.get("code").asInt()).isEqualTo(Code.INVALID_ARGUMENT.value());
}

@Test
void shouldDenyMissingContentType() {
final String validJson = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"} }";
Expand Down
5 changes: 5 additions & 0 deletions grpc/src/test/proto/testing/grpc/transcoding.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ service HttpJsonTranscodingTestService {
rpc EchoWrappers(EchoWrappersRequest) returns (EchoWrappersResponse) {
option (google.api.http) = {
get: "/v1/echo/wrappers"

additional_bindings {
post: "/v1/echo/wrappers"
body: "value"
}
};
}

Expand Down