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
feature: add custom http status code with error object #1334
feature: add custom http status code with error object #1334
Conversation
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.
In general, I like the core concept here a lot: If there's a property on the error thrown (you called it error.responseHttpCode
, I suggest that it be error.code
), then the specific server can chose how to relay that back to the connection.
It's probably important that each server be able to handle the code's error directly, and then our existing logic for determining the code sent to http connections can be a default behavior.
From a style point-of-view in your tests and examples, I would suggest that you opt for a more "functional" declaration style... rather than use generators to create the errors, you can likely just crete the error directly in the action's run method
classes/actionProcessor.js
Outdated
@@ -98,6 +98,11 @@ module.exports = class ActionProcessor { | |||
} | |||
|
|||
if (error) { | |||
const hasCustomResponseCode = error.responseHttpCode && this.connection.rawConnection |
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.
I think the actionProcessor
, is the wrong place for this logic. Every type of connection (websocket, socket, and custom ones) will have a connection.rawConnection
). This might belong with the other response code logic for web server @ https://github.com/actionhero/actionhero/blob/master/servers/web.js#L349-L361
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.
I see your point, yes moving it to the webserver logic makes more sense.
will move this logic to the required file. 😄
config/servers/web.js
Outdated
* } | ||
* | ||
* function SuspiciousActivity (message, extraFields) { | ||
* Error.call(this) |
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.
What's this
in this function?
It might be clearer to create the error directly in the run
method of the action rather than using a generator/factory:
const error = new Error('402: Suspicious activity detected')
error.code = 402
return error
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.
Node internally uses error.code
which means that it's a first-class property we can start relying on https://nodejs.org/api/errors.html#errors_error_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.
This note probably doesn't belong in the config file for the web server, but perhaps in the Tutorial for either Actions or the Web Server
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.
It might be clearer to create the error directly in the
run
method of the action rather than using a generator/factory:
Ohh!! Makes sense, will do that
This note probably doesn't belong in the config file for the webserver, but perhaps in the Tutorial for either Actions or the Web Server
Can you guide me on where to add this so that this will come inside the Tutorial for Actions or Web Server?
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.
Should I add them in the action tutorial OR web-server tutorial?
Adding them to the webserver tutorial makes more sense to me, thoughts?
throw new NotFoundError(`404: Filter '${data.params.query}' not found `) | ||
} | ||
} | ||
const hasRandomKey = !!data.params.randomKey |
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.
I've been out of touch with JS for a bit. What does !!data.params.randomkey
do?
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.
If randomKey
is not passed in the request, so it will be undefined
, so !undefined
will be true and doing !!undefined
will become false which means that the variable is not sent.
If randomKey
is set it will return true for !randomKey
and doing !!randomKey
will return true.
I wanted to not do this kind of thing as I know it seems confusing, but I was unable to find any kind of isEmpty
sort-of Util but was unable to find, so used this type of thing.
Should I remove this and create an isEmpty
sort-of Util and go ahead with that?
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.
I believe the typical convention is to use const hasRandomKey = data.params && data.params.randomKey
. Has that changed recently?
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.
You are absolutely right, doing const hasRandomKey = data.params && data.params.randomKey
would produce the same result, but then hasRandomKey
will contain the value of data.params.randomKey
not a boolean
, I thought that since the variable name is hasRandomKey
it represents a boolean
, plus that makes easy to read the if condition rather than predicting what it will do. Is this a good way to go?
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.
Looking really good!
Just some minor changes requested
5edfdb1
to
d299336
Compare
d299336
to
f790e2e
Compare
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 Great! Well done!
Currently, we have to add the custom error response code like this:
data.connection.rawConnection.responseHttpCode = 404
We can use the error.code that already exists in the
error object
Now in action, you can do something like this :