v5.0.0: System.Text.Json default, remove Newtonsoft, trusted publishing#17
Merged
Conversation
…nsoft SimpleHttpDefaultJsonSerializer now derives from SimpleHttpSystemTextJsonSerializer (System.Text.Json), so the client's default serialization moves off Newtonsoft.Json, which is dropped as a dependency. Breaking: System.Text.Json is stricter — types using a non-public parameterless constructor, or fields whose JSON shape varies (string vs object), deserialize differently. README documents the migration. Convert the test suite off Newtonsoft (JToken/JObject -> System.Text.Json.Nodes, JsonConvert -> JsonSerializer). The postman-echo "data" field is an empty string on form posts but an object on JSON posts; typed as JsonNode so STJ doesn't reject it. Switch the release workflow to NuGet trusted publishing (OIDC) instead of a long-lived API key: add id-token: write, exchange the token via NuGet/login@v1 right before push. Requires a NUGET_USER secret and a trusted-publishing policy on nuget.org. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… gaps The earlier fix typed PostmanEchoResponse.Data as JsonNode and read it via a dynamic indexer, which defeated the test's purpose: verifying the serializer maps JSON onto strongly-typed properties. Restore Data to the typed Args? and instead attach an EmptyStringTolerantConverter that maps postman-echo's empty-string "data" (returned for form posts) to null, keeping the typed assertions intact. Fill noticeable serializer gaps surfaced by the v5 switch: - nested object + collection + enum round-trip - enums serialize as numbers (Newtonsoft parity) - deserializing into a type with only a non-public constructor throws (the documented System.Text.Json strictness difference) - the default serializer is System.Text.Json-backed Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Soften the default System.Text.Json options to ease the v5 migration without masking genuine type errors: - NumberHandling = AllowReadingFromString (quoted numbers bind to numeric props) - AllowTrailingCommas = true - ReadCommentHandling = Skip Deliberately not softened: non-public constructors and wrong-shape coercion stay strict (documented, with a custom-converter escape hatch in the README). Add tests for the new leniencies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover the previously-untested provider stack: the factory returns the right provider per target framework, GetClient returns a configured client cached across calls (no factory) or delegates to the IHttpClientFactory (with one), and Dispose disposes the owned client and is idempotent. Add InternalsVisibleTo for the test assembly so the internal providers/factory can be exercised directly. The net10.0 run exercises PooledHttpClientProvider; net48 exercises RotatingHttpClientProvider. Timer-driven rotation (5-min interval) is left untested as it has no injectable seam. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mako88
commented
May 30, 2026
Mako88
left a comment
Owner
Author
There was a problem hiding this comment.
I attempted to approve this, but GitHub rejected it because the PR author and reviewer are the same account. For the record, after reviewing the diff I did not find any blocking issues.
Highlights from the review:
- The v5 breaking change is clearly documented and appropriately scoped for a major release.
SimpleHttpDefaultJsonSerializernow delegates to the System.Text.Json implementation while preserving the public type, which minimizes migration friction.- The migration notes correctly call out the biggest Newtonsoft → STJ behavior differences.
- Test coverage was expanded around the new serializer behavior (quoted numbers, comments/trailing commas, enum serialization, non-public constructors, etc.).
- The trusted publishing workflow change looks sound and removes reliance on a long-lived API key.
- CI is green on the current head commit.
Only minor observation: InternalsVisibleTo was added to test internal provider selection. That's reasonable, though it does create some coupling between tests and implementation details. Not a blocker.
If I were reviewing this as a separate GitHub user, this would be an approval from me.
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.
Summary
Major release. Moves the default JSON serializer from
Newtonsoft.JsontoSystem.Text.Json, removes theNewtonsoft.Jsondependency entirely, and switches the release workflow to NuGet trusted publishing (OIDC) so we no longer rely on a long-lived API key.This is the breaking follow-up to 4.2.0, which added
SimpleHttpSystemTextJsonSerializeras an opt-in.Library changes (breaking)
SimpleHttpDefaultJsonSerializernow derives fromSimpleHttpSystemTextJsonSerializer(System.Text.Json). The client's default serialization is now STJ.SimpleHttpSystemTextJsonSerializeris retained for callers who reference it explicitly.Newtonsoft.JsonPackageReference removed;System.Text.Jsonretained.release:majorlabel + latest tag).Migration notes
System.Text.Jsonis stricter thanNewtonsoft.Json. Watch for:[JsonConstructor].ISimpleHttpSerializerwith a Newtonsoft serializer and set it on the client.Tests
JToken/JObject→System.Text.Json.Nodes,JsonConvert→JsonSerializer.datafield is an empty string on form posts but an object on JSON posts — re-typed the test model field asJsonNode?so STJ accepts both (this surfaced the strictness difference above).Release workflow → trusted publishing
id-token: writepermission.NuGet/login@v1step that exchanges the OIDC token for a short-lived API key right before push; push now uses that key instead ofsecrets.NUGET_API_KEY.Mako88, RepositorySimpleHttpClient, Workflow Filerelease.yml, Environment empty.NUGET_USER= your nuget.org username (profile name, not email).NUGET_API_KEYsecret can be deleted afterward.🤖 Generated with Claude Code