Support exceptions and re-use of business logic for UI and APIs#1240
Support exceptions and re-use of business logic for UI and APIs#1240vboctor merged 29 commits intomantisbt:masterfrom
Conversation
|
@cproensa With this proposal script files will validate its input, construct an array of command data and run a command. This enable plugins to hook on a semantic level rather than on BugData updates. Commands can also include before state in execution results as needed. |
|
Independent of whether we go with this approach or another one, I would like to gradually get rid of all business logic of action pages and move it to a common place that is used with REST APIs and web UI. One way to achieve that is to get rid of the action pages and have the web UI build fully on top of the REST API. But it currently the APIs trigger errors in an unfriendly way to REST API (rather than throwing exceptions) and hence REST API has to duplicate all the checks to avoid errors from underlying APIs. So in all cases, we need to refactor, so the goal of this discussion is to figure out the model to refactor to. |
|
I like where this is heading. Some minor comments inline. |
| * | ||
| * @return void | ||
| */ | ||
| function execute() { |
There was a problem hiding this comment.
This implementation means that callers would need to invoke validate() by themselves. I would suggest moving the validate call inside the execute function, so that it's done automatically.
There was a problem hiding this comment.
The intention was to call validate() from within execute(), but I forget to call it.
| function validate() { | ||
| # Validate issue id | ||
| if( !isset( $this->data['issue_id'] ) ) { | ||
| throw new CommandException( HTTP_STATUS_BAD_REQUEST, 'issue_id missing', ERROR_GPC_VAR_NOT_FOUND ); |
There was a problem hiding this comment.
It might be worth extracting out a InvalidDataCommandException or similar that returns the 400 Bad Request status, since it's going to be a very often encountered situation.
There was a problem hiding this comment.
Makes sense. Will revisit the exception hierarchy or having helper methods to create such exception instances. I have already done that for the REST/SOAP APIs.
More thoughts to consider: |
@cproensa My thinking is that we have a command per user action / intent. A command can't use other commands. However, commands can build on re-usable APIs to capture common knowledge. |
This goes both ways. Let's have a status update, for example from "assigned" to "closed" status. And still, this does not remove the need to ensure the correct execution of BugData events (#1098), if you think it's related. |
|
@cproensa I have revised the pattern and got the issue monitor command working, though I didn't implement yet the exception triggering and handling.
Think of the Command as a Controller but is not tied to HTTP concepts. It is a layer above our current core APIs that captures a set of business logic that needs to be executed to achieve a higher level action. Hope this simplifies the model and make it easier to transition to from our current implementation. @rombert The validate() method is automatically called now, which was missed from my initial iteration. We could technically just have a process method, but the current model provides separation that can be beneficial in the future. All - let me know your thoughts in general and specifically if you have thoughts about exception modeling / handling, which is my next step. |
|
API overall looks good to me, thanks. As for exception handling, I usually prefer letting exceptions bubble up and having a top-level exception handler that displays the error page and potentially logs the error. This avoids having to handle the exception in each page and also is more flexible depending on the invocation context. We can easily swap different behaviours, for instance static HTML page vs JSON response. |
|
I did some thinking about error handling. I think we can use a model where:
The advantage of this model is that we can gradually make the transition to exceptions while still having a system that works end to end even with unmodified code calling into a method that throws an exception instead of trigger_error() before. As I was looking into this, I also found a couple of issues that I reported in the bug tracker. These are 23705 and 23706. Would be good to get feedback on these. |
|
@rombert @cproensa @dregad @atrol I have done the work to make the error handling work end-to-end, so you can get a full picture of how things work. I have the following work:
The above is minor and shouldn't affect the review. Once the changes are checked-in, we can gradually convert scenarios as needed. For example, I will have follow up changes to reduce code redundancy between APIs logic and core APIs for existing and new APIs. |
|
Few more changes:
|
|
Feedback from @atrol that was provided in #1253
|
|
I think feedback on such work item comes in three buckets:
I have opened this PR 25 days ago and I tried to start with the simple big picture so as to get feedback in bucket 1 (the code was not even fully implemented, just a skeleton at that point). Got some feedback and progressed further. I think we should quickly close on 1 so I can invest further and make progress. I have done a fair bit of testing for 2, and can do more. My goal was to get it in early after 2.9.0 and test, but I was waiting on review from more team members. As for the risk and timing, I have tried not to boil the ocean in this PR, and just support the basic error handling approach and exercise it with an end-to-end scenario. So even though I expect to gradually use the new model in more code, I have tried to keep it to few code paths to keep the change scoped. As for the release timing, I think given the end of year vacations, we could delay or skip the end of December release. But we can get to that once you have a look at the PR and give it few minutes or trying on your machines.
I'm not sure I want to put a bunch of big PRs into a single release, I would rather trickle them in as they are ready. |
|
I have lost track on this. |
The goal of this PR is to provide a way to share business logic between Web UI and REST API. In order to achieve that, we have to change error handling to bubble up errors instead of output to the screen. The PR also includes an example of adding users to monitor list that was changed to re-use a command between REST API and Web UI. The PR also does that while maintaining backward compatibility. |
| /** | ||
| * @var integer | ||
| */ | ||
| private $loggedInUserId; |
| define( 'ERROR_USER_CURRENT_PASSWORD_MISMATCH', 812 ); | ||
| define( 'ERROR_USER_EMAIL_NOT_UNIQUE', 813 ); | ||
| define( 'ERROR_USER_BY_EMAIL_NOT_FOUND', 814 ); | ||
| define( 'ERROR_USER_BY_REALNAME_NOT_FOUND', 814 ); |
Use Command Pattern to achieve the following: - Re-using of business logic across Web UI, REST API, and SOAP API. - Having commands that execute a specific intent e.g. assign, close, monitor rather than just update. - Enable core code to hook into such commands to handle side effects like email messages. - Enable plugins code to hook in such commands to modify changes, block the change, post to slack, etc. - Use exceptions that can be surfaced to APIs or Web UI. - Enable having a clean model using ADODB or some higher level ORM library that is used by commands.
- Simplify commands to just encapsulate business logic. - Commands no longer have hooks, use existing events. - Remove context and use standard APIs. - Error handling not implemented. The command is used from UI, but can easily be used from APIs.
The code assumes that such errors will halt executions, so such errors shouldn’t ever resume. Fixes #23706
- Add MantisException, ServiceException, and ClientException. - Add unhandled exception handler that forward to trigger error on error mode is display. - Force termination on case of E_USER_ERROR (aliased to ERROR in Mantis). This allows replacing trigger_error( …, ERROR) with throwing the appropriate exception while maintaining backward compatibility for UI code.
- Map errors to API error buckets (e.g. not found, bad request, forbidden, etc) - Implement unhandled exception handler for APIs. - Remove redundant check in Issue Get and rely on exceptions instead. - bug_ensure_exists() and getting bug now triggers an exception in case bug not found instead of trigger_error().
This exception should be used for bad state in db and configs.
The array can contain: id, name, realname, email.
Fixes #23710
Enforce for E_ERROR and E_RECOVERABLE_ERROR and not just E_USER_ERROR. Updated documentation, config_defaults_inc.php samples and defaults. Fixes #23706
- Specific route handlers don’t need to catch exceptions just to return them in the response. - In case of errors include a json with Mantis error code, message, and localized message for thrown exceptions. - Convert RestFault and throw, though in this case only include the message in json, no code or localized message. LegacyApiFaultException can be removed once all APIs throw proper exceptions instead of returning RestFault. Fixes #23714
This is to support the returning of SoapFault and throwing Mantis exceptions.
This is running code demonstrating a command for adding users to monitor an issue. However, exceptions are not implemented yet, invalid users are silently skipped for now.
The goals of this pattern are: