DRAFT: One possible approach for simpler errors. #61
Conversation
Would fix GoogleCloudPlatform#37 if we decided to do this. We want to review this as a possible cross-platform approach though.
|
Note: we could potentially add an adapter to the Google.Cloud.Functions.Framework type that basically had the code that's in FunctionEnvironment.Execute. That would make it easier to get a consistent experience when not using the Invoker package. |
| namespace Google.Cloud.Functions.Framework | ||
| { | ||
| // More decisions and considerations: | ||
| // - Another message to be set on the response? (As content? Something else?) |
There was a problem hiding this comment.
I don't like this. It might be used to write random content, specially if success status codes are allowed.
There was a problem hiding this comment.
Well users can already write whatever content they want... it just gives an easy way to give callers a hint as to what's wrong (e.g. which required parameter is missing)
There was a problem hiding this comment.
Yes, that's true, I guess what I really don't like is the combination of this and being able to set success status code from the exception. I can imagine it being used to control flow and that makes me icky.
I like a lot what MVC has with the IActionResult and all sorts of derived types, even types for bad status code like UnauthorizedResult, NotFoundResult, etc. It gave the opportunity to cleanly control for those cases I think.
| { | ||
| // More decisions and considerations: | ||
| // - Another message to be set on the response? (As content? Something else?) | ||
| // - Prohibit success status codes? |
There was a problem hiding this comment.
I'd say yes, although maybe provide another way to set different success status codes? But definitely wouldn't like an exception controlling different success statuses.
There was a problem hiding this comment.
It would certainly be an anti-pattern to do so. Whether that makes it worth explicitly prohibiting or not, I don't know... but I'm happy enough to do it.
| var function = context.RequestServices.GetRequiredService<IHttpFunction>(); | ||
| await function.HandleAsync(context); | ||
| } | ||
| catch (FunctionException ex) |
There was a problem hiding this comment.
Maybe catch everything else as well to log a rethrow?
There was a problem hiding this comment.
No need for that - any exceptions that make it to the very top of the pipeline are logged and the response becomes a 500.
|
Not sure if I am the best person to review these changes. Going to unassign, but feel free to ping me on docs or other non .NET specific changes. |
|
Closing as I'm not expecting us to do this for beta, and it's now rather ancient. (Recreating is probably simpler than rebasing.) |
Would fix #37 if we decided to do this. We want to review this as a possible cross-platform approach though.