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

add naming strategy option to JsonSerializer #525

Merged
merged 3 commits into from
Oct 24, 2019

Conversation

mabead
Copy link
Contributor

@mabead mabead commented Sep 6, 2019

Description of changes:

I propose to add a Newtonsoft naming strategy to the Amazon.Lambda.Serialization.Json.JsonSerializer. This makes it possible to change the naming strategy to camel case for example.

This is particularly usefull for people that follow the google guideline that says to format json documents in camel case and that also follow the C# guideline that says to name properties using pascal case.

Having a consistent naming strategy is particularly important when using step functions that are case sensitive.

Here's an example of the new behaviour:

class CamelCaseJsonSerializer : JsonSerializer
{
    public CamelCaseJsonSerializer()
        : base(_ => { }, new CamelCaseNamingStrategy())
    {
    }
}

class FooBar
{
    public int SomeValue { get; set; }
    public string SomeOtherValue { get; set; }
}

class Program
{
    private static string TestSerializer(JsonSerializer serializer)
    {
        var x = new FooBar
        {
            SomeValue = 12,
            SomeOtherValue = "1212",
        };

        var memoryStream = new MemoryStream(200);

        serializer.Serialize(x, memoryStream);
        memoryStream.Seek(0, SeekOrigin.Begin);

        StreamReader reader = new StreamReader(memoryStream);
        return reader.ReadToEnd();
    }

    static void Main(string[] args)
    {
        Console.WriteLine($"Default serialization:    {TestSerializer(new JsonSerializer())}");
        Console.WriteLine($"Camel case serialization: {TestSerializer(new CamelCaseJsonSerializer())}");
    }
}

This will write the following in the console:

Default serialization:    {"SomeValue":12,"SomeOtherValue":"1212"}
Camel case serialization: {"someValue":12,"someOtherValue":"1212"}

Possible improvement:

I think that using a CamelCaseJsonSerializer is a pretty common need. Should it be added to Amazon.Lambda.Serialization.Json?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@normj
Copy link
Member

normj commented Sep 18, 2019

Looks like a good addition. Would you mind adding a unit tests to the EventTests project?

@mabead
Copy link
Contributor Author

mabead commented Sep 18, 2019

@normj Good point. A unit test has been added.

@mabead
Copy link
Contributor Author

mabead commented Sep 19, 2019

@normj What do you think about the idea of adding the CamelCaseJsonSerializer directly in Amazon.Lambda.Serialization.Json?

@normj
Copy link
Member

normj commented Sep 20, 2019

I'm okay adding CamelCaseJsonSerializer to Amazon.Lambda.Serialization.Json but it will need all the other constructors from the base class and clear documentation on when to use it. If somebody is using Lambda for service events like S3 upload then I don't want anybody to accidentally use CamelCaseJsonSerializer. I assume your primary use case is using Lambda with your own custom events and response objects or with Step Functions.

@mabead
Copy link
Contributor Author

mabead commented Sep 20, 2019

@normj You're right about my primary use case.

Regarding CamelCaseJsonSerializer , you clearly know more than I do about the impacts of camel casing vs pascal casing. I am therefore not confident that I could document the impacts and use cases fully, clearly and correctly.

For this reason, please ignore my suggestion of adding CamelCaseJsonSerializer to Amazon.Lambda.Serialization.Json. I conside my PR to be complete at this point.

Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. I'll get it merged in when do the next release.

@mabead
Copy link
Contributor Author

mabead commented Oct 7, 2019

@normj Do you have any rough idea of when you will create the next release? I am waiting for this to use it. No rush, I just want to have an idea of when this will be available.

@normj normj merged commit 118f3bd into aws:master Oct 24, 2019
@mabead mabead deleted the json-naming-strategy branch October 24, 2019 12:58
@normj
Copy link
Member

normj commented Oct 25, 2019

Version 1.7.0 of Amazon.Lambda.Serialization.Json has been released with this PR.

@mabead
Copy link
Contributor Author

mabead commented Oct 25, 2019

Thanks @normj . FYI, I wrote about this new capability.

@normj
Copy link
Member

normj commented Oct 25, 2019

The blog post is great! Hope you don't mind I tweeted out your post. I didn't see you on twitter to tag you on the tweet.

https://twitter.com/socketnorm/status/1187828850769592320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants