Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializer uses different property casing than Amazon.Lambda.Serialization.Json.JsonSerializer

[bjorg]

It appears that the default casing behavior has changed between Amazon.Lambda.Serialization.Json.JsonSerializer and Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializer.

Here is some sample code to test the difference:

// create an instance to serialize
var record = new Record {
    Foo = "Hello world!"
};

// show serialization with original Lambda serializer based on Newtonsoft.Json
var oldSerializer = SerializeWith(record, new Amazon.Lambda.Serialization.Json.JsonSerializer());
Console.WriteLine($"Amazon.Lambda.Serialization.Json.JsonSerializer: {oldSerializer}");

// show serialization with new Lambda serializer based on System.Text.Json
var newSerializer = SerializeWith(record, new Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializer());
Console.WriteLine($"Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializer: {newSerializer}");

// show serialization with System.Json.Text
var jsonTextSerializer = System.Text.Json.JsonSerializer.Serialize<Record>(record);
Console.WriteLine($"System.Text.Json.JsonSerializer: {jsonTextSerializer}");

// local functions
string SerializeWith<T>(T value, Amazon.Lambda.Core.ILambdaSerializer serializer) {
    using var buffer = new MemoryStream();
    serializer.Serialize<T>(value, buffer);;
    return System.Text.Encoding.UTF8.GetString(buffer.ToArray());
}

The above code produces the following output:

Amazon.Lambda.Serialization.Json.JsonSerializer: {"Foo":"Hello world!"}
Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializer: {"foo":"Hello world!"}
System.Text.Json.JsonSerializer: {"Foo":"Hello world!"}

[normj]

Agreed I should not have switched the casing between the 2 libraries. I think we are lacking tests for custom request and responses as the tests now mostly focus on AWS events.

Now that this has shipped changing the default behavior is really unfeasible. My suggestion is to add a new constructor that takes in an enum for casing style so you can declare the casing you want to use. I could then update the templates to use the new constructor. How do you feel about that work around?

[bjorg]

I don't know what the idea here was. I understand that you don't want to break folks who may have taken a dependency. It seems the mistake was to believe that AWS has consistency on naming of JSON fields (i.e. AWSNamingPolicy). It does not. Some services use Pascal-casing, such as CloudFormation:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/crpg-ref-responses.html

Automagically changing casing and not respecting the default System.Text.Json behavior is a fatal flaw, IMHO. Maybe consider releasing a Amazon.Lambda.Serialization.SystemTextJson.LambdaJsonSerializerV2 and put the old one on ice.

[bjorg]

To clarify, as I'm not familiar with how this works, the assembly attribute declaration is only used for deserialization, correct?

[assembly: LambdaSerializer(typeof(Amazon.Lambda.SystemTextJson.LambdaJsonSerializer))]

However, is it even needed if I use this handler signature?

Task<Stream> FunctionHandlerAsync(Stream stream, ILambdaContext context)

What happens if there is no LambdaSerializerAttribute declaration for the assembly or entry-point method?

[3GDXC]

@normj an option may be to add attributes to explicitly name the json properties and therefore respect the naming regardless of casing

[martincostello]

I'd agree with that suggestion - it's tedious one-off work to add them, but then they're always correct.

[3GDXC]

@normj IMHO this is an implementation issue that needs/should be address longer term as a BUG as this is a BREAKING CHANGE that was is unintentional

[normj]

@3GDXC I agree it is an implementation bug. Just thinking on the solution. Currently in the package we have one serializer called LambdaJsonSerializer. What if we add PascalCaseLambdaJsonSerializer and CamelCaseLambdaJsonSerializerserializer which extend from LambdaJsonSerializer. I could change the templates to use PascalCaseLambdaJsonSerializer to keep the existing behavior. It is sort of a more explicit version of @bjorg suggestion of having a LambdaJsonSerializerV2.

[3GDXC]

@normj IMHO it would be better an avoid confusion to add the JsonPropertyName attributes to the messages so that regardless of serializer configuration/options the resulting Json adhered to the attribute naming.

I fully understand why your reluctant to introduce a further breaking change (with the correction) within a matter of days from release of the .NET 3.1 support; and if we (royal we) had a number of UP-FOR-GRABS issues no unit/regression testing the community could assist with these to make implementation evolve and test prior to release.

Happy to assist if/when needed just say the word.

great work thus far, loving seeing aws lamba and .net core support grow

[normj]

@3GDXC Keep in mind the casing only affects return objects where we have to go from .NET object to JSON string. In the few response objects we vend I do use the JsonPropertyName, here is an example: https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.APIGatewayEvents/APIGatewayProxyResponse.cs#L18

The problem is for response objects that other people create where I don't control whether they use the JsonPropertyName attribute or not.

[3GDXC]

@normj good point; may be the advisory should also include ALWAYS use JsonPropertyName attributes to enforce explicit naming of your properties and/or data contracts (best practices)

[3GDXC]

@normj an alternative may be to abstract away the JsonSerializerOptions into a LambdaSerializerOptions class and add the options as a constructor parameter in the attribute so the serializer can have custom options that developer can override on an assembly/method level

