srijs / rust-aws-lambda

Support for running Rust programs on AWS Lambda
https://srijs.github.io/rust-aws-lambda
MIT License
319 stars 17 forks source link

exception on api gateway #13

Closed quezlatch closed 6 years ago

quezlatch commented 6 years ago

it's more than likely I'm doing something wrong as my rust knowledge is spectacularly limited, but when I try and use api gateway I get:

{
  "errorMessage": "invalid type: null, expected a string at line 1 column 13",
  "errorType": "Error"
}

It seems to be a problem with input

I've attached a zip of the project to show this. run the api with sh ./build.sh and do curl http://127.0.0.1:3000/ in another tab to hit the endpoint. it needs docker to build as well

incidentally, the musl build is larger than the standard build. I thought it was supposed to be the other way round...

thanks, Mike

srijs commented 6 years ago

Hi Mike, thanks for the report! At a first glace, this looks like a problem deserializing the proxy request event.

In particular, the body field is expected to be a String, but since you are making a GET request without a request payload, the field has the value null in the event payload.

@LegNeato do we need a custom deserializer for string fields which can handle null values? Or would you prefer another way of fixing this issue? I guess we could wrap all fields with Option<>, but that would probably hurt ergonomics quite a bit?

Just as an aside, the reason your musl build is larger is because now the binary bundles both your application as well as the libc, whereas the standard build dynamically loads the libc which is already present on AWS linux, and so doesn't need to be bundled.

LegNeato commented 6 years ago

That's surprising to me as it means the official Go code would also have a problem handling it and isn't correct. I'll look more in depth on Monday, perhaps we just need to update the defs there. I really don't want to make everything an Option<>...

For build size if you want there are docker images with Amazon Linux you can build and test in for dynamic linking but I would highly suggest sticking with musl.

mockersf commented 6 years ago

I'm having the same issue with the default test event for an api gateway lambda.

Looking at how the golang json decoder is working (this example slightly modified), a null json object for a string get decoded as "", so that would explain why there's no error with the official Go library.

So between making every String an Option<>, or setting a default to "", I'm not sure which is the less unpleasant...

LegNeato commented 6 years ago

Thanks for digging into this, I was afraid that would be the behavior and that really sucks. Neither solution is ideal, sigh.

LegNeato commented 6 years ago

I think I am going to support both options and let users opt into the behavior/API they want.

LegNeato commented 6 years ago

Starting to work on this now.

LegNeato commented 6 years ago

Please let me know if the latest changes work for you / the docs are clear!

quezlatch commented 6 years ago

Not sure if my cargo.toml is right, but if I have:

[dependencies]
aws_lambda = { git = "https://github.com/srijs/rust-aws-lambda", features = ["string-null-empty"] }

or even

[dependencies]
aws_lambda = { git = "https://github.com/srijs/rust-aws-lambda", default-features = false, features = ["string-null-empty"] }

I get a whole load of error[E0124]: field <field name> is already declared errors

quezlatch commented 6 years ago

and with the string-null-none I get:

error[E0277]: the size for value values of type `str` cannot be known at compilation timeadata...  
  --> .cargo/git/checkouts/rust-aws-lambda-2a7b20b15eabd9cc/29931cf/aws_lambda_runtime/src/proto/encoder.rs:14:21
   |
14 | #[derive(Serialize, SchemaSerialize)]
   |                     ^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `str`
   = note: to learn more, visit <https://doc.rust-lang.org/book/second-edition/ch19-04-advanced-types.html#dynamically-sized-types--sized>
   = note: required because of the requirements on the impl of `serde_schema::SchemaSerialize` for `&'static str`
   = note: required by `serde_schema::SchemaSerialize::schema_register`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: Could not compile `aws_lambda_runtime`.  

this happens on stable and nightly...

srijs commented 6 years ago

@quezlatch that second error is a problem I'm currently fixing in https://github.com/srijs/rust-serde-schema, will report back once it's sorted out!

srijs commented 6 years ago

@quezlatch Should be fixed now on serde_schema master, can you again after doing a cargo update to pick up the commit?

quezlatch commented 6 years ago

so, it compiles now and I'm seeing the options correctly. however I'm now getting:

{
  "errorMessage": "invalid type: null, expected a map at line 1 column 82",
  "errorType": "Error"
}

this is subtly different in that it's complaining about a Map now

LegNeato commented 6 years ago

Interesting! It looks like rather than panicing on deserialization this is happening when you return a response? If so can you confirm that your function code is being called and it is indeed on the serialization side / when you return a response back?

quezlatch commented 6 years ago

It's definitely the request I'm afraid :)

This:

    lambda::start(|()| {
        let text = String::from("cheese");
        Ok(ApiGatewayProxyResponse {
                headers: HashMap::new(),
                body: Some(text),
                is_base64_encoded: Some(false),
                status_code: 200
            })
        })

gives cheese

and this:

    lambda::start(|input: ApiGatewayProxyRequest| {
        let text = match input.body {
            Some(i) => i,
            None => String::from("empty")
        };
        Ok(ApiGatewayProxyResponse {
                headers: HashMap::new(),
                body: Some(text),
                is_base64_encoded: Some(false),
                status_code: 200
            })
        })

gives

START RequestId: a8196ad2-e9a1-1824-1f3a-0b125ef07722 Version: $LATEST
END RequestId: a8196ad2-e9a1-1824-1f3a-0b125ef07722
REPORT RequestId: a8196ad2-e9a1-1824-1f3a-0b125ef07722  Duration: 1.16 ms   Billed Duration: 100 ms Memory Size: 128 MB Max Memory Used: 2 MB   
{
  "errorMessage": "invalid type: null, expected a map at line 1 column 82",
  "errorType": "Error"
}
LegNeato commented 6 years ago

Alrighty, thanks for testing that. I guess I need to do the whole song and dance for every type. As a consumer would you expect an empty map or None to be the default when lambda doesn't have a value?

quezlatch commented 6 years ago

An empty map doesn't seem like an unreasonable way to go...

mockersf commented 6 years ago

seems the difference between the two was discussed in #12

LegNeato commented 6 years ago

Third time's a charm? 🙈

quezlatch commented 6 years ago

it was so close!

START RequestId: 3ad36c65-bb8d-14e6-a312-21ffb679f39f Version: $LATEST
END RequestId: 3ad36c65-bb8d-14e6-a312-21ffb679f39f
REPORT RequestId: 3ad36c65-bb8d-14e6-a312-21ffb679f39f  Duration: 0.74 ms   Billed Duration: 100 ms Memory Size: 128 MB Max Memory Used: 2 MB   
{
  "errorMessage": "missing field `authorizer` at line 1 column 597",
  "errorType": "Error"
}
2018-07-11 08:36:51 Function returned an invalid response (must include one of: body, headers or statusCode in the response object). Response received: 

I think the problem is being manifested at here

LegNeato commented 6 years ago

Ugh, of course I removed default HashMap serde values right before I pushed because they seemed redundant. Appreciate you sticking with this and helping me debug! Please try the latest version and let me know what else I have wrong 😄

quezlatch commented 6 years ago

seems to work a treat now. many thanks! I've also added a pr #15 which should make it easier to switch between string-null-none and string-null-empty

LegNeato commented 6 years ago

Sweet! Thanks again for testing.