Remove* errors are ignored
bodhi opened this issue · 11 comments
(Apologies, I'd make a PR to fix it, but I'm in the middle of something else right now)
Each of RemoveAuthorize, RemoveAccess and RemoveRefresh are defined to return an error, but the error is ignored in server.FinishAccessRequest
The question is what the correct error handling would be. As far as I can see all three Remove* calls happen in Server.FinishAccessRequest after the new access token was created. The new token can be used and the user may not actually care about the failed cleanup operations.
However at the very least the error should simply be logged by osin. When implementing the Remove* functions as developer it is reasonable to assume that if I have to return an error, then it is actually handled somehow up the call stack, so I would not normally log the error myself. In the current state those errors just disapear.
OSIN shouldn't be concerned with logging. I agree that it is reasonable to expect the error to be handled somehow by OSIN. I don't see a good way to do so, so in my opinion the solution is to remove the return value from the interface. That forces implementations to deal with errors. @RangelReale will have to decide if a BC break is acceptable.
Edit: s/should/shouldn't/
Server could have an error handler that would allow customizing what is done with the errors. It could make a logging error handler implementation available if desired
My comment above had an unfortunate typo. I updated it accordingly. To clarify: OSIN is a library. It should not impose its opinions about logging on its users. Users of that library should decide if and how to do logging.
I was also considering removing the return value from the interface and
would prefer that very much.
Initially I was thinking this wouldn't work for to the BC break but
actually there are BC breaks listed at the top of the current change list
so hopefully this is an option :)
Peter Schultz notifications@github.com schrieb am Fr., 18. Nov. 2016,
17:52:
My comment above had an unfortunate typo. I updated it accordingly. To
clarify: OSIN is a library. It should not impose its opinions about logging
on its users. Users of that library should decide if and how to do logging.—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/RangelReale/osin/issues/135#issuecomment-261581446,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAsvTDPeXz0b4qZ9itYKR6xb5AZiZ69eks5q_dexgaJpZM4Kn5qm
.
the information is useful, don't remove it from the interface. If anything, I'd prefer a hook in server to decide what to do with non-fatal errors like that
The disadvantage I see in a hook is that there is no useful default
implementation and it is very easy to forget to implement a custom one.
Many users will probably not even be aware of it because tl;dr.
A missing return value on the other hand forces the implementing developer
to think about how to handle the error. The interface documentation should
assist at this point with some useful advice.
Jordan Liggitt notifications@github.com schrieb am Fr., 18. Nov. 2016,
18:04:
the information is useful, don't remove it from the interface. If
anything, I'd prefer a hook in server to decide what to do with non-fatal
errors like that—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/RangelReale/osin/issues/135#issuecomment-261584747,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAsvTGGHZNm4piGBvCsj2INo1jREYxORks5q_dqSgaJpZM4Kn5qm
.
Issues go stale after 90d of inactivity.
Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.
If this issue is safe to close now please do so with /close.
/lifecycle stale
Stale issues rot after 30d of inactivity.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.
If this issue is safe to close now please do so with /close.
/lifecycle rotten
/remove-lifecycle stale
Rotten issues close after 30d of inactivity.
Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.
/close
@openshift-bot: Closing this issue.
In response to this:
Rotten issues close after 30d of inactivity.
Reopen the issue by commenting
/reopen.
Mark the issue as fresh by commenting/remove-lifecycle rotten.
Exclude this issue from closing again by commenting/lifecycle frozen./close
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.