smolnar82 commented on PR #1327:
URL: https://github.com/apache/knox/pull/1327#issuecomment-5067884198
There is one small change I'll open a follow=up PR for:
in line 122 you catch RuntimeException:
```
} catch (RuntimeException e) {
return errorResponse(Response.Status.INTERNAL_SERVER_ERROR,
"storage_error",
"Failed to register issuer");This would indicate either a JDBC
issue or if max limit exceeded.
```
I think it's better to separate those 2 and return a clean error message to
the clients in case they face the limit.
Another finding (sort of related):
Untyped `Map<String,Object> + blind casts` causes client type errors surface
as 500s. This is the main one. Lines 100/116:
```
final String rawUrl = (String) parsed.get("issuerUrl");
...
final String clusterName = (String) parsed.get("clusterName");
```
A parseable-but-wrong-typed body like `{"issuerUrl": 123}` or
`{"clusterName": {}}` throws `ClassCastException` -> caught by `catch
(RuntimeException)` at line 122 -> `500 storage_error "Failed to register
issuer"`. That's a client mistake (should be `400 invalid_request`) misreported
as a server storage failure. A typed request DTO deserialized by JAX-RS
(`RegisterIssuerRequest`, for instance) would eliminate the casts, give
automatic `400`s on type mismatch, and read more idiomatically. The current
hand-rolled `Map` parse is the root of this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
