-
Notifications
You must be signed in to change notification settings - Fork 83
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
refactor: migrate errors server controller to typescript #327
Changes from all commits
4bae8e1
abc4e92
c1686f5
a9165b3
d9f7fcd
ac523e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,39 +1,41 @@ | ||||||
'use strict' | ||||||
import _ from 'lodash' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update all functions that do not need the See further reading for differences between arrow functions and regular functions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also move and rename this file to the utility function it is, probably into Read more about utility vs service functions/classes here, and read more about the differences between service and controllers layers (this is more Java-like but the point still stands) to see why the functions in this file does not belong in the controller layer. Do hit me (or Google) up for a more detailed explanation if you still have any queries |
||||||
|
||||||
const _ = require('lodash') | ||||||
import { IMongoError } from 'src/types/error' | ||||||
|
||||||
/** | ||||||
* Default error message if no more specific error | ||||||
* @type {String} | ||||||
*/ | ||||||
exports.defaultErrorMessage = 'An unexpected error happened. Please try again.' | ||||||
export const defaultErrorMessage = | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first glance, I thought this is only used in one place, but seems to also be used in tests. Add a comment stating this is also exported for tests? Though I don't actually think it's necessary, we can also just hardcode this string in the tests. If you still want to export it, constants should be named in UPPER_SNAKE_CASE for consistency.
Suggested change
|
||||||
'An unexpected error happened. Please try again.' | ||||||
|
||||||
/** | ||||||
* Private helper for getMongoErrorMessage to return Mongo error in a String | ||||||
* @param {Object} err - MongoDB error object | ||||||
* @param {IMongoError} err - MongoDB error object | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Remove types from JSDocs, it's already typed in the function signature |
||||||
* @return {String} errorString - Formatted error string | ||||||
*/ | ||||||
const mongoDuplicateKeyError = function (err) { | ||||||
|
||||||
const mongoDuplicateKeyError = function (err: IMongoError): string { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of this function, I think it is too hackish instead of fixing the base issue. In fact, in the entire schema, there is only one instance of a schema with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also keep it in if you want, but I'm not a fan of this weird error message interpolation. |
||||||
let errorString = '' | ||||||
try { | ||||||
let fieldName = err.err.substring( | ||||||
if (err.err) { | ||||||
const fieldName = err.err.substring( | ||||||
err.err.lastIndexOf('.$') + 2, | ||||||
err.err.lastIndexOf('_1'), | ||||||
) | ||||||
errorString = | ||||||
fieldName.charAt(0).toUpperCase() + fieldName.slice(1) + ' already exists' | ||||||
} catch (ex) { | ||||||
} else { | ||||||
errorString = 'Unique field already exists' | ||||||
} | ||||||
return errorString | ||||||
} | ||||||
|
||||||
/** | ||||||
* Gets Mongo error object and returns formatted String for frontend | ||||||
* @param {Objrct} err MongoDB error object | ||||||
* @param {IMongoError} err? MongoDB error object | ||||||
* @return {String} message - Error message returned to frontend | ||||||
*/ | ||||||
exports.getMongoErrorMessage = function (err) { | ||||||
export function getMongoErrorMessage(err?: IMongoError | string): string { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole function seems like it needs a good refactor, maybe we should try to clean up the function? I don't think we should create the import { MongoError } from 'mongodb'
import { Error as MongooseError } from 'mongoose'
...
export const getMongoErrorMessage = (
err?: MongoError | MongooseError | string,
): string => { ... } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggested refactor: export const getMongoErrorMessage = (
err?: MongoError | MongooseError | string,
): string => {
if (!err) {
return ''
}
// Handle base Mongo engine errors.
if (err instanceof MongoError) {
switch (err.code) {
case 10334: // BSONObj size invalid error
return 'Your form is too large to be supported by the system.'
default:
return defaultErrorMessage
}
}
// Handle mongoose errors.
if (err instanceof MongooseError.ValidationError) {
// Join all error messages into a single message if available.
const joinedMessage = Object.values(err.errors)
.map((err) => err.message)
.join(', ')
return joinedMessage ?? err.message ?? defaultErrorMessage
}
if (err instanceof MongooseError) {
return err.message ?? defaultErrorMessage
}
return err ?? defaultErrorMessage
} |
||||||
let message = '' | ||||||
if (!err) { | ||||||
return '' | ||||||
|
@@ -50,17 +52,19 @@ exports.getMongoErrorMessage = function (err) { | |||||
message = 'Your form is too large to be supported by the system.' | ||||||
break | ||||||
default: | ||||||
message = exports.defaultErrorMessage | ||||||
message = defaultErrorMessage | ||||||
} | ||||||
} else if (!_.isEmpty(err.errors)) { | ||||||
// Prefer specific error messages to a generic one | ||||||
let errMsgs = [] | ||||||
for (let subError in err.errors) { | ||||||
const errMsgs = [] | ||||||
for (const subError in err.errors) { | ||||||
errMsgs.push(err.errors[subError].message) | ||||||
} | ||||||
message = errMsgs.join(', ') | ||||||
} else { | ||||||
message = err.message | ||||||
if (err.message) { | ||||||
message = err.message | ||||||
} | ||||||
} | ||||||
return message | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
export interface IMongoError { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unneeded, as explained above |
||
name?: string | ||
message?: string | ||
code?: number | ||
errmsg?: string | ||
err?: string | ||
errors?: { | ||
[subError: string]: { message: string } | ||
} | ||
[propName: string]: any | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,65 +1,62 @@ | ||
describe('Errors Controller', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remember to update tests |
||
const controller = spec( | ||
'dist/backend/app/controllers/errors.server.controller', | ||
) | ||
import { | ||
defaultErrorMessage, | ||
getMongoErrorMessage, | ||
} from 'src/app/controllers/errors.server.controller' | ||
|
||
describe('Errors Controller', () => { | ||
describe('getMongoErrorMessage', () => { | ||
it('should return blank string if no error', () => { | ||
expect(controller.getMongoErrorMessage()).toEqual('') | ||
expect(getMongoErrorMessage()).toEqual('') | ||
}) | ||
|
||
it('should return string if error is string', () => { | ||
let err = 'something failed' | ||
expect(controller.getMongoErrorMessage(err)).toEqual(err) | ||
const err = 'something failed' | ||
expect(getMongoErrorMessage(err)).toEqual(err) | ||
}) | ||
|
||
it('should call mongoDuplicateKeyError helperif error code is 11000 or 11001 (with field name)', () => { | ||
let err = { | ||
const err = { | ||
err: | ||
'E11000 duplicate key error index: test.so.$foo_1 dup key: { : 5.0 }', | ||
code: 11001, | ||
n: 0, | ||
nPrev: 1, | ||
ok: 1, | ||
} | ||
let expected = 'Foo already exists' | ||
expect(controller.getMongoErrorMessage(err)).toEqual(expected) | ||
const expected = 'Foo already exists' | ||
expect(getMongoErrorMessage(err)).toEqual(expected) | ||
}) | ||
|
||
it('should call mongoDuplicateKeyError helper if error code is 11000 or 11001 (no error msg)', () => { | ||
let err = { | ||
const err = { | ||
code: 11000, | ||
n: 0, | ||
nPrev: 1, | ||
ok: 1, | ||
} | ||
let expected = 'Unique field already exists' | ||
expect(controller.getMongoErrorMessage(err)).toEqual(expected) | ||
const expected = 'Unique field already exists' | ||
expect(getMongoErrorMessage(err)).toEqual(expected) | ||
}) | ||
|
||
it('should return error message for other error code', () => { | ||
let err = { | ||
const err = { | ||
code: 12, | ||
n: 0, | ||
nPrev: 1, | ||
ok: 1, | ||
} | ||
expect(controller.getMongoErrorMessage(err)).toEqual( | ||
controller.defaultErrorMessage, | ||
) | ||
expect(getMongoErrorMessage(err)).toEqual(defaultErrorMessage) | ||
}) | ||
|
||
it('should return error message if no error code', () => { | ||
let err = { | ||
const err = { | ||
errors: { | ||
error1: { | ||
message: 'error message 1', | ||
}, | ||
}, | ||
} | ||
expect(controller.getMongoErrorMessage(err)).toEqual( | ||
err.errors.error1.message, | ||
) | ||
expect(getMongoErrorMessage(err)).toEqual(err.errors.error1.message) | ||
}) | ||
}) | ||
}) |
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.
nit: prefer explicit destructure of used imports: