[release/8.0] Use invariant culture in CBOR date encoding #92924
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tagging subscribers to this area: @dotnet/area-system-formats-cbor, @bartonjs, @vcsjones Issue DetailsBackport of #92539 to release/8.0 /cc @eiriktsarpalis @filipnavara Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
|
Is there any reasonable way (egg a relevant analyzer, or grep) to attempt to scan the Cbor code as a whole for any other place culture is assumed? Presumably they would be bugs. Does changing local culture to a highly bogus custom culture cause any tests to fail? |
eiriktsarpalis
changed the title
eiriktsarpalis
added
the
Servicing-consider
TL;DR yes, I just pushed changes that incorporate testing to the change as well. |
|
Sorry, I meant does it make any tests fail even after the fix - indicating current culture is being used elsewhere in the code. Not sure the easiest way to apply a custom (or at least non US) culture to all tests, maybe in a static ctor on some core cbor type? Just throwing this idea out there. |
I think we're using machines configured with non US cultures in some of our CI legs, but guessing none of them have a non-Gregorian calendar which is what triggers the particular issue. |
That's how the issue was originally discovered. I had my machine set to a really weird culture and I run all the CBOR tests with it. The fix made the remaining tests pass. |
|
Ah - we're set then. (And setting the machine culture is easier than code, doh) Btw do all libs test pass with this culture? In the past I've had to fix tests from time to time because they failed in eg Russia. |
|
Perhaps they were outer loop ones, given the CI coverage mentioned. Although IIRC that is something "not that different" to en-US --maybe es-ES. |
I didn't get to test that yet... we totally should do it though. Maybe file an issue so we don't forget to do it? |
|
@eiriktsarpalis - I am not familiar with CBOR, and I assume this is NOT the case, but is there any stream version indicator that we or a user could rev in reaction to this change so that someone could chose to remain compatible with the old behavior for loading old streams? |
|
Other than information that users choose to add to their own schema, the CBOR format itself doesn't have any versioning attached to its encoding. |
|
Approved for me. |
carlossanlop
added
Servicing-approved
|
Approved by Tactics via email. |
carlossanlop
added
the
needs-breaking-change-doc-created
|
Added the |
|
It's tracked by #92595 so we can remove it from here. |
eiriktsarpalis
removed
the
needs-breaking-change-doc-created
ghost
locked as resolved and limited conversation to collaborators
Backport of #92539 to release/8.0
/cc @eiriktsarpalis @filipnavara
Customer Impact
Since it was released in .NET 5, the
System.Formats.CborNuGet package included built-in methods for serializing and deserializingDateTimeOffsetvalues according to RFC 7049. Unfortunately, the implementations weren't using invariant culture when formatting/parsingDateTimeOffsetvalues. This can result in inconsistent or even incorrect date encodings in machines with cultures using non-Gregorian calendars.Even though the bug has been present since the release of the library, consensus is that we'd like this to be backported to .NET 8 so that a fixed LTS release becomes available to customers.
Testing
Adding unit testing validating the fix.
Risk
Moderate. While the product fix is small and straightforward, this is a breaking change for users that rely on the existing behavior to deserialize encodings created by earlier versions of the library. We will document this as a breaking change and provide workarounds, however it was deemed that a compatibility switch is not necessary, given that the library is an out-of-band package that can be updated independently of the framework.