|
| 1 | +# Software Design Document |
| 2 | + |
| 3 | +Reference: Exception Handling<br> Authors: Andre Briggs, Dennis Seah |
| 4 | + |
| 5 | +| Revision | Date | Author | Remarks | |
| 6 | +| -------: | ------------ | ----------- | -------------------------------- | |
| 7 | +| 0.1 | Mar-04, 2020 | Dennis Seah | Initial Draft | |
| 8 | +| 1.0 | Mar-10, 2020 | Dennis Seah | Incorporate comments from Yvonne | |
| 9 | + |
| 10 | +## 1. Overview |
| 11 | + |
| 12 | +User of `spk` command line tool needs to have a better understanding of what |
| 13 | +happened when command does not execute successfully. Currently, we output error |
| 14 | +message to logs (using `winston logger`); and it is difficult to go through the |
| 15 | +log entries to pinpoint the error. This is because error, warning and |
| 16 | +information messages are interleaved. |
| 17 | + |
| 18 | +The other problem is that in some cases, we drop the existing error message and |
| 19 | +throw a new one. This results in losing the root cause of the error. For |
| 20 | +instance |
| 21 | + |
| 22 | +``` |
| 23 | +try { |
| 24 | + ... |
| 25 | +} catch (err) { |
| 26 | + if (err.message.indexOf("VS123456")) { |
| 27 | + throw Error("Azure WebAPI call is not completed.") |
| 28 | + } |
| 29 | + throw err; |
| 30 | +} |
| 31 | +``` |
| 32 | + |
| 33 | +In some cases, we ignore the exception and move on. For instance |
| 34 | + |
| 35 | +``` |
| 36 | +try { |
| 37 | + ... |
| 38 | +} catch (err) { |
| 39 | + logger.error(err); |
| 40 | +} |
| 41 | +``` |
| 42 | + |
| 43 | +Reader of the code has a hard time understanding why error is ignored. |
| 44 | + |
| 45 | +## 2. Out of Scope |
| 46 | + |
| 47 | +This document shall not cover exceptions/errors that are thrown by third party |
| 48 | +software and/or from Microsoft's client API (e.g. from Azure DevOps). |
| 49 | + |
| 50 | +## 3. Proposal |
| 51 | + |
| 52 | +### 3.1 Documenting harmless exception. |
| 53 | + |
| 54 | +There are cases when we intentional ignore exceptions. For instance |
| 55 | + |
| 56 | +``` |
| 57 | + try { |
| 58 | + return !!(await coreAPI.getProject(name)); |
| 59 | + } catch (_) { |
| 60 | + // exception is thrown because project is not found |
| 61 | + // this is not an error condition because we are |
| 62 | + // using this call to check if project exist or not. |
| 63 | + logger.info(`Unable to get project, ${name}`); |
| 64 | + } |
| 65 | + return false; |
| 66 | +``` |
| 67 | + |
| 68 | +The proposal is to have adequate comments in the code to explain why exception |
| 69 | +is caught and ignored. Typically, we do not advise people to do this unless we |
| 70 | +are sure about it. Taking the example above, we may have authentication and |
| 71 | +authorization failures too. |
| 72 | + |
| 73 | +### 3.2 Maintain Exception Chain. |
| 74 | + |
| 75 | +Most of the time, we have a good idea of issues when we have the complete |
| 76 | +exception chain. For instance |
| 77 | + |
| 78 | +``` |
| 79 | + o Command excecute function calls |
| 80 | + o Pipeline service helper function which calls |
| 81 | + o Azdo pipeline API via web client |
| 82 | +``` |
| 83 | + |
| 84 | +The proposal is to have a way to maintain this exception chain; and preserve the |
| 85 | +statusCode. Taking the above example and presuming that authentication failed in |
| 86 | +web client call. We would like to have a JSON object like this |
| 87 | + |
| 88 | +``` |
| 89 | +{ |
| 90 | + "statusCode": 101, |
| 91 | + "message": "Execution of spk project install-lifecycle-pipeline could not be completed.", |
| 92 | + "error": { |
| 93 | + "message": "Execute of installPipeline function failed." |
| 94 | + "error": { |
| 95 | + "statusCode": 401, |
| 96 | + "message": "Unauthorized access to pipeline...." |
| 97 | + } |
| 98 | + } |
| 99 | +} |
| 100 | +``` |
| 101 | + |
| 102 | +And this JSON shall be `stringified` and logged as `error` in the final output |
| 103 | +of the log. |
| 104 | + |
| 105 | +The typescript interface of this ErrorChain is |
| 106 | + |
| 107 | +``` |
| 108 | +interface ErrorChain { |
| 109 | + statusCode?: number; |
| 110 | + message: string; |
| 111 | + error?: ErrorChain; |
| 112 | +} |
| 113 | +``` |
| 114 | + |
| 115 | +### 3.3 List of error codes |
| 116 | + |
| 117 | +To be completed |
| 118 | + |
| 119 | +| Error Code | Description | |
| 120 | +| ---------: | --------------------------------- | |
| 121 | +| 101 | Execution of command failed | |
| 122 | +| 110 | Validation of input values failed | |
| 123 | +| 201 | Azure pipeline API call failed | |
| 124 | +| ... | ... | |
| 125 | + |
| 126 | +## 4. Dependencies |
| 127 | + |
| 128 | +We do not have additional dependencies. The only dependency is `winston log`. |
| 129 | + |
| 130 | +## 5. Known issues |
| 131 | + |
| 132 | +None |
| 133 | + |
| 134 | +## 6. Risks & Mitigations |
| 135 | + |
| 136 | +We have to be careful so we do not expose secrets/passwords in logs. |
| 137 | + |
| 138 | +## 7. Documentations |
| 139 | + |
| 140 | +A formal document to be created and posted under |
| 141 | +https://github.com/CatalystCode/spk/tree/master/guides |
0 commit comments