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

Proposal - add netcoreapp3.0 multi target support and replace Newtonsoft.Json #497

Closed
msimpsonnz opened this issue Jul 31, 2019 · 6 comments
Labels
guidance Question that needs advice or information.

Comments

@msimpsonnz
Copy link

I know there has been some work done already to support .NET Core 3 runtime and issues with installing preview software in the build system, however, I would like to propose the following PR

With .NET Core 3.0 due for release in September 2019 and the LTS version 3.1 due in November 2019, I would like to propose a PR that will setup multi target support for .NET Core 3.0 and replace the current Newtonsoft library with the new System.Text.Json.
The .NET Team published an article a while back addressing the future of JSON in .NET Core 3.0, where they talk about removing the dependency on Json.NET and providing a high performance API.
I have built a proof with a basic scenario which showed an drop in total Lambda duration time, implementation is here.

Below is a list of core Libraries that need to be updated:

  • Amazon.Lambda.Serialization.Json
  • Amazon.Lambda.APIGatewayEvents
  • EventsTests

There are a number of tests and tools that are using various Newtonsoft versions, so would suggest we leave this alone in the first draft of the PR. The aim would be to multi target so this will not create a breaking change for people using an older version of the framework.

@msimpsonnz
Copy link
Author

msimpsonnz commented Jul 31, 2019

I've had a first crack at this

  • By adding the JsonSerializationProperty for Camel Case I managed to get 9 tests passing out 28 from the EventsTests
  • Rather that a custom contract resolver in the main function, we could handle naming issues by exception? I added [JsonPropertyName("detail-type")] to CloudWatchEvent.cs to fix the property name with a dash and this fixed a further 4 tests to bring the total to 15
  • I wanted to get something working so I haven't looked at the custom functions needed to get the stream and binary data, at the moment I have compiler checks to only do them for the current 1.3 and 2.0 Standard libs

The other tests that are failing are
*Amazon.Lambda.RuntimeSupport.IntegrationTests.CustomRuntimeTests.TestAllHandlersAsync

I was also getting an error about 'Microsoft.NETCore.App', version '1.0.4' which is out of support and probably needs a look

My fork and branch are here
https://github.com/msimpsonnz/aws-lambda-dotnet/tree/netcore3json

@msimpsonnz
Copy link
Author

msimpsonnz commented Jul 31, 2019

I've done a bit more work on this, I moved to .NET Core Preview 7 which had some breaking changes in the JSON API, so I fixed these up

  • Now have 22 tests passing out of 28, used the method above to target [JsonPropertyName]
  • 6 failing tests all using custom deserilizers for streams ect, not going to do any more until agree on a way forward above

@msimpsonnz
Copy link
Author

Ok, so I went down the rabbit hole on this one. Looks like some of the older Events use a different pattern to the newer ones that have been added for example KinesisEvent vs KinesisFirehose.
The main issue with the failing tests at the moment is the upstream SDK Models being delivered as MemoryStream and this is where the DataContractResolver was used with the Json.NET implimentation
I've re-factored KinesisEvent to mimic KinesisFirehose, bring the Data model in from the upstream SDK and doing any custom conversions, such as base64 or epoch in that library.
This looks cleaner to me than having a default contracts library as there are only a 5 or 6 event sources this affects, but these checks are being done for every call.
So now I have 23 tests passing out of 28
I'll take a look at the others if this is the direction it is going?

@normj normj added the guidance Question that needs advice or information. label Aug 2, 2019
@normj
Copy link
Member

normj commented Aug 2, 2019

Thanks for looking into the new System.Text.Json NuGet package. It has been on my list of things to do but hadn't gotten around to it.

In my head when I have been thinking about this I had imagined a new NuGet package based on System.Text.Json instead of trying to combine them into the same serializer. That way the new NuGet package wouldn't have any of the baggage of Newtonsoft.Json. It would still need to support all of the same Event serializers and I hadn't figured out how the naming would work. Doing it as a separate package gives consumers the choices of which serializers to use based on which dependency they add and serializer they registered.

@msimpsonnz
Copy link
Author

All good, after looking at this in more detail, I think this could use a wider discussion. Naming is a good point as there is a collision with the current namespace for the serializer.

With regard to the supported AWS events, would it be worth seeing if this could be pushed up into the AWSSDK? The S3 Notification has some JSON parsing methods and this is using another different tool LitJson, could these be used as we are inheriting these classes
Where Events from ALB just need a "simple" JSON contract and don't need the extra baggage of going though a custom serializer to catch events that are not related

@msimpsonnz
Copy link
Author

Looks like Norm is working on this in #568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

2 participants