make ErrorResponse#errorType efficient and impossible to mis-represent #50

softprops · 2018-12-17T13:48:14Z

Issue #, if available:

Description of changes:

While studying the source of this project I came across of few improvements that I thought would be helpful to spread across independent pull requests. In this case I noticed that ErrorResponse#errorType was represented as a string though it's values as it seems today, are enumerable. In cases like these, especially for public fields, it makes it possible to represent invalid values. As such its error prone to work with :). There's another optimization here is that with enum values, there's no need to allocate new strings when building the errorType value of the ErrorResponse object.

Included in addition to the changes are tests to validated them.

@davidbarsky Here's a few things interesting things I noticed.

By habit I'm used to running cargo +nightly fmt --all after making changes. I noticed in this case that increased the side of this diff because it seems it hasnt been run recently and has affected other unrelated files. I wanted to get your input here. Should I just reformat those in this pull. An alternative that I'd prefer is opening a pull that does that + adds cargo fmt --all -- check to pull to prevent the introduction of adhoc style fmting. Thoughts?
ErrorResponse#stacktrace serializes to null by default. There's a link to the runtime open api spec here. Though I can't speak to its accuracy and currecness it seems a bit unclear which fields are required and which aren't in the ErrorResponse definition.

in the spec it says meaning all fields are optional according to openapi

    ErrorResponse:
      type: object
      properties:
        errorMessage:
          type: string
        errorType:
          type: string

however... in the same doc there's a comment at the top that describes ErrorResponses as having a stackTrace ( with an empty value )

# Runtimes should report all errors using Lambda standard error format, in order to integrate with other AWS services:
#
# Content-Type: application/vnd.aws.lambda.error+json:
# {
#     "errorMessage": "...",
#     "errorType": "...",
#     "stackTrace": [],
# }
#

You may want to pass this feedback long to the the lambda team if you've got the right communication channels open :)

With serde you can instruct it to not serialize a value when not present or "empty" It's unclear what this should be but I thought it was out of scope for my change. I'm happy to adfress here but just as happy to address in a follow up or track with new gh issue.

By submitting this pull request

I confirm that my contribution is made under the terms of the Apache 2.0 license.
I confirm that I've made a best effort attempt to update all relevant documentation.

softprops · 2018-12-17T13:50:26Z

It's worth calling out I discovered the stackTrace thing while writing a unit test. It makes me think there's value in upping code coverage a bit for the sake of maintaining integrity across changes

softprops · 2018-12-17T13:52:02Z

lambda-runtime-client/src/error.rs

+    fn handled_error_response_serializes() {
+        assert_eq!(
+            serde_json::to_string(&ErrorResponse::handled("💀")).expect("failed to serialize ErrorResponse"),
+            r#"{"errorMessage":"💀","errorType":"Handled","stackTrace":null}"#


unrelated, but note the json null ( serde default for Option::None ). we may want to catch this in a follow up if that's not intentional

Unsure, I've reached out to the lambda runtime team about this.

softprops · 2018-12-17T13:53:13Z

lambda-runtime-client/src/error.rs

@@ -61,10 +61,10 @@ impl ErrorResponse {
    ///
    /// # Return
    /// A populated `RuntimeError` object that can be used with the Lambda Runtime API.
-    pub fn unhandled(message: String) -> ErrorResponse {
+    pub fn unhandled<M>(message: M) -> ErrorResponse where M: Into<String> {


happen to back out of this but in general this is fmt is typically more ergonomic in rust as it now accepts anything that implements Into<String> like slices and the like

happen to back out of this but in general this is fmt is typically more ergonomic in rust as it now accepts anything that implements Into like slices and the like.

Nice catch, agreed. we were a little pressed before release, so it's nice that you're catching all these little gotchas that we missed first time around...

softprops · 2018-12-17T14:12:15Z

addressing the rustfmt thing over in #51

softprops · 2018-12-17T15:38:34Z

lambda-runtime-client/src/error.rs


 /// This object is used to generate requests to the Lambda Runtime APIs.
 /// It is used for both the error response APIs and fail init calls.
 /// custom error types should implement the `RuntimeError` trait and return
 /// this object to be compatible with the APIs.
 #[derive(Serialize)]
+#[serde(rename_all="camelCase")]


this removes some potential for error prone manual string formatted by using a feature built into serde that will do the formatting for you resulting in less code and less error-proneness.

Good call, thanks.

davidbarsky · 2018-12-17T15:55:42Z

Merged #51.

davidbarsky · 2018-12-17T15:57:20Z

It's worth calling out I discovered the stackTrace thing while writing a unit test. It makes me think there's value in upping code coverage a bit for the sake of maintaining integrity across changes

Yes, agreed. I've been working on a clean-room implementation of the runtime (with unit tests) to see if I can discover any blindspots. I'll have a PR for that up shortly (or at the very least, a separate repo that you can view).

sapessi · 2018-12-17T16:23:32Z

Hey @softprops, hold off on these changes. Same thing for my PR (#26) - it was just an experiment. We are likely to change the way errors are handled for the next release of the framework. We are discussing this internally with the Lambda team too. I will open an RFC issue about the proposed methodology this week.

sapessi · 2018-12-17T18:09:36Z

See #54

softprops · 2018-12-20T10:19:18Z

closing this pull for now to keep the merge queue clear of debris. thanks for the link!

softprops added 2 commits December 17, 2018 22:30

make ErrorResponse#errorType efficient and imposible to mis-represent

5581c49

use enums and avoid unnecessary allocations

fab5a8a

softprops changed the title ~~make ErrorResponse#errorType efficient and imposible to mis-represent~~ make ErrorResponse#errorType efficient and impossible to mis-represent Dec 17, 2018

softprops commented Dec 17, 2018

View reviewed changes

test what we think we're testing

48ed30d

softprops mentioned this pull request Dec 17, 2018

Handle any errors #26

Closed

softprops commented Dec 17, 2018

View reviewed changes

softprops closed this Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ErrorResponse#errorType efficient and impossible to mis-represent #50

make ErrorResponse#errorType efficient and impossible to mis-represent #50

softprops commented Dec 17, 2018

softprops commented Dec 17, 2018

softprops Dec 17, 2018

davidbarsky Dec 17, 2018

softprops Dec 17, 2018

davidbarsky Dec 17, 2018

softprops commented Dec 17, 2018

softprops Dec 17, 2018

davidbarsky Dec 17, 2018

davidbarsky commented Dec 17, 2018

davidbarsky commented Dec 17, 2018

sapessi commented Dec 17, 2018

sapessi commented Dec 17, 2018

softprops commented Dec 20, 2018 •

edited

Loading

make ErrorResponse#errorType efficient and impossible to mis-represent #50

make ErrorResponse#errorType efficient and impossible to mis-represent #50

Conversation

softprops commented Dec 17, 2018

softprops commented Dec 17, 2018

softprops Dec 17, 2018

Choose a reason for hiding this comment

davidbarsky Dec 17, 2018

Choose a reason for hiding this comment

softprops Dec 17, 2018

Choose a reason for hiding this comment

davidbarsky Dec 17, 2018

Choose a reason for hiding this comment

softprops commented Dec 17, 2018

softprops Dec 17, 2018

Choose a reason for hiding this comment

davidbarsky Dec 17, 2018

Choose a reason for hiding this comment

davidbarsky commented Dec 17, 2018

davidbarsky commented Dec 17, 2018

sapessi commented Dec 17, 2018

sapessi commented Dec 17, 2018

softprops commented Dec 20, 2018 • edited Loading

softprops commented Dec 20, 2018 •

edited

Loading