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

Amazon.Lambda.CloudWatchEvents definition doesn't work with Newtonsoft.Json when compiling on .NET Core 3.1 #634

Closed
bjorg opened this issue Apr 18, 2020 · 11 comments
Assignees
Labels
bug This issue is a bug. module/lambda-client-lib p2 This is a standard priority issue queued

Comments

@bjorg
Copy link
Contributor

bjorg commented Apr 18, 2020

For .NET Core 3.1 compilation target, Amazon.Lambda.CloudWatchEvents automatically assumes that the function uses System.Text.Json. However, if it uses Newtonsoft.Json like before, because only the target framework was changed, then the DetailType property is not properly deserialized.

The problematic line is here: https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.CloudWatchEvents/CloudWatchEvent.cs#L43

@normj
Copy link
Member

normj commented Apr 18, 2020

I'm not sure why you would see any change in behavior when targeting .NET Core 3.1. There wasn't any special logic for DetailType and the unit test that tests both serializer is passing with a value for that field. Can you include a sample of the event you are seeing?

https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/test/EventsTests.Shared/EventTests.cs#L1249-L1266

@bjorg
Copy link
Contributor Author

bjorg commented Apr 18, 2020

Maybe I jumped the gun. Here's what I saw in the CloudWatch logs when I emitted the received event. Notice how DetailType is null.

{
    "Version": "0",
    "Account": "xyz",
    "Region": "us-west-2",
    "Detail": {
        "Message": "hello world!"
    },
    "DetailType": null,
    "Source": "LambdaSharp.Sample",
    "Time": "2020-04-18T01:15:39Z",
    "Id": "ab26e799-0b3b-637c-2f96-6a428401fdf3",
    "Resources": [
        "lambdasharp:stack:SteveBvNext-Sample-Event",
        "lambdasharp:module:Sample.Event",
        "lambdasharp:tier:SteveBvNext"
    ]
}

This is the actual event that was sent:

{
  "version": "0",
  "id": "ab26e799-0b3b-637c-2f96-6a428401fdf3",
  "detail-type": "MyFirstEvent",
  "source": "LambdaSharp.Sample",
  "account": "xyz",
  "time": "2020-04-18T01:15:39Z",
  "region": "us-west-2",
  "resources": [
    "lambdasharp:stack:SteveBvNext-Sample-Event",
    "lambdasharp:module:Sample.Event",
    "lambdasharp:tier:SteveBvNext"
  ],
  "detail": {
    "Message": "hello world!"
  }
}

This is the code I used, following the pre-3.1 pattern:

[assembly: LambdaSerializer(typeof(Amazon.Lambda.Serialization.Json.JsonSerializer))]

namespace Sample.Event.ReceiverFunction {

    public class EventDetails {

        //--- Properties ---
        public string? Message { get; set; }
    }

    public class FunctionResponse { }

    public class Function : ALambdaFunction<CloudWatchEvent<EventDetails>, FunctionResponse> {

        //--- Methods ---
        public override async Task InitializeAsync(LambdaConfig config) { }

        public override async Task<FunctionResponse> ProcessMessageAsync(CloudWatchEvent<EventDetails> request) {
            LogInfo(SerializeJson(request));
            return new FunctionResponse();
        }
    }
}

For context, SerializeJson uses the Amazon.Lambda.Serialization.Json.JsonSerializer.

Full code: https://github.com/LambdaSharp/LambdaSharpTool/blob/WIP-v0.7-DotNetCore31/Samples/EventSample/ReceiverFunction/Function.cs

It's certainly possible that the issue was present before 3.1. I didn't test it until now.

@bjorg
Copy link
Contributor Author

bjorg commented Apr 29, 2020

I think I figured it out!

The problem stems from this line line:

