mengw15 commented on code in PR #6446:
URL: https://github.com/apache/texera/pull/6446#discussion_r3593529749
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala:
##########
@@ -152,18 +152,22 @@ class ComputingUnitAccessResource {
}
// TODO: add try except and check how to display error message in the
frontend
- val granteeId = userDao.fetchOneByEmail(email).getUid
- if (granteeId == null) {
+ val grantee = userDao.fetchOneByEmail(email)
+ if (grantee == null) {
throw new IllegalArgumentException("User with the given email does not
exist")
Review Comment:
One reality-check on the "(4xx)" part of the description: this service
registers no ExceptionMapper (`ComputingUnitManagingService` registers only
resources/auth/logging), so an `IllegalArgumentException` thrown from a
resource method still surfaces through Dropwizard's default
`LoggingExceptionMapper` as a 500 — and that mapper does not expose the
exception message to the client ("There was an error processing your request.
It has been logged…"). The NPE fix and the in-place upsert stand on their own,
but the user-visible outcome for a mistyped email is still an opaque 500.
Throwing `jakarta.ws.rs.BadRequestException` (400 + message) here — and
arguably in the permission checks above, which share the convention — would
deliver the actionable 4xx the description promises. The new tests call the
methods directly, so they'd only need the exception type swapped.
--
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]
