-
Notifications
You must be signed in to change notification settings - Fork 105
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
Improve error handling if process definition not found #287
Improve error handling if process definition not found #287
Conversation
Replace Runtime Exception with proper ResponseStatusException. Hint: the later will also improve/relate to PR camunda-community-hub#286 "Feature/error handler2"
@nitram509 awesome. Thank you for this contribution 🎉 I'll review the PR soon 🚀 |
@saig0 I did expect, a merge conflict will occur because other PRs made changes on the same code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🎉
@nitram509, it seems to work smoothly. Do you still need to adjust this PR?
|
||
model.put("resource", WARNING_NO_XML_RESOURCE_FOUND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could merge the default case in the below Optional
chain by using ifPresentOrElse()
.
processRepository
.findByKey(instance.getProcessDefinitionKey())
.map(this::getProcessResource)
.ifPresentOrElse(
resource -> model.put("resource", resource),
() -> model.put("resource", WARNING_NO_XML_RESOURCE_FOUND));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine for me ... seems to result in the same behavior and avoiding the exception handling is a bit more elegant.
@saig0 Looks good to me. Nothing to do from my side, so good to merge 👍 |
Since Zeebe Simple Monitor was build to support developers on local testing etc.,
I found myself in situations, where e.g. the process definition was no more available in the database.
This caused the BPMN diagram renderer to fail with exception and was confusing for me as user of Simple Monitor.
Unfortunately, I can't trace back, what caused the fact, that the process was not available, but this would be worth another PR, right? ;)
This PR just makes the renderer more robust in case of in-consistent data.
Additionally, I did replace the
throw new RuntimeException()
in the ViewControllerwith proper
throw new ResponseStatusException()
which will also improve/relate to PR #286 "Feature/error handler2".Just for the record, the error can be reproduced, if someone deletes the process definition from an existing database.
When requesting a single instance, e.g.
curl http://localhost:8082/views/instances/2251799814000236
the response looks like this