else if (type.FullName.StartsWith("Amazon.Lambda.CloudWatchEvents.")

The code requires that the serialized type MUST belong to the Amazon.Lambda.CloudWatchEvents namespace. I was able to confirm this by creating two version of the same function, which should behave identically, but don't.

Buggy Version

In the first version, I use CloudWatchEvent<T> directly and it does not work. DetailType is logged as an empty string (null).

namespace CloudWatchEventListener {

    public class EventDetails {

        //--- Properties ---
        public string Message { get; set; }
    }

    public class Function {

        //--- Methods ---
        public string FunctionHandler(CloudWatchEvent<EventDetails> request, ILambdaContext context) {
            Console.WriteLine($"DetailType = {request.DetailType}"); // <-- DetailType is empty
            return "Okay";
        }
    }
}

Fixed Version

In this version, I created a derived type in the Amazon.Lambda.CloudWatchEvents namespace and it worked as expected.

namespace Amazon.Lambda.CloudWatchEvents {

    public class MyCloudWatchEvent : CloudWatchEvent<CloudWatchEventListener.EventDetails> { }
}

namespace CloudWatchEventListener {

    public class EventDetails {

        //--- Properties ---
        public string Message { get; set; }
    }

    public class Function {

        //--- Methods ---
        public string FunctionHandler(MyCloudWatchEvent request, ILambdaContext context) {
            Console.WriteLine($"DetailType = {request.DetailType}"); // <-- DetailType is shown
            return "Okay";
        }
    }
}

I created a repository with both function version. Hopefully that helps in reproducing and fixing this issue. Assuming it's still relevant since it only applies to Newtonsoft.Json serialization.
https://github.com/bjorg/CloudWatchEventSerializationBug

@bjorg
Copy link
Contributor Author

bjorg commented Oct 14, 2020

Any hope of getting this fixed eventually?

@ashishdhingra ashishdhingra added bug This issue is a bug. B labels Nov 5, 2020
@ashishdhingra
Copy link
Contributor

Probable fix is to remove condition type.FullName.StartsWith("Amazon.Lambda.CloudWatchEvents.") in the code

else if (type.FullName.StartsWith("Amazon.Lambda.CloudWatchEvents.")

Needs review from development team.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2022

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Feb 5, 2022
@ashishdhingra
Copy link
Contributor

Do not close.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Feb 6, 2022
@jamesbascle
Copy link

jamesbascle commented Jun 9, 2022

This is causing me an issue right now as well. The offending line is in fact the second part of the condition mentioned in #634 (comment) since the base type of, for instance, CloudWatchEvent<JObject>, is just Object, not CloudWatchEvent<>. It's just totally faulty logic. The only way I've been able to get around it is by making my own class and hacking it into the AWS namespace as follows:

namespace Amazon.Lambda.CloudWatchEvents
{
    public class MyHackyCloudWatchEvent<T> : CloudWatchEvent<T>
    {
    }
}

which is, obviously, fairly weird.

I think what the author was intending to do was check that the current type is a reification of an open generic type CloudWatchEvent<T>, which would entail calling type.GetGenericTypeDefinition() and checking if that is typeof(CloudWatchEvent<>).

@ashishdhingra ashishdhingra added queued p2 This is a standard priority issue and removed B labels Nov 2, 2022
@ashishdhingra
Copy link
Contributor

Reproducible using customer provided sample code. The condition at

else if (type.FullName.StartsWith("Amazon.Lambda.CloudWatchEvents.")
appears to be wrong.

Instead the condition should be logical OR since customer could use provided CloudWatchEvent type in Amazon.Lambda.CloudWatchEvents namespace OR create a derived type from the CloudWatchEvent class in custom namespace.

@ashishdhingra
Copy link
Contributor

Fix released in Amazon.Lambda.Serialization.Json 2.2.2

Copy link
Contributor

github-actions bot commented Aug 1, 2024

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
bug This issue is a bug. module/lambda-client-lib p2 This is a standard priority issue queued
Projects
None yet
Development

No branches or pull requests

4 participants