Restore backward-compatible reader positioning in JsonSerializerPrimitives#3430
Merged
JoshLozensky merged 6 commits intodevfrom Mar 11, 2026
Merged
Restore backward-compatible reader positioning in JsonSerializerPrimitives#3430JoshLozensky merged 6 commits intodevfrom
JoshLozensky merged 6 commits intodevfrom
Conversation
…d compatible reader positioning Co-authored-by: JoshLozensky <103777376+JoshLozensky@users.noreply.github.com>
…pat tests Co-authored-by: JoshLozensky <103777376+JoshLozensky@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Remove breaking changes between 7.x and 8.x Identity Model
Restore backward-compatible reader positioning in JsonSerializerPrimitives
Mar 2, 2026
JoshLozensky
reviewed
Mar 3, 2026
src/Microsoft.IdentityModel.Tokens/Json/JsonSerializerPrimitives.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: JoshLozensky <103777376+JoshLozensky@users.noreply.github.com>
JoshLozensky
approved these changes
Mar 5, 2026
Contributor
|
Can these changes be validated with one of our higher level libraries which leverage IdentityModel? Even running an E2E test locally to gut check that these behavioral un-breaking changes don't lead to unintended consequences? |
westin-m
approved these changes
Mar 9, 2026
Contributor
Did this offline |
kllysng
approved these changes
Mar 11, 2026
Contributor
kllysng
left a comment
There was a problem hiding this comment.
Thank you @JoshLozensky for your due diligence on validating the changes
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.
Developer Notes:
Addresses issue #2943
There is a different branch with a test project proving this PR's solution. Since that other branch includes this solution, to see the tests fail copy the test project into the dev branch. Those tests aren't part of this PR as they use intentionally mismatched versions of our libraries the compatibility of which is not actually a guarantee we make and the tests won't hold much value for future development.
AI Notes:
PR #2491 changed all
Read*methods inJsonSerializerPrimitivesto unconditionally advance theUtf8JsonReaderafter reading a value. This silently broke older compiled packages (e.g.Microsoft.IdentityModel.Protocols.OpenIdConnect<7.4.0) when paired with newerMicrosoft.IdentityModel.Tokens— every other JSON property was skipped because the reader was advanced twice per property.Restore backward-compatible reader positioning in JsonSerializerPrimitives
Description
Root cause: Old callers use
while(reader.Read())and manually advance to the value before callingRead*(read=false), expecting the reader to remain AT the value on return. The post-read advance added in #2491 causedwhile(reader.Read())to skip over the next token.Changes to
JsonSerializerPrimitives.cs:reader.Read()from:ReadBoolean,ReadLong,ReadString,ReadStringAsBool,ReadStringOrNumberAsString,ReadStringAsObject,ReadNumber, and scalar branches ofReadPropertyValueAsObject(True/False/Null/default)ReadJsonElementkeeps its advance — necessary so nestedEndObject/EndArraytokens don't prematurely terminate the outerwhile(true)loopReadString,ReadStringOrNumberAsString,ReadStringAsObject, andReadStringsis now conditional on thereadparameterReadStrings(IList),ReadStrings(ICollection),ReadStringsSkipNulls,ReadArrayOfObjects) now use explicitreader.Read()after each element; EndArray advance is conditional onreadReadArrayOfObjects: forStartObject/StartArrayelements,ReadPropertyValueAsObjectcallsReadJsonElementwhich already advances past the complex value — the explicitreader.Read()is now only applied for scalar types (Null, Number, String) whereReadPropertyValueAsObjectleaves the reader AT the valueCompatibility matrix:
read=false(old)read=true(new)while(reader.Read())Read()advances ✓Read()advances ✓while(true)+else if (!reader.Read())Read()advances ✓Read()advances ✓Tests added:
ReaderPositionAfterRead_BackwardCompatibility— asserts reader stays AT value afterRead*(read=false), and ATEndArrayafterReadStrings(read=false)ReaderPositionAfterRead_NewCallingPattern— asserts thewhile(true)loop correctly reads all properties withread=true💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.