Handling of exceptions occurred outside of handler inside handler itself?
zrzka opened this issue · 5 comments
Description
Found this by accident and it's rather a question, thing for discussion, ...
I've got my custom lambda wrapper, which wraps async-lambda-fn:
(defn promise-lambda-fn
[creds env event-schema promise-fn & rest]
(async-lambda-fn
(fn [ev context]
(go
(try
(if-let [error (schema/check event-schema ev)]
(fail! context (ex/bad-request error))
(-> (apply promise-fn creds env ev context rest)
(p/then #(succeed! context %))
(p/catch #(fail! context %))))
(catch :default e
(fail! context (ex/internal-server-error "promise-lambda-fn" e))))))))This wrapper is used in this way:
(def EventSchema
{:principal-id ks/ErebosUserId ...})
(def ^:export handler
(promise-lambda-fn
(credentials)
env-variables
EventSchema
invite-users))It's a simple wrapper which is auto producing internal server error in case of exception or bad request in case if event is malformed. So far, so good. Works.
By accident, we did send atom value instead of proper message and lambda output is ...
{
"errorMessage": "RequestId: 185a635d-3ffa-11e7-9ee6-33e1af8e69ed Process exited before completing request"
}... with backtraces like ...
Error: No protocol method IEmptyableCollection.-empty defined for type object: [object Object]
at Object.cljs$core$missing_protocol [as missing_protocol] (/var/task/target/erebos-lambda/cljs/core.cljs:272:4)
... and this happens because of ...
https://github.com/nervous-systems/cljs-lambda/blob/master/cljs-lambda/src/cljs_lambda/util.cljs#L25
... basically, js->clj :keywordize-keys true can't handle this particular event.
Question
What if I like to return specific response from lambda in this case? Something like our ex/bad-request (400) instead of this generic error, which produces 500 for example? Event is valid JSON and I personally don't like when 500 is returned instead of 400, just because this can't be parsed / keywordized.
I can live with this, but it's not good if I want to provide better status codes / response to API consumers.
Event
Sample event we did send by accident.
{
"payload": {
"message": {
"state": {
"meta": null,
"cnt": 1,
"root": {
"edit": {},
"bitmap": 512,
"arr": [
{
"ns": null,
"name": "value",
"fqn": "value",
"_hash": 305978217,
"cljs$lang$protocol_mask$partition0$": 2153775105,
"cljs$lang$protocol_mask$partition1$": 4096
},
"foo",
null,
null,
null,
null,
null,
null
]
},
"has_nil_QMARK_": false,
"nil_val": null,
"__hash": null,
"cljs$lang$protocol_mask$partition0$": 16123663,
"cljs$lang$protocol_mask$partition1$": 8196
},
"meta": null,
"validator": null,
"watches": {
"meta": null,
"cnt": 0,
"arr": [],
"__hash": -15128758,
"cljs$lang$protocol_mask$partition0$": 16647951,
"cljs$lang$protocol_mask$partition1$": 8196
},
"cljs$lang$protocol_mask$partition1$": 16386,
"cljs$lang$protocol_mask$partition0$": 6455296
}
}
}I wouldn't have expected that there are JSON inputs which lambda accepts, but which js->clj can't deal with.
If you are concerned about this, I think being able to disable the input parsing in wrap-lambda-fn is maybe the most sensible thing I can think of - i.e. (async-lambda-fn (fn ...) {:parse-input? false}). While there are potentially other sources of error which could come up before the function's error-handler is in play, I don't think any of them are likely enough to justify intervening in a more general way.
N.B. it won't change the dynamics of this issue, but cljs-lambda has direct support for promises - you could probably do something like:
(async-lambda-fn
(fn [ev context]
(try
(if-let [error (schema/check event-schema ev)]
(fail! context (ex/bad-request error))
(apply promise-fn creds env ev context rest))
(catch :default e
(fail! context (ex/internal-server-error "promise-lambda-fn" e))))I was surprised as well. I'm not even sure if it's worth to spend time on {:parse-input? ...}, just did want to know your opinion on this. Will think about it and will either close this one later or provide pull request.
Re promise support. Know this, but this code is working and unless I'm going to fix something in this wrapper (or enhance it), I'm not going to touch it. Kind of rule - do not touch what works even if it's not super nice.
Fair enough. N.B. I have a branch which switches to using the AWS SDK, and includes support for tracing. I will publish a snapshot this week - any help with testing'd be great.
Sounds cool. Count me in, just ping me and we will give it a spin.
From my point of view, case closed. Closing issue.