Serde feature does not follow the spec

[adriandelgado]

Deserializing a string with comma separating values should not result in a vector of strings. The whatwg spec does not assign any special meaning to the comma.

This can be confirmed inside a javascript console

const url1 = new URL("http://example.com?foo=bar,baz");
console.log(url1.searchParams.get("foo")); // "bar,baz"
const url2 = new URL("http://example.com?foo=bar&foo=baz");
console.log(url2.searchParams.get("foo")); // "bar"
console.log(url2.searchParams.getAll("foo")); // [ "bar", "baz" ]

If foo is a multivalued query, the serde_json::Value should already contain a vector of strings.

[calavera]

This is a tricky issue because by making this compliant, we'll break compatibility with the main use case of this crate, which is to be used with the aws_lambda_events crate. AWS API Gateway does this very weird thing with headers and parameters, and not following the spec here is better for the final user of aws_lambda_events:

[adriandelgado]

I have to unfortunately agree. It seems like a downgrade from Version 1.0. I think this makes it impossible to have commas inside a query string.

I stumbled into this bug (feature?) while writing a crate to integrate the async-graphql crate with lambda-http. POST request work fine, however, one should also be able to write graphql queries using GET requests.

One workaround I can think of is to manually re-join the vector with commas inside my crate but I have to check If this doesn't have unintended consequences.

[calavera]

what if we had two features, the serde feature compliant with the spec, and another feature like serde-lambda-http with the current code?

I'm not sure if this would solve your problem though.

[adriandelgado]

My problem can only be solved by AWS launching a version 3.0 in which multivalued headers are serialized as an array of strings instead of creating an ad-hoc format.

However, I think it's a good idea still. It makes it so that other people outside the AWS ecosystem can use this crate.

One question. If the payload is version 1.0, does the aws_lambda_events crate also split the multivalued queries by commas? I can join the vector based on the RequestContext because version 1.0 shouldn't have this issue I think.

[calavera]

One question. If the payload is version 1.0, does the aws_lambda_events crate also split the multivalued queries by commas? I can join the vector based on the RequestContext because version 1.0 shouldn't have this issue I think.

yes, you can do that. Version 1.0 doesn't split them by comma. Which could also open an option to fix this problem. If we have two deserializers, one for v2, and another one spec compliant under the same feature, we can add attributes to the aws_lambda_events fields to use different deserializers.

[adriandelgado]

That would be nice. I would help with a PR but I'm new to that codebase and I think it uses some codegen stuff that I don't understand. 😅

Thanks for your help!

[calavera]

oh, it's actually not that hard anymore. This is what we need to do if you want to update your PR:

1. Move the current deserialization logic to a new module inside the serde module, something like serde::aws::api_gateway_v2.
2. Make the compliant version that you have the default deserialization option like you have in your changes.
3. When that's merged and released, go to the apigw module in aws_lambda_events, and update the deserializer: https://github.com/LegNeato/aws-lambda-events/blob/master/aws_lambda_events/src/apigw/mod.rs#L144 (I can do this last part)