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

Change StripeDateTimeConverter to write epoch not MS /Date(.)/ format #1144

Merged
merged 1 commit into from
Mar 23, 2018
Merged

Change StripeDateTimeConverter to write epoch not MS /Date(.)/ format #1144

merged 1 commit into from
Mar 23, 2018

Conversation

barclayadam
Copy link
Contributor

This closes #208 that I created over 3 years ago. I'm using Stripe on another project and have run into the same issue.

Stripe API does not support the /Date(...)/ format the JSON converter writes, which is why this converter is not placed on properties that are written to Stripe, only those that are read-only.

This PR allows the objects returned from Stripe to be stored locally as JSON and then rehydrated. We do this to have a cache locally of some of the objects to avoid repeated calls to Stripe to get the required information.

@brandur
Copy link
Contributor

brandur commented Mar 22, 2018

Thanks @barclayadam! We appreciate you taking the time to put in a few tests

We're relatively new maintainers for this library, and still trying to work out a few of the quirks — but out of curiosity, do you have any idea why stripe-dotnet was doing this in the first place? I'm a little afraid that someone's depending on the currently exported string format somewhere.

BTW, it appears that your added tests are timezone-sensitive. Here's the CI failure:

Failed   Stripe.Tests.Xunit.datetime_json_converter.should_roundtrip_datetimes
Error Message:
 Expected <2018-03-22 08:15:03>, but found <2018-03-22 15:15:03>.
Stack Trace:
   at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message) in C:\projects\fluentassertions-vf06b\Src\FluentAssertions.Net45\Execution\XUnit2TestFramework.cs:line 32
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args) in C:\projects\fluentassertions-vf06b\Src\Core\Execution\AssertionScope.cs:line 197
   at FluentAssertions.Primitives.NullableDateTimeAssertions.Be(Nullable`1 expected, String because, Object[] becauseArgs) in C:\projects\fluentassertions-vf06b\Src\Core\Primitives\NullableDateTimeAssertions.cs:line 109
   at Stripe.Tests.Xunit.datetime_json_converter.should_roundtrip_datetimes() in C:\projects\stripe-dotnet\src\Stripe.Tests.XUnit\_infrastructure\datetime_json_converter.cs:line 20

It might be best to try and use UTC everywhere.

@barclayadam
Copy link
Contributor Author

@brandur I made the change yesterday to UTC to fix that test.

To be honest I have no idea why the WriteJson method was using the MS format. My gut is a copy-paste job and the write method was never updated/changed to match the semantics of Stripe being Unix Epoch.

I'd be surprised if people are relying on this as they would have had to have workarounds to use the same objects to reload but it is possible they could be dumping the JSON out and reading it in some other, more manual way.

@brandur-stripe
Copy link
Contributor

I'd be surprised if people are relying on this as they would have had to have workarounds to use the same objects to reload but it is possible they could be dumping the JSON out and reading it in some other, more manual way.

Yeah, agreed on that — thanks for getting back to me.

This change seems pretty sane to me.

@brandur-stripe brandur-stripe merged commit 9400970 into stripe:master Mar 23, 2018
@brandur-stripe
Copy link
Contributor

I don't think (and hoping somewhat) that there won't be enough users relying on this behavior to justify a major release. I ended up releasing it as 15.1.0 (a minor bump).

@jaymedavis
Copy link
Contributor

My gut is a copy-paste job and the write method was never updated/changed to match the semantics of Stripe being Unix Epoch.

I think you're right. Who wrote this crap.

@barclayadam
Copy link
Contributor Author

@brandur-stripe Thanks for the quick turnaround on this 👍

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.

JSON round-tripping
4 participants