Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exceptions): add generic exception handler for UserException #737

Merged
merged 2 commits into from Aug 8, 2020

Conversation

marchello2000
Copy link
Contributor

No description provided.

@@ -134,6 +133,7 @@ private void storeException(
HttpServletRequest request, HttpServletResponse response, Exception ex) {
// store exception as an attribute of HttpServletRequest such that it can be referenced by
// GenericErrorController
logger.warn("Handled error in generic exception handler", ex);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this because otherwise there often no logs of the exception that caused this handler to fire...

@marchello2000 marchello2000 added the ready to merge Approved and ready for merge label Aug 8, 2020
@mergify mergify bot added the auto merged label Aug 8, 2020
@mergify mergify bot merged commit 5b40914 into spinnaker:master Aug 8, 2020
@ezimanyi
Copy link
Collaborator

@marchello2000 @cfieber : This looks to be breaking application creation (and potentially some other workflows that interact with front50).

The issue is that front50 defines a NotFoundException to extend UserException here. This means that with the new logic, front50 is returning a 400 if you request an object that does not exist.

In some places (particularly orca) we have logic that ignores a 404 response and handles a missing object as a special case. One example is application creation, where we first check if the application exists (expecting a 404) and create it if it's missing.

I'm not sure whether the fix is to:
(1) no longer have NotFoundException extend UserException (interestingly the generic kork one does not, but front50 makes its own)
(2) add a more specific handler for NotFoundException
(3) somehow reduce the scope of this change

Either way I think we should fix/revert this reasonably quickly as the integration tests are completely broken (and in this case I don't think it's a false positive...I'm unable to create an application in my test spinnaker either).

@ajordens
Copy link
Contributor

My vote is to jettison the front50-specific NotFoundException and favor the kork generic ones that should work correctly here.

cc: @marchello2000

@marchello2000
Copy link
Contributor Author

Ugh... @ezimanyi, thanks for catching this.
if i make the change in Front50 (cracking it open now) will we be ok? i will search who else extendes userexception

marchello2000 added a commit to marchello2000/front50 that referenced this pull request Aug 10, 2020
Follow up from spinnaker/kork#737 where I added a generic 400 response exception handler for UserException.
But `front50` needs to return an actual 404 (since callers, e.g. `orca` expect and handle it).
Kork already has a generic 404 handler for NotFoundException
@ezimanyi
Copy link
Collaborator

UserException is fairly new, so I suspect that there aren't too many things around that extend it. (I think that it's actually fairly recently that the front50 NotFoundException was changed to extend it.). So my feeling is that fixing this in front50 (and doing a quick check for similar cases in other services) should be fine.

@marchello2000
Copy link
Contributor Author

made this: spinnaker/front50#922 looking at other services nothing jumps out, (keel uses userexception a bunch and i've let keel devs know)

@marchello2000 marchello2000 deleted the mark/userexceptionhandler branch August 10, 2020 21:30
mergify bot pushed a commit to spinnaker/front50 that referenced this pull request Aug 10, 2020
Follow up from spinnaker/kork#737 where I added a generic 400 response exception handler for UserException.
But `front50` needs to return an actual 404 (since callers, e.g. `orca` expect and handle it).
Kork already has a generic 404 handler for NotFoundException
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants