Skip to content
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

Query Parameter is only deserialized up to the first parameter and all the rest are left as None #802

Closed
Bryson14 opened this issue Jan 30, 2024 · 10 comments

Comments

@Bryson14
Copy link
Contributor

I am using Axum rust to handle Query parameters in the URL within the runtime. The issue that I can't figure out is that the program will only deserialize the first query parameter. If there are two or more, then the second and third query parameters will be read as null.

#![warn(clippy::cargo, clippy::unwrap_used)]
mod config;
mod models;
mod route_handlers;
mod web_server;
use aws_sdk_dynamodb::Client;
use config::{make_config, Opt};
use lambda_runtime::tower::ServiceBuilder;
use std::env::set_var;
use tower_http::add_extension::AddExtensionLayer;
use web_server::get_router;

const REGION: &str = "us-east-1";

#[tokio::main]
async fn main() -> Result<(), lambda_http::Error> {
    // AWS Runtime can ignore Stage Name passed from json event
    // https://github.com/awslabs/aws-lambda-rust-runtime/issues/782
    set_var("AWS_LAMBDA_HTTP_IGNORE_STAGE_IN_PATH", "true");

    // required to enable CloudWatch error logging by the runtime
    tracing_subscriber::fmt()
        .with_max_level(tracing::Level::INFO)
        // disable printing the name of the module in every log line.
        .with_target(false)
        // disabling time is handy because CloudWatch will add the ingestion time.
        .without_time()
        .init();

    // creating a dynamo db client for the router to use
    let config = make_config(Opt {
        region: Some(REGION.to_owned()),
        verbose: true,
    })
    .await;
    let client = std::sync::Arc::new(Client::new(
        &config.expect("Could not unwrap aws config for dynamodb client"),
    ));

    let router = get_router();

    // adding the client into the router as an extension layer
    let router = ServiceBuilder::new()
        .layer(AddExtensionLayer::new(client))
        .service(router);

    lambda_http::run(router).await
}

pub fn get_router() -> axum::Router {
    // Sets the axum router to produce only ERROR level logs. The handlers can produce their own level of logs.
    const LOG_LEVEL: Level = Level::ERROR;

    // With Routing, order does matter, so put the more specific first and then more generic routes later
    Router::new()
        .route("/conversations", get(list_conversations));

pub async fn list_conversations(
    Extension(db_client): Extension<Arc<DynamoDbClient>>,
    Query(params): Query<PaginationRequest>,
) -> WebResult {
    info!("params: {:?}", params);
    let client = db_client.clone();
    scan_items::<Conversation>(&client, DBTable::Conversation, params).await
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
pub struct PaginationRequest {
    // The hash key
    pub next_key_a: Option<String>,

    // The sort key
    pub next_key_b: Option<String>,

    // if no pagination should be used for the client, and all data should be returned
    pub fetch_all: Option<bool>,
}


When I invoke the api gateway, with next_key_a first, it will be read but next_key_b will be None. If i switch the query parameters, they will then change from Some to None

Calling https://5oclxtbcq3.execute-api.us-east-1.amazonaws.com/test-stage/conversations?next_key_b=7e95e561-3b3f-4a82-954f-fb37f8c0f257&next_key_a=053baf43-ac9c-4754-a94a-de982c169204 will cause next_key_b to be Some() and next_key_a to be None.

Then calling https://5oclxtbcq3.execute-api.us-east-1.amazonaws.com/test-stage/conversations?next_key_a=7e95e561-3b3f-4a82-954f-fb37f8c0f257&next_key_b=053baf43-ac9c-4754-a94a-de982c169204 will be the opposite.

I think it is something that happens before the payload from API Gateway hits the axum web server because I've tried a bunch of unit tests to make sure this struct can handle this from the url:

 #[test]
    fn test_pagination_request_full() {
        let uri: Uri = "http://example.com/path?next_key_a=hello&next_key_b=world&fetch_all=true"
            .parse()
            .unwrap();
        let result: Query<PaginationRequest> = Query::try_from_uri(&uri).unwrap();
        assert_eq!(result.0.next_key_a, Some(String::from("hello")));
        assert_eq!(result.0.next_key_b, Some(String::from("world")));
        assert_eq!(result.0.fetch_all, Some(true));
    }

    #[test]
    fn test_pagination_request_full_swapped() {
        let uri: Uri = "http://example.com/path?next_key_b=world&fetch_all=true&next_key_a=hello"
            .parse()
            .unwrap();
        let result: Query<PaginationRequest> = Query::try_from_uri(&uri).unwrap();
        assert_eq!(result.0.next_key_a, Some(String::from("hello")));
        assert_eq!(result.0.next_key_b, Some(String::from("world")));
        assert_eq!(result.0.fetch_all, Some(true));
    }

    #[test]
    fn test_pagination_request_partial() {
        let uri: Uri = "http://example.com/path?next_key_a=hello&fetch_all=true"
            .parse()
            .unwrap();
        let result: Query<PaginationRequest> = Query::try_from_uri(&uri).unwrap();
        assert_eq!(result.0.next_key_a, Some(String::from("hello")));
        assert_eq!(result.0.next_key_b, None);
        assert_eq!(result.0.fetch_all, Some(true));
    }
@calavera
Copy link
Contributor

can you access the Request object in Axum and check if the request.uri() has all the parameters correctly?

@calavera
Copy link
Contributor

I'm not sure how Axum transforms requests into Query struct, but I believe the runtime is generating the right Request object that we're sending to the frameworks. The tests in #803 show that the Request has all the query parameters, even with duplicated values.

@calavera
Copy link
Contributor

I added tests in #803 that show that Axum can correctly extract the query parameters from our Requests objects.

I noticed that Axum has two different versions of the extractors. I don't know which one you're using but the one in axum-extra seems to be more correct. See this comment: https://github.com/tokio-rs/axum/blob/main/axum/src/extract/query.rs#L38-L49

@calavera
Copy link
Contributor

calavera commented Feb 1, 2024

We've added tests in #803 and #804 that show that the runtime integration with Axum is doing the right thing. It's possible that you APIGW configuration has something to do with this.

Let me know if you've found something on your end, otherwise, we'll close this as a non issue in a couple of days.

@Bryson14
Copy link
Contributor Author

Bryson14 commented Feb 1, 2024

Thanks for the update, I'll be able to confirm this tomorrow.

@Bryson14
Copy link
Contributor Author

Bryson14 commented Feb 1, 2024

Here is what I have so far, I have a catch all route to try this out that looks like thi:

        .route("/*path", get(catch_get).post(catch_post))
}

/// A catch all route for GET routes for helpful debugging from the client side and API gateway
async fn catch_get(req: Request<Body>) -> JsonResponse {
    let uri = req.uri();
    let res = ErrorResponse {
        error_message: &format!(
            "unknown GET request to route '{uri}'. Check your path and try again. 
    If there is a extra url parameter passed on by API Gateway, 
    you can use the environment variable 'STAGE_NAME' to compensate for it."
        ),
    };
    info!("Unknown request caught. Response: {:?}", res);
    (StatusCode::NOT_FOUND, Json(res.to_value()))
}

When invoking from the lambda test window with this json, I get no path params. I assume because lambda runtime has to interperet the json object into a url request

{
  "path": "/conversations/idk/c8089eae-5e7d-11ec-94c8-b42e99b5b2c6?next_key_a=123",
  "httpMethod": "GET",
  "headers": {
    "Accept": "*/*",
    "Content-Type": "application/json"
  }
}

So this json payload results in 404 :

{
  "statusCode": 404,
  "headers": {
    "content-type": "application/json",
    "content-length": "282"
  },
  "multiValueHeaders": {
    "content-type": [
      "application/json"
    ],
    "content-length": [
      "282"
    ]
  },
  "body": "{\"error_message\":\"unknown GET request to route '/conversations/idk/c8089eae-5e7d-11ec-94c8-b42e99b5b2c6'. Check your path and try again. \\n    If there is a extra url parameter passed on by API Gateway, \\n    you can use the environment variable 'STAGE_NAME' to compensate for it.\"}",
  "isBase64Encoded": false
}

From Cloud Shell

Since api gateway doesn't support testing since I just have a single resouce with /{+proxy} pointing towards my lambda, I have to use the shell to invoke the API

curl https://123abc.execute-api.us-east-1.amazonaws.com/test-stage/what?limit=1&okay=why&hello=world&thanks=true leads to :

{"error_message":"unknown GET request to route 'https://5oclxtbcq3.execute-api.us-east-1.amazonaws.com/what?limit=1'. Check your path and try again. \n    If there is a extra url parameter passed on by API Gateway."}

Removing the first query parameter leads
curl https://123abc.execute-api.us-east-1.amazonaws.com/test-stage/what?okay=why&hello=world&thanks=true leads to :

{"error_message":"unknown GET request to route 'https://5oclxtbcq3.execute-api.us-east-1.amazonaws.com/what?okay=why'. Check your path and try again. \n    If there is a extra url parameter passed on by API Gateway."}

Maybe the +proxy from api gateway doesn't pass along all the query params?

Screenshot of the proxy resource and api gateway

image

@Bryson14
Copy link
Contributor Author

Bryson14 commented Feb 1, 2024

I was playing around more with the lambda console and sending json payloads directly:

This payload was interpreted correctly:

{
  "path": "/conversations/idk/c8089eae-5e7d-11ec-94c8-b42e99b5b2c6?next_key_a=123",
  "queryStringParameters": {
      "hello": "world"
  },
  "httpMethod": "GET",
  "headers": {
    "Accept": "*/*",
    "Content-Type": "application/json"
  }
}

Adding two or three string query parameters was read into the lambda correctly as well. However, when trying to pass in a number as a query parameter, I got a lambda-runtime error:

{
  "path": "/conversations/idk/c8089eae-5e7d-11ec-94c8-b42e99b5b2c6?next_key_a=123",
  "queryStringParameters": {
      "hello": "world",
      "name": "me",
      "test": 1
  },
  "httpMethod": "GET",
  "headers": {
    "Accept": "*/*",
    "Content-Type": "application/json"
  }
}

{
  "errorType": "&lambda_runtime::deserializer::DeserializeError",
  "errorMessage": "failed to deserialize the incoming data into the function's payload type: this function expects a JSON payload from Amazon API Gateway, Amazon Elastic Load Balancer, or AWS Lambda Function URLs, but the data doesn't match any of those services' events\n"
}

Perhaps keeping numbers is intentional part of the lambda runtime...

But I finally got the lambda function to work correctly when passing in the payload like this:

{
  "path": "/conversations",
  "queryStringParameters": {
      "limit": "1",
      "next_key_a":"7e95e561-3b3f-4a82-954f-fb37f8c0f257",
      "next_key_b":"675fc382-9dd7-4fe0-8c5b-c98089e525ce"
  },
  "httpMethod": "GET",
  "headers": {
    "Accept": "*/*",
    "Content-Type": "application/json"
  }
}

I this means that API Gateway is handling the request in a way we are not expecting.

@calavera
Copy link
Contributor

calavera commented Feb 1, 2024

Adding two or three string query parameters was read into the lambda correctly as well. However, when trying to pass in a number as a query parameter, I got a lambda-runtime error:

This is correct. Query string parameters MUST be string. This is definitely what API Gateway sends in normal circumstances if you send HTTP requests to the endpoint that it generates for you. What you're basically experiencing is that the Lambda console generates invalid payloads and sends them to the Lambda function directly. I've seen this happening before. We cannot do much about it unfortunately.

To be honest, I have very little experience configuring API Gateway, so I cannot help you much. @bnusunny might know if that /{+proxy} pattern makes sense or not.

@calavera
Copy link
Contributor

calavera commented Feb 4, 2024

I believe this is not related to the runtime. I just deployed the axum example behind an APIGW rest api and it works just fine:

image

I used the CDK stack in #807.

@calavera calavera closed this as completed Feb 4, 2024
Copy link

github-actions bot commented Feb 4, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants