Fix REST responses for failed Admin operations #2291
Conversation
XN137
force-pushed
the
fix-admin-rest-responses
branch
from
c7b3a45 to
339ac46
Compare
| @@ -640,7 +661,7 @@ public Response addGrantToCatalogRole( | |||
| .log("Don't know how to handle privilege grant: {}", grantRequest); | |||
| return Response.status(Response.Status.BAD_REQUEST).build(); | |||
| } | |||
| return Response.status(Response.Status.CREATED).build(); | |||
| return toResponse(result, Response.Status.CREATED); | |||
There was a problem hiding this comment.
This means that errors were not propagated to clients, right?
There was a problem hiding this comment.
yeah, some methods do throw exceptions internally (and those get propagated to clients correctly) but the returned boolean value of the methods was often ignored.
as part of this PR we have to fix a test in PolarisManagementServiceIntegrationTest.java which was silently failing before.
XN137
force-pushed
the
fix-admin-rest-responses
branch
3 times, most recently
from
9cf05b2 to
d9a55c1
Compare
the `boolean` return values of many methods in `PolarisAdminService` were often simply not getting used at all, thus the REST api returned success in those cases even though the `PrivilegeResult` was marked as failed. due to this fix a silently failing test now needs to be adjusted. we return the `PrivilegeResult` instead of a `boolean` to give the client at least some indication of what has gone wrong on the server side. note that some of the other operations were throwing Expcetions already, which are already reported back correctly to the client.
XN137
force-pushed
the
fix-admin-rest-responses
branch
from
d9a55c1 to
56ad6dc
Compare
| .returns(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), Response::getStatus); | ||
| assertThat(response.hasEntity()).isTrue(); | ||
| ErrorResponse errorResponse = response.readEntity(ErrorResponse.class); | ||
| assertThat(errorResponse.message()).contains("Operation failed: GRANT_NOT_FOUND"); |
There was a problem hiding this comment.
there could be an argument that GRANT_NOT_FOUND should be a 404 instead of internal server error but it would mean we need to translate all BaseResult.ReturnStatus values to a proper HTTP code.
i think this can be figured out in a followup and we should first stop silently failing in those cases.
There was a problem hiding this comment.
Hm - HTTP/500 feels odd, as it's rather a user (argument) error. HTTP/400 would be better IMHO.
There was a problem hiding this comment.
ok switched to HTTP/400
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
If nobody objects, I'll merge later today. |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
the
booleanreturn values of many methods inPolarisAdminServicewere often simply not getting used at all, thus the REST api returned
success in those cases even though the
PrivilegeResultwas markedas failed.
due to this fix a silently failing test now needs to be adjusted.
we return the
PrivilegeResultinstead of abooleanto give theclient at least some indication of what has gone wrong on the server
side.
note that some of the other operations were throwing Expcetions already,
which are already reported back correctly to the client.