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

fix: Ensures the serialization of the SendGridMessage is untainted by defaults set by applications on the JsonSerializer #938

Merged
merged 2 commits into from
May 12, 2020

Conversation

duncanchung
Copy link
Contributor

Fixes

Fixes #937

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Ensures the serialization of the SendGridMessage is untainted by defaults set by applications on the JsonSerializer.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Sep 29, 2019
@duncanchung
Copy link
Contributor Author

If anyone can tell me what failed the Travis build, I'm happy to fix.

I believe the error responsible is:

$ curl -s https://codecov.io/bash > .codecov
curl: (7) Failed to connect to codecov.io port 443: Connection timed out
The command "curl -s https://codecov.io/bash > .codecov" exited with 7.

I don't know how to re-run the build (I'm guessing a networking issue caused the check to fail?).

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty type: bug bug in the library labels Oct 9, 2019
@childish-sambino childish-sambino changed the title Ensure SendGridMessage serialization is unaffected app fix: Ensures the serialization of the SendGridMessage is untainted by defaults set by applications on the JsonSerializer May 12, 2020
@childish-sambino childish-sambino merged commit 1f5c7ce into sendgrid:master May 12, 2020
@danparker276
Copy link

This fix broke my application. It doesn't camelcase anymore, please change back or fix. Should be at least backwards compatible. I'll investigate why this is happening more.
Setting this in the constructor doesn't work anymore:
JsonConvert.DefaultSettings = () => new JsonSerializerSettings { ContractResolver = new CamelCasePropertyNamesContractResolver() };

@duncanchung
Copy link
Contributor Author

Hi @danparker276

I've just tested (on my original branch) that default serialization is unaffected. This passes.

        [Fact]
        public void TestKitchenSinkIsUnaffectedByCustomContractResolver()
        {
            var originalGetDefaults = JsonConvert.DefaultSettings;

            JsonConvert.DefaultSettings = () => new JsonSerializerSettings
            {
                ContractResolver = new Newtonsoft.Json.Serialization.CamelCasePropertyNamesContractResolver()
            };

            try
            {
                TestKitchenSink();

                // Ensure default behavior is not broken
                var testObject = new {
                    PropertyName = "PropertyValue",
                    Dictionary = new Dictionary<string, string> { { "DictionaryKey", "DictionaryValue" } }
                };
                var expectedValue = "{\"propertyName\":\"PropertyValue\",\"dictionary\":{\"dictionaryKey\":\"DictionaryValue\"}}";
                var actualValue = JsonConvert.SerializeObject(testObject);
                Assert.True(expectedValue == actualValue);
            }
            finally
            {
                JsonConvert.DefaultSettings = originalGetDefaults;
            }
        }

I can't get this to run in the latest just yet (.net core framework conflicts). Will get a pull request to get this merged when I get it to run (to ensure defaults aren't broken).

I'm curious how we can reproduce your issue.

@mfarnz
Copy link

mfarnz commented Nov 5, 2020

This breaks all of our email templates, which were expecting camel-case JSON. Variables are now coming across in Pascal case. Shouldn't this, at least, be configurable? Or am I missing something?

@duncanchung
Copy link
Contributor Author

@mfarnz I understand the issue.

By fixing #937 (by resetting the serialisation to cater for dictionaries used for email headers) I've broken email template parameters (most probably passed via dictionaries as well).

Let me confirm this.

@duncanchung
Copy link
Contributor Author

@mfarnz I don't think there's one solution to fix the serialization of both email headers and email template parameters when using the CamelCasePropertyNamesContractResolver.

So, here's my fix so that you can choose what you want to do - backwards compatible for your work.

Let me file a bug, and suggest this as a fix - see if it gets approved.

@mfarnz
Copy link

mfarnz commented Nov 6, 2020

@duncanchung thanks for the update. The change looks good to me.

@duncanchung
Copy link
Contributor Author

Issue raised: #1063

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio type: bug bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization issue if non-defaults used
5 participants