Substitution Data passed at the recipient level is not correctly passed to SparkPost API

[LeeHester]

It looks like you guys broke this at both the transmission level, and the recipient level. It seems you have fixed it in version 1.3.0, but only at the transmission level. The user level is still broken.

[darrencauthon]

@LeeHester What is the user level? Do you have an example of that?

In the meantime, if you stick with lower-case and snake case'd values, it should work fine.

[darrencauthon]

Just need a filter for this: https://github.com/SparkPost/csharp-sparkpost/blob/768a282180dff96fe338c8029bbca45fd2eadd6f/src/SparkPost/Recipient.cs#L19

[LeeHester]

Darren, do you work for SparkPost?

[darrencauthon]

@LeeHester Nope :)

[LeeHester]

Ok, I assumed you did because you were the largest contributor to their "official" C# SDK. In release 1.2.0, a bug was introduced where I believe the text case (snake_case, PascalCase, camelCase, etc) was changed with how anyonymous objects are serialized when when sending substitution data to SparkPost. There are two places to set substitution data: on the transmission itself, and on the recipient. Prior to 1.2.0, I would use PascalCased anonymous objects for my substitution data and they would get converted to camelCased JSON. All of my templates expect camelCased data. The 1.2.0 release seemed to change that behavior, although I'm not sure what type of casing it changed to. This was a disastrous change for me, because all my templates that used camelCased data no longer got the data in the format they expected. That caused literally millions of emails to our customers that weren't rendered correctly.
In version 1.3.0, it looks like you changed the casing rules again, back to what they were (camelCase, as far as I can tell), but only on substitution data specified on the transmission itself. The casing of the substitution data on the recipients hasn't been "fixed" (what would be fixed for me, which is putting back the rule that camelCased it, so it is back to how it was working before the 1.2.0 release).
I don't plan on making a pull request, because I don't plan on using this library any more. From my perspective, it is unstable, not integration tested, and has had two crippling changes in the last two "releases". I made the issue so that anyone who wants to use this is aware of the issue, and can fix it if they care to.

[darrencauthon]

@LeeHester I'm sorry for the problem you encountered. The problem was introduced by me... I switched to using a convention to convert this library's PascalCase objects to the snake_case keys accepted by the API. This rule got applied to the substitution data in 1.2.0. As soon as someone reported it to me, I had the fix out. And now that you told me about the other bit of substitution data, I'll have that fixed soon, too.

I'm responsible for the PascalCase => snake_case key change, but I never made any change to camelCase. I can't account for what happened there.

So just for the record, there was only one crippling update. 😄

And also for the record, there are integration tests that I run before each release. I verify email sending, template sending (with replace tags), and each of the features that have been slowly added. I don't check those integration tests into a test runner in the project because I don't know how to verify the results other than to... put in a pause and query a mailbox?

My integration tests didn't catch the bug because all of my tests (and production code) uses snake case replace tags. Tests will only cover what the author of the tests thought to cover. And I'm sorry, but I just didn't see how these two pieces fit together. But as soon as I do see them, we get the tests and we get it locked down so we don't have any regressions.

I have one production app with an email originally written for Mandrill and ALLCAPSREPLACETAGS. My emails have continued to work, even through 1.2.0, because I haven't updated the library past an earlier one. I haven't updated the library because I haven't had time to stop and test the new library against the email templates. I'm not going to update any library in my system without verifying that it hasn't broken the existing functionality.

I'm honestly sorry for causing you any issues, but you need to know for your own sake: Do not update libraries without verifying them in master. Not just this one, but any library. You are responsible for the emails you send. If I was blasting "literally millions of emails to customers" you should check just one of those few million emails before you press the Nuget "Upgrade" button. Especially for a library as young as this one.

If you want to make a library better, make it better. If you don't, don't. I've helped out as much as I can, and many .Net devs have also contributed code, information, and other help. We're just working together to make a solution that works for us. If our free help doesn't meet your standards, make your own. 🐱

[LeeHester]

Thanks for the lecture. I was just reporting a bug.

[darrencauthon]

Don't close it yet, the fix hasn't been applied yet. 😄

[darrencauthon]

@LeeHester

Thanks for the lecture. I was just reporting a bug.

You reported the bug, yes... but then took some shots at the library. Unstable, untested, etc.

The casing of the substitution data on the recipients hasn't been "fixed" (what would be fixed for me, which is putting back the rule that camelCased it, so it is back to how it was working before the 1.2.0 release)

Just to be clear, the library has never done an anything-to-camel-case, and never will. The everything-to-snake-case conversion was real, an accident, and has been fixed and covered by unit tests. And based on this ticket I've altered my integration tests to cover PascalCase, and the emails I received covered those values.

This ticket has been resolved with #55 and released on Nuget as version 1.3.1.

[darrencauthon]

Thank you for reporting the issue! 😄