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

Update serialization support for Amazon.Lambda.CognitoEvents & Code Generated JsonSerlizationContext's #1567

Open
1 of 2 tasks
Simonl9l opened this issue Aug 12, 2023 · 5 comments
Assignees
Labels
feature-request A feature should be added or improved. module/lambda-client-lib needs-investigation p2 This is a standard priority issue

Comments

@Simonl9l
Copy link

Simonl9l commented Aug 12, 2023

Describe the feature

Per https://github.com/aws/aws-lambda-dotnet/tree/master/Libraries/src/Amazon.Lambda.CognitoEvents

It would likely be valuable to the dev community if the packages such that this support system.text.json code generated serialization and provided JsonSerlizationContext(s).

Use Case

The value proposition, specifically in regard to Amazon.Lambda.CognitoEvents is that Lambda's implementing Cognito Trigger, say via the Annotations library or raw Lambda Handler implementation have specific response timing constraints/requirements that make AoT a requirement, that in turn requires code generated serialization.

This likely applies to all the Lambda Event packages, that in some cases is more applicable to Minimal API than Annotations.

Whist one can can define one's own code generated serialization contexts for type sin other packages, it can have undesirable effects with any trimmed assemblies as the metadata to generate the serializer is not available - see dotnet/runtime#78029.

Proposed Solution

These JsonSerlizationContext(s) can be added into the package and can then be used explicit, or configured into Lambda by chaining the IJsonTypeInfoResolver resolvers. For example:

using System.Text.Json.Serialization;
using Amazon.Lambda.CognitoEvents;

namespace Amazon.Lambda.CognitoEvents
{
    [JsonSerializable(typeof(CognitoCreateAuthChallengeEvent))]
    [JsonSerializable(typeof(CognitoDefineAuthChallengeEvent))]
    ... // each of the events
    public partial class CognitoEventsSerializationContext : JsonSerializerContext
    {
    }
}

The system.text.json support to combination of serialization contexts:

var combinedResolver = JsonTypeInfoResolver.Combine(
    MySerializationContext.Default, // my code gen serializer context for my other types.
    CognitoEventsSerializationContext.Default);

and configured in say a Minimal API context with (.Net7)

 builder.Services.ConfigureHttpJsonOptions(options =>
    {
        options.SerializerOptions.TypeInfoResolver = combinedResolver;
    });

However the SourceGeneratorLambdaJsonSerializer would need some updates to accept the IJsonTypeInfoResolver from the combine.

Other Information

Related there see to be numerous ongoing conversation regarding System.Text.Json support for pre-existing contracts such as [DataMember] etc. but expect that ton of these solutions support System.Text.Json code generation and JsonSerlizationContext's.

Whilst this code mostly was updated a year ago, it seem to make the premise that #if NETCOREAPP3_1 be the correct define to determine if to use [DataMember] or [JsonPropertyName] attributes in support of System.Text.Json.

Per the MSFT docs NETCOREAPP3_1 preprocessor symbol is not defined when targeting .NET 5 - Recommended Action seems to indicate it should use #if NETCOREAPP.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

AWS .NET SDK and/or Package version used

Amazon.Lambda.CognitoEvents 2.1.1

Targeted .NET Platform

.Net 7 and above

Operating System and version

All

@Simonl9l Simonl9l added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 12, 2023
@ashishdhingra ashishdhingra added module/lambda-client-lib needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Aug 14, 2023
@ashishdhingra
Copy link
Contributor

Needs review with the team.

@normj
Copy link
Member

normj commented Aug 18, 2023

Hi @Simonl9l

I didn't add the JsonSerializationContext into the event packages because outside of AOT/Trimming this would increase the size of the assemblies if users weren't using the source generator. Also in event packages with multiple events there can be collisions of you add them all to the serialization context. For example in API Gateway event package if I added both the REST API and HTTP API request/response they cause a collision because the share some of the same names of types. So if we can't always create the monolithic serialization context for each package we have to figure out a scheme for grouping the contexts together. All this work was done before AOT was a think for .NET. I agree now that we have AOT we need to figure out a strategy to make this work.

We adding a serialization context in the event package help because the converters are in the Amazon.Lambda.Serialization.SystemTextJson so the converters would still be external?

