Rest server: refine errors #807

Gerhut · 2018-07-04T05:19:01Z

coveralls · 2018-07-04T05:22:23Z

Coverage decreased (-28.4%) to 51.809% when pulling 29648f7 on qixcheng/rest-server-refine-errors into a97798a on master.

sterowang · 2018-07-06T03:09:34Z

rest-server/src/config/express.js

    message: err.message,
-    error: config.env === 'development' ? err.stack : '',


error [](start = 4, length = 5)

If we removed error here, will it cause client incompatible?

Error#stack, like comment, is listed mostly just for developer use and it is useless for automatic tools, and it has no content in production environment, so any client compatible with production environment should not depend on this field.

hwuu · 2018-07-10T00:56:23Z

rest-server/src/util/error.js

+const httpErrors = require('http-errors');
+const statuses = require('statuses');
+
+const createError = exports = module.exports = (status, code, message) => {


2 questions here:

What does createError = exports = module.exports mean?

What is the difference between module.exports and exports?

Since this module will be used by other developers lots of times, I concerned giving more hints to editor to help developer using it.

The simple answer is:

module.exports = () => {} is the general form of export a value as module itself, and can be imported by const createError = require('error');

exports.unknown = () => {} is the general form of export a value as module's field, and can be imported by const createUnknownError = require('error').unknown;

These two general forms can be easily detected by editors and give helps to autocomplete/intellisense.

When the commonJS module is initialized, module loader will assign an empty object to both exports and module.exports, and use the value of module.exports as module exports.
In other words. exports is just an alias of module.exports. When module.exports is overwritten, exports still reference the original module.exports object assigned by module loader, which will not be exported. So simply assign value to exports.unknown will not make the value exported, though it can be detected by editors. After assign the newly exported module.exports value to exports, any field assigned to exports can be exported in normal.

However, since d.ts is written, these hints only helps editors without supports of TypeScript but able to detect exports of commonJS module. Such as legacy WebStorm. (Didn't use it for a while)

hwuu · 2018-07-10T00:57:19Z

rest-server/src/util/error.js

+    return error;
+};
+
+const createUnknownError = exports.unknown = (cause) => {


2 questions here:

What does exports.unknown mean?

Where is createUnknownError used? I didn't find other place referring this function (I just found createError.unknown).

exports.unknown explained in the previous comment.

createUnknownError is used in line 36 Error.captureStackTrace, which helps avoid useless createUnknownError context output to error.stack.

hwuu · 2018-07-10T01:11:41Z

rest-server/src/util/error.d.ts

+
+declare function createError(status: Status, code: Code, message: string): HttpError;
+declare namespace createError {
+    declare function unknown(cause: Error | string): HttpError;


What does Error | string mean please?

Error | string means union type: either an Error object or a string.

The upstream error may be an error object by utility library, or an HTTP response text.

hwuu · 2018-07-10T01:13:41Z

rest-server/src/util/error.js

+// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+


Doesn't this file need to explicitly 'require' error.d.ts?

Editor will find d.ts with the same filename automatically.

hwuu · 2018-07-10T01:43:18Z

rest-server/src/util/error.d.ts

+    'Unauthorized';
+declare type Code =
+    'ERR_BAD_CONFIGURATION' |
+    'ERR_CONFLICT_JOB' |


CONFLICT --> CONFLICTED

hwuu · 2018-07-10T01:43:43Z

rest-server/src/util/error.d.ts

+declare type Code =
+    'ERR_BAD_CONFIGURATION' |
+    'ERR_CONFLICT_JOB' |
+    'ERR_CONFLICT_USER' |


CONFLICT --> CONFLICTED

hwuu · 2018-07-10T01:44:23Z

rest-server/src/util/error.d.ts

+    'Not Found' |
+    'Unauthorized';
+declare type Code =
+    'ERR_BAD_CONFIGURATION' |


Maybe there is no need to use the ERR_ prefix?

In previous practices, error code is transfered code by code, human by human, and may lost its error means in a high rate. Using a prefix to remind it is an error.

hwuu · 2018-07-10T01:45:28Z

rest-server/src/util/error.d.ts

+
+declare type Status = 
+    'Bad Request' |
+    'Conflict' |


Conflict --> Conflicted

This is the standard HTTP response message from https://github.com/jshttp/statuses/blob/master/codes.json#L34

Gerhut · 2018-07-13T03:23:06Z

According to Microsoft REST API Guidelines, all error codes were renamed to CamelCase.

Since no error.code field in current (mainline) version, this change introduces no backward compatibility problems.

* REST Server: refine user errors. * REST Server: typo * REST Server: move `user` field check in PUT user to param-validation * REST Server: add user test case * REST Server: Rename error codes according to the Mirosoft api-guideline * REST Server: make admin param in add/update user optional.

* REST Server: refine token errors * REST Server: Rename error codes according to the Mirosoft api-guideline

* REST Server: refine vc errors. * REST Server: add vc negative test case * REST Server: Rename error codes according to the Mirosoft api-guideline

* Refine job errors * Add test cases * REST Server: Rename error codes according to the Mirosoft api-guideline

…ror/readme REST Server: update error cases to README

Gerhut added 4 commits July 3, 2018 18:40

REST Server: refine error fields.

d658a2f

REST Server: refine paramter validation errors.

93911d0

REST Server: lint

f89144e

Rest Server: fix test case of invalid paraveters

9fc2fc6

Gerhut requested review from hwuu, wangcan0329 and abuccts July 4, 2018 05:19

REST Server: fix code in test case.

dcfb91c

Gerhut force-pushed the qixcheng/rest-server-refine-errors branch from 186f3fe to dcfb91c Compare July 4, 2018 05:25

REST Server: Add license to new files

84a395b

sterowang reviewed Jul 6, 2018

View reviewed changes

This was referenced Jul 6, 2018

REST Server: refine vc errors. #828

Merged

REST Server: refine user errors. #832

Merged

REST Server: refine token errors #835

Merged

REST Server: fix base directory of coveralls

3466b4f

Gerhut mentioned this pull request Jul 9, 2018

Refine job errors #839

Merged

hwuu reviewed Jul 10, 2018

View reviewed changes

REST Server: Rename error codes according to the Mirosoft api-guideline

6ac234e

abuccts mentioned this pull request Jul 13, 2018

[Rest Server] Refine error handling in rest server #660

Closed

hwuu approved these changes Jul 17, 2018

View reviewed changes

Gerhut and others added 8 commits July 18, 2018 14:42

REST Server: refine token errors (#835)

9074724

* REST Server: refine token errors * REST Server: Rename error codes according to the Mirosoft api-guideline

REST Server: refine vc errors. (#828)

bdc250a

* REST Server: refine vc errors. * REST Server: add vc negative test case * REST Server: Rename error codes according to the Mirosoft api-guideline

Fix lint

bb2eb4c

Refine job errors (#839)

4efc862

* Refine job errors * Add test cases * REST Server: Rename error codes according to the Mirosoft api-guideline

REST Server: update error cases to README

a642fbd

REST Server: Add periods to error messages.

1f957fb

REST Server: fill ellipsis in error message.

4cf71a0

REST Server: rename "no job ssh error" to "no job ssh info error"

e10e27b

Gerhut mentioned this pull request Jul 19, 2018

REST Server: check request payload type, fix #909 #925

Merged

Gerhut and others added 4 commits July 20, 2018 11:39

REST Server: fix indent in README

56ef329

REST Server: Add status code in successful response of doc

95e9ef5

REST Server: refine error messages from "not found" to "be not found"

9b3491c

Merge pull request #914 from Microsoft/qixcheng/rest-server-refine-er…

29648f7

…ror/readme REST Server: update error cases to README

Gerhut merged commit f61a94c into master Jul 24, 2018

Gerhut mentioned this pull request Jul 24, 2018

[Rest Server] Expose task role completion policy #879

Merged

Gerhut deleted the qixcheng/rest-server-refine-errors branch July 30, 2018 15:15

Gerhut mentioned this pull request Aug 1, 2018

It's not user-friendly to show the code error info when input the password #967

Closed

Gerhut mentioned this pull request Aug 28, 2018

Unified error messages in rest-server and web portal #401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest server: refine errors #807

Rest server: refine errors #807

Gerhut commented Jul 4, 2018 •

edited

Loading

coveralls commented Jul 4, 2018 •

edited

Loading

sterowang Jul 6, 2018

Gerhut Jul 6, 2018

hwuu Jul 10, 2018

Gerhut Jul 10, 2018

hwuu Jul 10, 2018 •

edited

Loading

Gerhut Jul 10, 2018

hwuu Jul 10, 2018

Gerhut Jul 10, 2018

hwuu Jul 10, 2018

Gerhut Jul 10, 2018

hwuu Jul 10, 2018

hwuu Jul 10, 2018

hwuu Jul 10, 2018

Gerhut Jul 10, 2018

hwuu Jul 10, 2018

Gerhut Jul 13, 2018

Gerhut commented Jul 13, 2018

		message: err.message,
		error: config.env === 'development' ? err.stack : '',

Rest server: refine errors #807

Rest server: refine errors #807

Conversation

Gerhut commented Jul 4, 2018 • edited Loading

coveralls commented Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwuu Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerhut commented Jul 13, 2018

Gerhut commented Jul 4, 2018 •

edited

Loading

coveralls commented Jul 4, 2018 •

edited

Loading

hwuu Jul 10, 2018 •

edited

Loading