[bjorg]

What about flagging it as a regression and fixing it as a breaking change now instead of spreading more harm? I call it harm because LambdaJsonSerializer, which is based on System.Text.Json, does not respect the default behavior of how to serialize properties. Of course using [JsonPropertyName] fixes it, but requiring everyone to do something to counter an unwanted behavior seems heavy handed.

How many people will continue to encounter this as ?!?!? moment as they adopt LambdaJsonSerializer as their standard serializer?

[panodk]

Hi there I'm encountering this problem in Step Functions after switching the lambda Tasks over to .Net 3.1 + the new serializer. It's wreaking havoc because the output now is in camelcase so the next state machine shape tries to evaluate the new JSON using the Amazon States Language and throws a Step Function exception.

There is a a kludgy workaround for the moment. By setting the LAMBDA_NET_SERIALIZER_DEBUG=true in the environment variable, the _.options is never set in the serializer causing the case to be returned untouched. I'm not sure if that will result in other repercussions other than additional JSON being emitted into the Cloudwatch Logs.
https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.Serialization.SystemTextJson/LambdaJsonSerializer.cs#L69-L90

IMO, it would be a pain to decorate all of our models with the [JsonPropertyName] decoration as our models are buried in a number of nuget libraries. Ideally I'd like the default behavior to return to the original PascalCasing as before but I'm fine with using an explicit PascalCaseLambdaJsonSerializer with the lambdas when it is called in our Step Function project.

Thanks!

[bjorg]

I'm pretty sure that kludgy workaround is a bug.

Good point on data-structures defined by upstream assemblies.

[martincostello]

I don't know what side effect it would have to other things without [JsonPropertyName] attributes, but using the LambdaJsonSerializer constructor that allows you to customise the JSON serializer can be reverted to the default PascalCase behaviour by setting JsonSerializerOptions.PropertyNamingPolicy to null.

[bjorg]

Linking to issue #628 since it's related to this discussion.

[bjorg]

I think this is related.

Looking at APIGatewayProxyRequest.cs, I noticed there are no [JsonPropertyName] annotations. The only reason this works is because a) LambdaJsonSerializer defaults to case-insensitive deserialization and b) LambdaJsonSerializer uses camel-casing on serialization.

I can see how this saved a lot of hours on annotating all the request/response classes in the various helper assemblies, but it means that when we use the helper assemblies, we have to use LambdaJsonSerializer in our functions.

In hindsight, wouldn't it make more sense to put the [LambdaSerializer(typeof(Amazon.Lambda.Serialization.Json.JsonSerializer))] annotation on the POCO class used by the function handler rather than the function class itself? It seems that, ultimately, the function must use the serializer matching the request/response classes.

[grahamehorner]

@bjorg IMO the POCO is the wrong place to have attribute metadata about the serialization that is to be used; the POCO should only be concerned with it's domain i.e. the model validation attributes and property type/naming; serialization should respect/use these model annotations.

IMHO the LambdaSerializer attribute should be changed to accept the type of serialization eg. Amazon.Lambda.Serialization.Json.JsonSerializer with an optional serialization options; if the option is not supplied default & compatible settings would be used.

[bjorg]

But the POCO needs to be annotated with the correct serializer attributes: [JsonProperty] for Newtonsoft and [JsonPropertyName] for System.Text.Json. Consequently, the POCO is tied to the serializer.

[bjorg]

