aws / aws-lambda-dotnet

Libraries, samples and tools to help .NET Core developers develop AWS Lambda functions.
Apache License 2.0
1.58k stars 478 forks source link

The LambdaSerializer attribute is ignored #1582

Closed Kieranties closed 8 months ago

Kieranties commented 1 year ago

Describe the bug

The LambdaSerializer attribute is explicitly ignored in the code base.

This means there is no way to customize the serialization of incoming request bodies. The functionality detailed here does not appear to be possible.

It should be noted that outgoing request bodies can be customized by implementing a custom IHttpResult (as unfortunately the implementation there uses a static instance of JsonSerializer too)

Expected Behavior

At the assembly level, specifying a custom serializer implementation should be honoured. e.g. Adding [assembly: LambdaSerializer(typeof(Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer))] should use an instance of Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer to perform serialization duties.

Current Behavior

Any customization is fully ignored.

Reproduction Steps

  1. Create a project based on Lambda annotations
  2. Follow the instructions documented at https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.Serialization.SystemTextJson/README.md#customizing-serialization-options
  3. Provide a customization such as a different naming policy or custom converter
  4. Run and execute your function

Result: customizations are not used.

Possible Solution

I've taken a look at this today and a trivial (though likely incorrect) way to grab the type defined in the attribute registered at the assembly level is to resolve the attribute value like so:

  var data = lambdaMethodSymbol.ContainingAssembly.GetAttributeData(context, TypeFullNames.LambdaSerializerAttribute);
  var serializerType = data.ConstructorArguments[0].Value;

Note: Additional checks would be needed to check for the method level attribute too, this is just a method to move forward for further investigation.

And then assign to the model:

var model = new LambdaFunctionModel()
{
    //...snip
    Serializer = serializerType.ToString(),
    //...snip
};

A small change to the templates where body responses are being processed is required e.g. in an incoming request

var serializer = new <#= _model.Serializer #>();
<#= parameter.Name #> = serializer.Deserialize<<#= parameter.Type.FullName #>>(__request__.Body);

And I thought I was good... I can switch out the type for my own custom implementation but... what is the contract to honour for the serializer?

I originally used Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer but the contract for serialization requires a stream, which is not required when calling the default System.Text.Json.Serializer.

I can of course implement my own type that honours the same contract as System.Text.Json.Serializer but it feels to me the user should be guided a little better. i.e the serializer should be checked that the type registered implements a given interface. System.Text.Json.Serializer has no interface.

Is it worth investigating alternative resolution paths for customization? Such as an attribute to specify serializer options for a method or assembly, or even allowing the options to be resolved from the container?

Additional Information/Context

I'm keen to have this fixed and am happy to contribute changes as required to get it done.

AWS .NET SDK and/or Package version used

Amazon.Lambda.Annotations 1.0.0

Targeted .NET Platform

net6.0

Operating System and version

Windows 11

ashishdhingra commented 1 year ago

Needs review with the team. Most likely a feature request.

Kieranties commented 1 year ago

@ashishdhingra The sourcegenerator raises this diagnostic error when the serializer attribute is not specified: You are required to register a serializer with the attribute. If the diagnostic exists to report an error on "the JSON serializer for Lambda events" not being registered, and then when registered that serializer is ignored. This looks and feels like a bug. You have a documented feature that does not work.

Currently, as this feature does not work - the only way to handle custom serialization of incoming requests is to litter request models with json serializer attributes. This is not possible when the models come from another source (e.g. an external nuget package).

It's either a bug that the attribute is required, or it's a bug that the feature is not implemented as documented.

normj commented 1 year ago

@ashishdhingra I think it is fair to consider this a bug that when serializing the IHttpRequest we are not honoring the registered ILambdaSerializer.

ashishdhingra commented 1 year ago

We should perhaps fix the issue https://github.com/aws/aws-lambda-dotnet/issues/1572 as well along with this issue.

ashishdhingra commented 1 year ago

P1 feature request considering the PR for issue https://github.com/aws/aws-lambda-dotnet/issues/1572 is merged first.

Deadvi5 commented 8 months ago

Any update on this? currently having the same problem in net 8 and latest aws packages

Kieranties commented 8 months ago

@Deadvi5 it looks like some work has been completed in this commit: https://github.com/aws/aws-lambda-dotnet/commit/d7e5ac08f4c7bed5f2b8a2a6ac746198d215ac0a

I am no longer using this api and instead have built an internal api for lambdas (annotations also prevented us from controlling what runs after the container is built in startup)

Deadvi5 commented 8 months ago

@Kieranties I was lucky enough to get the following packages released today!

Now everything seems to work as expected! I need to do a more deep testing session, I'll report if i found something strange

normj commented 8 months ago

This has been fixed as of version 1.1.0 of Amazon.Lambda.Annotations. The Visual Studio template will be updated to use the latest version of Amazon.Lambda.Annotations as part of the upcoming .NET 8 Lambda support.

github-actions[bot] commented 8 months ago

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. If you wish to keep having a conversation with other community members under this issue feel free to do so.