fix: report a missing refresh token as AuthException - #297
Conversation
retryLogin() signalled "no refresh JWT available" with a bare java.lang.Exception. Exceptions.translate has no case for that, so it fell through to the generic wrapper and the caller saw a DgraphException reporting INTERNAL -- an authentication failure presented as an internal error, and the one gap left in the typed exception hierarchy added in 25.0.0. Fail with AuthException carrying Status.UNAUTHENTICATED instead. translate passes DgraphException subclasses through untouched, so this reaches the caller intact. Retryability is unchanged: AuthException inherits isRetryable() == false, exactly as the generic DgraphException did. Callers catching DgraphException or StatusRuntimeException are unaffected. Only code that distinguishes AuthException sees a difference, which is the point. Add RetryLoginTest, covering both the direct refresh call and the path a user actually hits -- an expired token with no way to refresh it, surfacing through runWithRetries. Neither test needs a server: the missing-token check returns before any RPC. Both fail before this change.
|
No CI ran on this PR, and the green state is misleading. Marking it draft until that is fixed. Both workflows are gated on the base branch:
This PR targets What I verified locally instead
To land this Merge #294 first, then retarget this PR to Separately: widening those two triggers (for example to |
Merge path
Draft until the test suite has actually run on it.
ci-dgraph4j-tests.ymlandtrunk.ymlboth gate theirpull_requesttrigger onbranches: main, so while this PR targets a feature branch it gets CodeQL but neither the test suite nor the linters. GitHub still reports itMERGEABLEandCLEAN, because absent checks cannot fail — judge it by which checks are present, not by its merge state.Merging bottom-up closes that gap with no changes to CI:
main.mainautomatically. The repo hasdelete_branch_on_mergeenabled, and GitHub retargets open PRs based on a deleted head branch to the merged PR's base.ready_for_reviewis in the workflow's trigger list, so with the base nowmainthe full suite and the linters run here.gh stack merge 298would instead land both PRs atomically, which is faster but merges this one without the suite ever running on it — the post-mergepush: mainrun would catch a problem after the fact rather than before. #299 ("ci: run tests and linters on pull requests to any base branch") removes the base-branch filter that causes this, and once it merges the checks appear here and either path is validated. Until then, prefer the sequence above.Description
retryLogin()signalled "no refresh JWT available" with a barejava.lang.Exception.Exceptions.translatehas no case for that, so it fell through to the generic wrapper and the caller saw aDgraphExceptionreportingINTERNAL— an authentication failure presented as an internal error. It is the one gap left in the typed exception hierarchy added in 25.0.0, which already definesAuthExceptionfor exactly this condition.This fails with
AuthExceptioncarryingStatus.UNAUTHENTICATEDinstead.translatepassesDgraphExceptionsubclasses through untouched, so it reaches the caller intact.Compatibility
AuthExceptioninheritsisRetryable() == false, exactly as the genericDgraphExceptiondid, sowithRetrybehaves identically.catch (DgraphException)andcatch (StatusRuntimeException)are unaffected —AuthExceptionis a subclass of both.AuthExceptionsees a difference, which is the point of the change.INTERNALtoUNAUTHENTICATED. Code switching on the gRPC status code of this specific failure would observe that.Tests
RetryLoginTestcovers two cases, neither needing a server, since the missing-token check returns before any RPC is issued:refreshWithoutATokenFailsWithAuthExceptionAuthExceptionandUNAUTHENTICATEDexpiredTokenWithNoRefreshSurfacesAuthExceptionAuthExceptionthroughrunWithRetriesBoth fail before this change: the first sees a raw
java.lang.Exception, the second seesDgraphException: INTERNAL.Checklist
CHANGELOG.mdfile describing and linking tothis PR
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.