It looks like [DataMember] will be supported by both Newtonsoft.Json and System.Text.Json. However, not until .NET 5 for the latter. :(
dotnet/runtime#29975

In the meantime, a solution would be to annotate all POCOs with [DataMember] and [JsonPropertyName]. Both attributes are defined in .NET Core 3.1 and therefore don't require additional external dependencies.

Wouldn't this ensure consistent serialization/deserialization for all classes, at least for property names? Converters would need to be registered by the ILambdaSerialize implementations.

[martincostello]

The linked issue there is closed. My understanding from skim reading that thread is they don’t intend to support it: dotnet/runtime#29975 (comment)

[bjorg]

Yep. I misread. I saw the 5.0 link and jumped to the wrong conclusion.

[normj]

I published a PR #636 to address the issue. I would appreciate feedback or better yet downloading the preview build from the link in the PR and help verifying the change works for you.

[bjorg]

First, thanks for tackling this so quickly. Sorry that it ruined your Saturday.

At first glance, it looks good. I'm currently in the process to ripping out all Newtonsoft.Json references and I'm not in a state where I can verify the fix, unfortunately. For now, I simply copied the problematic serializer class and removed the offending statement. Hopefully by tomorrow EOD, I can test this change in my dev branch.

The first thing that comes to mind is potentially missing annotations. Were there any response data-structures that did not use [DataMember] and instead relied on the implicit camel-casing?

[normj]

@bjorg No worries. After a week of meetings, writing docs and helping kids with school it was very comforting to have a few quiet moments and do some Saturday coding.

We had the potential of missing [DataMember] annotation with the Newtonsoft serializer. I'm not too worried about that because for known types we do have the test for it. In this case my gap was missing tests on custom responses.

[bjorg]

Would it be possible to release a -rc1? The alternative, AFAIK, is for me otherwise hack on my .csproj files with the right compilation constants enabled. Is there another way?

[normj]

@bjorg In the PR there is a link to a zip file containing prebuilt NuGet packages, can you setup a local NuGet source and put the packages in there?

[bjorg]

Learned something new today: how to have a local feed. Turned out to be super easy in .NET Core (see SO article).

My most pertinent feedback is to expose _options as a protected/public Options property so that a derived class can use it as well.

Otherwise, everything great with this new code from my side. Thx!

[bjorg]

@normj let me know if/when there are updated nuget packages. Happy to give it another test run.

[phillip-haydon]

#544 (comment)

^ Is it because not using camel casing breaks API Gateway?

Cos Pascal works fine if the lambda is on ALB, but doesn't work on API Gateway, this inconsistency is confusing. How did this work prior to the move to system.text?

[lukebrowell]

This is a breaking change; The interface is not compliant. Due to lack of integration testing and/or a release candidate community review this violation of the interface segregation principle has slipped through. The longer this is left unresolved the greater the cost and wasted man-hours this will cause to AWS clients, resulting in reputational impact to AWS.

I recommend you move to mark this release as broken and discourage migration to 3.1 for all developers until a fix is agreed upon.
Furthermore I also recommend that any fix be discussed and fully tested by the community to reduce the likelihood of further compounding the problem

[normj]

@lukebrowell The work for the serializier was done in the public with the PR submitted back in January. #568 The PR attached a prebuilt package for testing that could have been done with custom Lambda runtimes.

We are discussing the fix here along with the PR and I welcome feedback on the proposed fix #636

I agree this is a breaking change and I'm disappointing it happen but I disagree on the severity you suggest. The issue only affects Lambda functions returning custom responses not all Lambda functions and the existing Amazon.Lambda.Serialization.Json works the same so I don't believe it is fair to say the whole 3.1 Lambda release is broken. But again I do understand the frustration and I'm sorry this bug slipped by.

I hope to push the changes in the PR earlier next week unless any significant feedback about the change comes in that causes the release to delay.

[normj]

@bjorg The PR has been updated with a link to preview2 of the prebuilt packages. https://normj-packages.s3.us-west-2.amazonaws.com/rework-serialization-preview2.zip

[grahamehorner]

@normj I realize the community is partly to responsible here with regards missing these issues as (we) gave pressure to release/support the .netcore 3.1 runtime as an official lambda image and had not reported this or gave feedback. IMHO while I understand your view with regards @lukebrowell comments I would agree in part with @lukebrowell and suggest that a unit of work be started (with full community involvement) to open up discussion around features/design of the aspnetcore lambda functions/services library with a view to addressing any shortcomings and/or bugs that have been identified in a manor that helps development moving forward, as this package feels a little of a rush job TBH.

[bjorg]

I would love to see a stronger community. I've been hanging out on awsdevelopers.slack.com, but the #dotnet channel is a tad quiet. Is there another place where Lambda .NET Core folks are congregating?

[grahamehorner]

@bjorg I'll be joining ;) see ya there (virtually)

[phillip-haydon]

@bjorg possible to get an invite?

[bjorg]

Getting an invitation link from the moderators. Will post it here.

[Kralizek]

Is it possible to keep this issue in topic?

[bjorg]

Agreed, let's keep this on topic. I created a community issue #647 on how to reach me to add you to the AWS slack group.

[normj]

Yes, I welcome suggestions on how to better setup community communication and where I can do better about my own communication and how I can get more involved.

[bjorg]

preview2 looks good to me.

[normj]

Version 2.0.0 of Amazon.Lambda.Serialization.SystemTextJson is out with the change. Main take away it update the serializer class to be DefaultLambdaJsonSerializer.

I also published a blog post that has a section describing the change.
https://aws.amazon.com/blogs/developer/one-month-update-to-net-core-3-1-lambda/

[BhangeeF16]

What Solved My problem :
I used [LambdaSerializer(typeof(Newtonsoft.Json.JsonSerializer))] on the Function Handler

and Assigned [JsonProperty("")] attribute to all POCO class Properties

and directly used the POCO class in the Handler Signature FunctionHandler(MyPocoClass input, ILambdaContext context)

The following is my Call in MAIN :

 private static async Task Main(string[] args)
    {
        Func<MyPocoClass , ILambdaContext, string> handler = FunctionHandler;
        await LambdaBootstrapBuilder.Create(handler, new DefaultLambdaJsonSerializer())
            .Build()
            .RunAsync();
    }

Following is My Handler Method:

[LambdaSerializer(typeof(Newtonsoft.Json.JsonSerializer))]
public static string FunctionHandler(MyPocoClass input, ILambdaContext context)
{
   // logic
}

MyPoco Class :

public class MyPocoClass
{
     [JsonProperty("MyProperty")]
     public string? MyProperty{ get; set; }
    
}