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 hanging issue in the HTTP client after status code binding feature #1954

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Fix hanging issue in the HTTP client after status code binding feature #1954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2ae4bc
Fix hanging issue with the status code binding feature
TharmiganK Apr 19, 2024
96ac444
Move the status code binding feature to a separate client
TharmiganK Apr 19, 2024
e12a66a
Fix test case
TharmiganK Apr 19, 2024
d07fa66
Merge branch 'master' into status-code-binding-fix
TharmiganK Apr 19, 2024
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
185 changes: 10 additions & 175 deletions ballerina-tests/http-client-tests/tests/sc_res_binding_tests.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,53 +240,17 @@ service /api on new http:Listener(statusCodeBindingPort2) {
}
}

final http:Client albumClient = check new (string `localhost:${statusCodeBindingPort2}/api`);
final http:StatusCodeClient albumClient = check new (string `localhost:${statusCodeBindingPort2}/api`);

@test:Config {}
function testGetSuccessStatusCodeResponse() returns error? {
Album album = check albumClient->/albums/'1;
Album expectedAlbum = albums.get("1");
test:assertEquals(album, expectedAlbum, "Invalid album returned");

AlbumFound albumFound = check albumClient->get("/albums/1");
Album expectedAlbum = albums.get("1");
test:assertEquals(albumFound.body, expectedAlbum, "Invalid album returned");
test:assertEquals(albumFound.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(albumFound.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(albumFound.mediaType, "application/json", "Invalid media type");

http:Response res = check albumClient->/albums/'1;
test:assertEquals(res.statusCode, 200, "Invalid status code");
json payload = check res.getJsonPayload();
album = check payload.fromJsonWithType();
test:assertEquals(album, expectedAlbum, "Invalid album returned");

Album|AlbumFound res1 = check albumClient->get("/albums/1");
if res1 is AlbumFound {
test:assertEquals(res1.body, expectedAlbum, "Invalid album returned");
test:assertEquals(res1.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(res1.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res1.mediaType, "application/json", "Invalid media type");
} else {
test:assertFail("Invalid response type");
}

AlbumFound|Album res2 = check albumClient->/albums/'1;
if res2 is AlbumFound {
test:assertEquals(res2.body, expectedAlbum, "Invalid album returned");
test:assertEquals(res2.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(res2.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res2.mediaType, "application/json", "Invalid media type");
} else {
test:assertFail("Invalid response type");
}

Album|AlbumNotFound res3 = check albumClient->get("/albums/1");
if res3 is Album {
test:assertEquals(res3, expectedAlbum, "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumFound|AlbumNotFound res4 = check albumClient->/albums/'1;
if res4 is AlbumFound {
test:assertEquals(res4.body, expectedAlbum, "Invalid album returned");
Expand All @@ -297,37 +261,10 @@ function testGetSuccessStatusCodeResponse() returns error? {
test:assertFail("Invalid response type");
}

Album|AlbumFound|AlbumNotFound res5 = check albumClient->get("/albums/1");
if res5 is AlbumFound {
test:assertEquals(res5.body, expectedAlbum, "Invalid album returned");
test:assertEquals(res5.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(res5.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res5.mediaType, "application/json", "Invalid media type");
} else {
test:assertFail("Invalid response type");
}

Album|AlbumNotFound|http:Response res6 = check albumClient->/albums/'1;
if res6 is Album {
test:assertEquals(res6, expectedAlbum, "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumNotFound|http:Response res7 = check albumClient->get("/albums/1");
if res7 is http:Response {
test:assertEquals(res.statusCode, 200, "Invalid status code");
payload = check res.getJsonPayload();
album = check payload.fromJsonWithType();
test:assertEquals(album, expectedAlbum, "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumNotFound|error res8 = albumClient->/albums/'1;
if res8 is error {
test:assertTrue(res8 is http:PayloadBindingError);
test:assertEquals(res8.message(), "incompatible http_client_tests:AlbumNotFound found for response with 200",
test:assertTrue(res8 is http:StatusCodeResponseBindingError);
test:assertEquals(res8.message(), "incompatible AlbumNotFound found for response with 200",
"Invalid error message");
error? cause = res8.cause();
if cause is error {
Expand All @@ -347,129 +284,27 @@ function testGetFailureStatusCodeResponse() returns error? {
test:assertEquals(albumNotFound.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(albumNotFound.mediaType, "application/json", "Invalid media type");

http:Response res = check albumClient->get("/albums/4");
test:assertEquals(res.statusCode, 404, "Invalid status code");
json payload = check res.getJsonPayload();
ErrorMessage errorMessage = check payload.fromJsonWithType();
test:assertEquals(errorMessage, expectedErrorMessage, "Invalid error message");

Album|AlbumNotFound res1 = check albumClient->/albums/'4;
if res1 is AlbumNotFound {
test:assertEquals(res1.body, expectedErrorMessage, "Invalid error message");
test:assertEquals(res1.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(res1.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res1.mediaType, "application/json", "Invalid media type");
} else {
test:assertFail("Invalid response type");
}

AlbumNotFound|http:Response res2 = check albumClient->get("/albums/4");
if res2 is AlbumNotFound {
test:assertEquals(res2.body, expectedErrorMessage, "Invalid error message");
test:assertEquals(res2.headers.user\-id, "user-1", "Invalid user-id header");
test:assertEquals(res2.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res2.mediaType, "application/json", "Invalid media type");
} else {
test:assertFail("Invalid response type");
}

Album|http:Response res3 = check albumClient->/albums/'4;
if res3 is http:Response {
test:assertEquals(res3.statusCode, 404, "Invalid status code");
payload = check res3.getJsonPayload();
errorMessage = check payload.fromJsonWithType();
test:assertEquals(errorMessage, expectedErrorMessage, "Invalid error message");
} else {
test:assertFail("Invalid response type");
}

http:Response|AlbumFound res4 = check albumClient->get("/albums/4");
if res4 is http:Response {
test:assertEquals(res4.statusCode, 404, "Invalid status code");
payload = check res4.getJsonPayload();
errorMessage = check payload.fromJsonWithType();
test:assertEquals(errorMessage, expectedErrorMessage, "Invalid error message");
} else {
test:assertFail("Invalid response type");
}

Album|error res5 = albumClient->/albums/'4;
if res5 is error {
test:assertTrue(res5 is http:ClientRequestError);
test:assertEquals(res5.message(), "Not Found", "Invalid error message");
test:assertEquals(res5.detail()["statusCode"], 404, "Invalid status code");
test:assertEquals(res5.detail()["body"], expectedErrorMessage, "Invalid error message");
if res5.detail()["headers"] is map<string[]> {
map<string[]> headers = check res5.detail()["headers"].ensureType();
test:assertEquals(headers.get("user-id")[0], "user-1", "Invalid user-id header");
test:assertEquals(headers.get("req-id")[0], "1", "Invalid req-id header");
}

} else {
test:assertFail("Invalid response type");
}

AlbumFound|error res6 = albumClient->get("/albums/4");
if res6 is error {
test:assertTrue(res6 is http:ClientRequestError);
test:assertEquals(res6.message(), "Not Found", "Invalid error message");
test:assertTrue(res6 is http:StatusCodeResponseBindingError);
test:assertEquals(res6.message(), "incompatible AlbumFound found for response with 404", "Invalid error message");
test:assertEquals(res6.detail()["statusCode"], 404, "Invalid status code");
test:assertEquals(res6.detail()["body"], expectedErrorMessage, "Invalid error message");
if res6.detail()["headers"] is map<string[]> {
map<string[]> headers = check res6.detail()["headers"].ensureType();
test:assertEquals(headers.get("user-id")[0], "user-1", "Invalid user-id header");
test:assertEquals(headers.get("req-id")[0], "1", "Invalid req-id header");
} else {
test:assertFail("Invalid headers type");
}

} else {
test:assertFail("Invalid response type");
}
}

@test:Config {}
function testUnionPayloadBindingWithStatusCodeResponse() returns error? {
Album|AlbumNotFound|map<json>|json res1 = check albumClient->/albums/'1;
if res1 is Album {
test:assertEquals(res1, albums.get("1"), "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

map<json>|AlbumNotFound|Album|json res2 = check albumClient->get("/albums/1");
if res2 is map<json> {
test:assertEquals(res2, albums.get("1"), "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

Album|MockAlbum|AlbumNotFound res3 = check albumClient->/albums/'1;
if res3 is Album {
test:assertEquals(res3, albums.get("1"), "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

MockAlbum|Album|AlbumNotFound res4 = check albumClient->get("/albums/1");
if res4 is MockAlbum {
test:assertEquals(res4, {...albums.get("1"), "type": "mock"}, "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumUnion1|AlbumNotFound res5 = check albumClient->/albums/'1;
if res5 is Album {
test:assertEquals(res5, albums.get("1"), "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumUnion2|AlbumNotFound res6 = check albumClient->get("/albums/1");
if res6 is MockAlbum {
test:assertEquals(res6, {...albums.get("1"), "type": "mock"}, "Invalid album returned");
} else {
test:assertFail("Invalid response type");
}

AlbumFound|AlbumNotFound|AlbumFoundMock1 res7 = check albumClient->/albums/'1;
if res7 is AlbumFound {
test:assertEquals(res7.body, albums.get("1"), "Invalid album returned");
Expand Down Expand Up @@ -611,7 +446,7 @@ function testStatusCodeBindingWithConstraintsSuccess() returns error? {
test:assertEquals(res1.headers.req\-id, 1, "Invalid req-id header");
test:assertEquals(res1.mediaType, "application/json", "Invalid media type");

AlbumFoundWithConstraints|AlbumFound|Album|http:Response res2 = check albumClient->get("/albums/2");
AlbumFoundWithConstraints|AlbumFound res2 = check albumClient->get("/albums/2");
if res2 is AlbumFoundWithConstraints {
test:assertEquals(res2.body, albums.get("2"), "Invalid album returned");
test:assertEquals(res2.headers.user\-id, "user-1", "Invalid user-id header");
Expand Down Expand Up @@ -652,7 +487,7 @@ function testStatusCodeBindingWithConstraintsFailure() returns error? {
test:assertFail("Invalid response type");
}

AlbumFoundWithInvalidConstraints3|Album|error res3 = albumClient->/albums/'2;
AlbumFoundWithInvalidConstraints3|error res3 = albumClient->/albums/'2;
if res3 is error {
test:assertTrue(res3 is http:MediaTypeValidationError);
test:assertEquals(res3.message(), "media-type binding failed: Validation failed for " +
Expand Down
2 changes: 1 addition & 1 deletion ballerina-tests/http-client-tests/tests/sc_res_binding_with_all_status_codes_tests.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import ballerina/http;
import ballerina/test;

final http:Client statusCodeBindingClient1 = check new (string `localhost:${statusCodeBindingPort1}`);
final http:StatusCodeClient statusCodeBindingClient1 = check new (string `localhost:${statusCodeBindingPort1}`);

service /api on new http:Listener(statusCodeBindingPort1) {

Expand Down
71 changes: 61 additions & 10 deletions ballerina/http_client_endpoint.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
} external;

private isolated function processPost(string path, RequestMessage message, TargetType targetType,
string? mediaType, map<string|string[]>? headers) returns Response|StatusCodeResponse|anydata|ClientError {
string? mediaType, map<string|string[]>? headers) returns Response|anydata|ClientError {
Request req = check buildRequest(message, mediaType);
populateOptions(req, mediaType, headers);
Response|ClientError response = self.httpClient->post(path, req);
Expand Down Expand Up @@ -130,7 +130,7 @@
} external;

private isolated function processPut(string path, RequestMessage message, TargetType targetType,
string? mediaType, map<string|string[]>? headers) returns Response|StatusCodeResponse|anydata|ClientError {
string? mediaType, map<string|string[]>? headers) returns Response|anydata|ClientError {
Request req = check buildRequest(message, mediaType);
populateOptions(req, mediaType, headers);
Response|ClientError response = self.httpClient->put(path, req);
Expand Down Expand Up @@ -172,7 +172,7 @@
} external;

private isolated function processPatch(string path, RequestMessage message, TargetType targetType,
string? mediaType, map<string|string[]>? headers) returns Response|StatusCodeResponse|anydata|ClientError {
string? mediaType, map<string|string[]>? headers) returns Response|anydata|ClientError {
Request req = check buildRequest(message, mediaType);
populateOptions(req, mediaType, headers);
Response|ClientError response = self.httpClient->patch(path, req);
Expand Down Expand Up @@ -214,7 +214,7 @@
} external;

private isolated function processDelete(string path, RequestMessage message, TargetType targetType,
string? mediaType, map<string|string[]>? headers) returns Response|StatusCodeResponse|anydata|ClientError {
string? mediaType, map<string|string[]>? headers) returns Response|anydata|ClientError {
Request req = check buildRequest(message, mediaType);
populateOptions(req, mediaType, headers);
Response|ClientError response = self.httpClient->delete(path, req);
Expand Down Expand Up @@ -277,7 +277,7 @@
} external;

private isolated function processGet(string path, map<string|string[]>? headers, TargetType targetType)
returns Response|StatusCodeResponse|anydata|ClientError {
returns Response|anydata|ClientError {
Request req = buildRequestWithHeaders(headers);
Response|ClientError response = self.httpClient->get(path, message = req);
if observabilityEnabled && response is Response {
Expand Down Expand Up @@ -313,7 +313,7 @@
} external;

private isolated function processOptions(string path, map<string|string[]>? headers, TargetType targetType)
returns Response|StatusCodeResponse|anydata|ClientError {
returns Response|anydata|ClientError {
Request req = buildRequestWithHeaders(headers);
Response|ClientError response = self.httpClient->options(path, message = req);
if observabilityEnabled && response is Response {
Expand All @@ -340,7 +340,7 @@

private isolated function processExecute(string httpVerb, string path, RequestMessage message,
TargetType targetType, string? mediaType, map<string|string[]>? headers)
returns Response|StatusCodeResponse|anydata|ClientError {
returns Response|anydata|ClientError {
Request req = check buildRequest(message, mediaType);
populateOptions(req, mediaType, headers);
Response|ClientError response = self.httpClient->execute(httpVerb, path, req);
Expand All @@ -363,7 +363,7 @@
} external;

private isolated function processForward(string path, Request request, TargetType targetType)
returns Response|StatusCodeResponse|anydata|ClientError {
returns Response|anydata|ClientError {
Response|ClientError response = self.httpClient->forward(path, request);
if observabilityEnabled && response is Response {
addObservabilityInformation(path, request.method, response.statusCode, self.url);
Expand Down Expand Up @@ -677,16 +677,67 @@
}
}

isolated function createStatusCodeResponseBindingError(boolean generalError, int statusCode, string reasonPhrase,
map<string[]> headers, anydata body = ()) returns ClientError {

Check warning on line 681 in ballerina/http_client_endpoint.bal

View check run for this annotation

Codecov / codecov/patch

ballerina/http_client_endpoint.bal#L681 

Added line #L681 was not covered by tests
if generalError {
return error StatusCodeResponseBindingError(reasonPhrase, statusCode = statusCode, headers = headers, body = body);
}
if 100 <= statusCode && statusCode <= 399 {
return error StatusCodeBindingSuccessError(reasonPhrase, statusCode = statusCode, headers = headers, body = body);
} else if 400 <= statusCode && statusCode <= 499 {
return error StatusCodeBindingClientRequestError(reasonPhrase, statusCode = statusCode, headers = headers, body = body);
} else {
return error StatusCodeBindingRemoteServerError(reasonPhrase, statusCode = statusCode, headers = headers, body = body);
}
}

isolated function processResponse(Response|ClientError response, TargetType targetType, boolean requireValidation)
returns Response|anydata|StatusCodeResponse|ClientError {
returns Response|anydata|ClientError {
if targetType is typedesc<Response> || response is ClientError {
return response;
}
int statusCode = response.statusCode;
if 400 <= statusCode && statusCode <= 599 {
string reasonPhrase = response.reasonPhrase;
map<string[]> headers = getHeaders(response);
anydata|error payload = getPayload(response);
if payload is error {
if payload is NoContentError {
return createResponseError(statusCode, reasonPhrase, headers);
}
return error PayloadBindingClientError("http:ApplicationResponseError creation failed: " + statusCode.toString() +
" response payload extraction failed", payload);
} else {
return createResponseError(statusCode, reasonPhrase, headers, payload);
}
}
if targetType is typedesc<anydata> {
anydata payload = check performDataBinding(response, targetType);
if requireValidation {
return performDataValidation(payload, targetType);
}
return payload;
} else {
panic error GenericClientError("invalid payload target type");
}
}

isolated function processResponseNew(Response|ClientError response, typedesc<StatusCodeResponse> targetType, boolean requireValidation)
returns StatusCodeResponse|ClientError {
if response is ClientError {
return response;
}
return externProcessResponse(response, targetType, requireValidation);
return externProcessResponseNew(response, targetType, requireValidation);
}

isolated function externProcessResponse(Response response, TargetType targetType, boolean requireValidation)
returns Response|anydata|StatusCodeResponse|ClientError = @java:Method {
'class: "io.ballerina.stdlib.http.api.nativeimpl.ExternResponseProcessor",
name: "processResponse"
} external;

isolated function externProcessResponseNew(Response response, typedesc<StatusCodeResponse> targetType, boolean requireValidation)
returns StatusCodeResponse|ClientError = @java:Method {
'class: "io.ballerina.stdlib.http.api.nativeimpl.ExternResponseProcessor",
name: "processResponse"
} external;
Loading
Loading