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: Allow SendGridMessage serialization to optionally bypass defaults #1064

Merged
merged 1 commit into from
Nov 16, 2020
Merged

fix: Allow SendGridMessage serialization to optionally bypass defaults #1064

merged 1 commit into from
Nov 16, 2020

Conversation

duncanchung
Copy link
Contributor

Fixes #1063

Fixes

Restores serialization of the SendGridMessage to that prior to #938 , but also adds the ability to bypass custom serialization settings (see #937 )

Will add discussion in comments.

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 Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 6, 2020
@duncanchung
Copy link
Contributor Author

It seems my pull request #938 was actually backwards incompatible.

The core of the issue is the CamelCasePropertyNamesContractResolver (which I would say is commonly used - though I don't know why there are only 2 reported issues on #938 ).

When used, it camel-cases dictionary keys (so you can never control the casing on e.g. email headers) and affects property names (e.g. template data).

Now, the only way to get control back on the email headers is to effectively reset the serialization (which was #938 ) but then e.g. all your template data won't match. (e.g. your template expects firstName and you are now passing FirstName).

This pull request is just a backwards compatible way to do #938 , defaulting to existing behavior and predictable .Net conventions (ie. serialization follows the application's JsonConvert.DefaultSettings [if modified]), with an explicit override on the Serialize method to reset back to NewtonSoftJson defaults. This way, you opt in to the reset of the serialization, and therefore opt in to resetting all templates, etc. to follow untainted NewtonSoftJson defaults.

@thinkingserious
Copy link
Contributor

Thank you for the quick turn around on the fix!

@thinkingserious thinkingserious merged commit 4ad3430 into sendgrid:main Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SendGridMessage serialization changed (backwards incompatible)
2 participants