Fix STJUnixDateTimeConverter to handle null values for nullable DateTime properties#3280
Closed
jmtdev0 wants to merge 1 commit intostripe:masterfrom
Closed
Fix STJUnixDateTimeConverter to handle null values for nullable DateTime properties#3280jmtdev0 wants to merge 1 commit intostripe:masterfrom
jmtdev0 wants to merge 1 commit intostripe:masterfrom
Conversation
…ime properties Fixes stripe#3157 - Convert STJUnixDateTimeConverter to JsonConverterFactory pattern - Add JsonConverterFactory support to SerializablePropertyCache - Add comprehensive unit tests (9 tests, all passing)
|
JMTDESKTOP25\jmart seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
1 similar comment
|
JMTDESKTOP25\jmart seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request: Fix STJUnixDateTimeConverter to handle null values for nullable DateTime properties
Why?
Fixes #3157
When deserializing a
Stripe.Eventpayload usingSystem.Text.Json, nullableDateTime?properties throw aJsonExceptionwhen the JSON value isnull. This affects common webhook scenarios where properties likecanceled_at,trial_end, orended_atare null.Error:
What?
This PR makes two changes:
1. Convert
STJUnixDateTimeConverterto aJsonConverterFactoryThe converter is now a factory that creates specialized converters for each type:
STJUnixDateTimeConverterImplfor non-nullableDateTimeSTJUnixNullableDateTimeConverterfor nullableDateTime?The nullable converter properly handles
JsonTokenType.Nulland returnsnullinstead of throwing an exception.2. Add
JsonConverterFactorysupport toSerializablePropertyCacheThe
GetConvertermethod now detects when a property-level[JsonConverter]attribute specifies aJsonConverterFactorytype and handles it appropriately by callingCreateConverterwith the property's actual type.Testing
Added comprehensive unit tests in
STJUnixDateTimeConverterTest.cs:Test execution output (click to expand)
All 9 tests pass without requiring stripe-mock.
Files Changed
src/Stripe.net/Infrastructure/JsonConverters/STJUnixDateTimeConverter.cs- Converted to JsonConverterFactory pattern with specialized converterssrc/Stripe.net/Infrastructure/JsonConverters/SerializablePropertyCache.cs- Added support for JsonConverterFactory in property-level attributessrc/StripeTests/Infrastructure/JsonConverters/STJUnixDateTimeConverterTest.cs- New comprehensive test suiteChangelog
STJUnixDateTimeConverternow properly handles null values for nullableDateTime?properties when deserializing JSON withSystem.Text.Json.Journey with AI Assistance
This fix was developed through an interactive conversation with a coding agent. Here's the chronology of how we arrived at this solution:
TL;DR: Claude Opus initially attempted a simple fix (just adding null handling). During the investigation, we discovered the
origin/prathmesh/stj-datetimeconvertorbranch that already contained a comprehensive solution. After running our tests, we realized the simple approach wasn't sufficient due toJsonConverter<DateTime>return type constraints, and we needed theJsonConverterFactorypattern—essentially arriving at the same solution already implemented in that branch.Phase 1: Issue Discovery & Analysis
STJUnixDateTimeConverter.Read()doesn't handleJsonTokenType.NullUnixDateTimeConverter) which correctly handles nullPhase 2: Differentiating Related Issues
Phase 3: Reproduction & Test Creation
Phase 4: Investigation of Existing Work
origin/prathmesh/stj-datetimeconvertorbranch with a more comprehensive solutionJsonConverterFactoryapproachSerializablePropertyCacheneeded modification to support factoriesPhase 5: Implementation
JsonConverter<DateTime>return type constraintsJsonConverterFactoryis necessary for proper null handling withDateTime?SerializablePropertyCachePhase 6: Validation
Acknowledgments
Thanks to @sebastiencrevier for the detailed bug report with reproduction steps, and to @prathmesh-stripe and @jar-stripe for their work and collaboration in addressing this issue.