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

The LambdaSerializer attribute is ignored #1582

Closed
Kieranties opened this issue Sep 8, 2023 · 10 comments
Closed

The LambdaSerializer attribute is ignored #1582

Kieranties opened this issue Sep 8, 2023 · 10 comments
Labels
annotations bug This issue is a bug. p1 This is a high priority issue queued

Comments

@Kieranties
Copy link

Kieranties commented Sep 8, 2023

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

@Kieranties Kieranties added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 8, 2023
@ashishdhingra ashishdhingra added annotations needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 11, 2023
@ashishdhingra
Copy link
Contributor

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

@ashishdhingra ashishdhingra added needs-review and removed needs-reproduction This issue needs reproduction. labels Sep 11, 2023
@Kieranties
Copy link
Author

Kieranties commented Sep 12, 2023

@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
Copy link
Member

normj commented Sep 12, 2023

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

@ashishdhingra
Copy link
Contributor

We should perhaps fix the issue #1572 as well along with this issue.

@ashishdhingra ashishdhingra added the p1 This is a high priority issue label Sep 15, 2023
@ashishdhingra
Copy link
Contributor

P1 feature request considering the PR for issue #1572 is merged first.

@Deadvi5
Copy link

Deadvi5 commented Feb 16, 2024

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

@Kieranties
Copy link
Author

@Deadvi5 it looks like some work has been completed in this commit: d7e5ac0

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
Copy link

Deadvi5 commented Feb 16, 2024

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

  • Amazon.Lambda.Annotations Version="1.2.0"
  • Amazon.Lambda.Serialization.SystemTextJson Version="2.4.1"

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
Copy link
Member

normj commented Feb 17, 2024

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.

@normj normj closed this as completed Feb 17, 2024
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annotations bug This issue is a bug. p1 This is a high priority issue queued
Projects
None yet
Development

No branches or pull requests

4 participants