A good call on the NETCOREAPP3_1 preprocessor. In most of our other repos we have moved to the NETXXX_GREATER preprocessors. We will need to make sure we do a pass on this repo to switch to the GREATER preprocessors.

@Simonl9l
Copy link
Author

@normj Thanks for the considered response!

As we understand it, if say to serialize you specify the type into which you want to deserialize, that implicitly includes namespaces per [JsonSerializable(typeof(CognitoCreateAuthChallengeEvent))]. so we're not sure where any name collisions would have any impact even if parts of the model sub-types are shared across Event Types (as happens with the Cognito ones)?

Do you have a specific example - as perhaps we're not following?

Patterns we have seen elsewhere is to add a serialization context's that are more granular on the basis they can all be chained in with .Net 7 - at least in regard to the minimal AI builder.Services.ConfigureHttpJsonOptions and its TypeInfoResolver. The big impact here is exposing an equivalent via the LambdaBootstrapBuilder(*).

Perhaps:

  • that granularity can go down to Event level?
  • the serializer contexts could be made available in a separate package so as not to impact non AoT use cases with the excess baggage in assembly size.
  • the event packages may need some code attribution to support usage in AoT projects so any trimming behaves correctly.

An issue we have run into elsewhere, is where packages have been published in a trimmed state, such that you can't create ones own serializer contexts as there is no Type Info available.

It also worth noting that net6.0 packaged serializer contexts are incompatible with net7.0 serializers due to a breaking change, and they hopefully won't break again with .net8.0 which we assume AWS will fully support as LTS.

From a 10,000ft AoT perspective we see the biggest challenges with the AWS dotnet package library including SDK and it's significant use of reflection - especially in the DI extensions. We believe that significant portions (inc Minimal API) of APS.NET Core will support AoT in .Net 8!

Glad to help per NETCOREAPP3_1 observation!

Of note per (*) LambdaBootstrapBuilder and not directly related to AoT, we dug a little and see it also uses the same functionary behind the Minimal API AddAWSLambdaHosting and observe we don't get the same timeout issues in a raw or Annotations Lambda using this in .Net 7 as we do with Minimal API. We have found deploying .Net7 Minimal API lambdas as SelfContained with the dotnet 6 runtime vs. the AL2 custom runtime works as expected. Leading us further to believe there is something in the AL2 runtime that is incompatible with .Net 7 Minimal API - we wonder if the same will apply to .Net 8, or if we'll see AL2023 anytime soon

@ashishdhingra ashishdhingra added the p2 This is a standard priority issue label Aug 22, 2023
@Simonl9l
Copy link
Author

Simonl9l commented Jan 28, 2024

@norm & @ashishdhingra gentlemen, its been a while and I find myself back here specifically as it relates to AoT.

However getting our efforts to fully AoT is a journey give dependency support, of some of the libraries we're using.

To ensure we're using the code generates serilaization correctly for AoT (as we get there), were using the csproj property <JsonSerializerIsReflectionEnabledByDefault>false</JsonSerializerIsReflectionEnabledByDefault> to ensure that PublishedTrimmed projects fail reflection-based serialization and MSFT's recommendation to Disable reflection defaults

Per the #1567 (comment) above, the reply only covered the second part of my feature request. We're use 3rd party libraries that do have their own serialization contexts.

In the spirit of alignment with .Net (8.0) it would be great if when using LambdaBootstrapBuilder one could pass in a chain of Serializer Contexts similar to this.

Relates to the .Net8.0 support are there any plans here in the short to medium term in they area.

It's also great to see NETCOREAPP3_1_OR_GREATER pre-processor directives all over the AWS/Lambda models vs NETCOREAPP3_1, per recent NuGet packages.

Thanks again for all the efforts!

@Simonl9l
Copy link
Author

Ah after some more digging...this is possible:

var builder = LambdaBootstrapBuilder.Create<JsonElement, JsonElement>(FunctionHandler, new SourceGeneratorLambdaJsonSerializer<LambdaSerializationContext>(
    options =>
    {
        options.TypeInfoResolverChain.Add(MyOtherSerializationContext.Default);
    }));

private async Task<JsonElement> FunctionHandler(JsonElement request, ILambdaContext context)
 {
      ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. module/lambda-client-lib needs-investigation p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants