Open Simonl9l opened 1 year ago
Needs review with the team.
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.
@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:
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
@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 https://github.com/aws/aws-lambda-dotnet/issues/1567#issuecomment-1684530809 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!
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)
{
...
}
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 providedJsonSerlizationContext
(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 https://github.com/dotnet/runtime/issues/78029.
Proposed Solution
These
JsonSerlizationContext
(s) can be added into the package and can then be used explicit, or configured into Lambda by chaining theIJsonTypeInfoResolver
resolvers. For example:The
system.text.json
support to combination of serialization contexts:and configured in say a Minimal API context with (.Net7)
However the
SourceGeneratorLambdaJsonSerializer
would need some updates to accept theIJsonTypeInfoResolver
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 supportSystem.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 ofSystem.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
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