fix(api): make run-time parameters usable on every job backend - #254
Merged
Conversation
The run request types its parameter values as `object?`, so System.Text.Json
materialized each one as a JsonElement. Nothing downstream can use that — an ADO
provider rejects it outright ("No mapping exists from object type
System.Text.Json.JsonElement to a known managed provider native type") — so every
parameterized report failed on the sync and in-memory-job paths, while the
Hangfire path happened to work because it round-trips parameters through
JobParameters, which converts them. Convert at the one boundary where they enter,
reusing PrimitiveObjectConverter rather than restating "JSON value -> CLR
primitive".
That equivalence also required two fixes in the conversion itself:
- JobParameters boxed EVERY whole number as a double. A conditional returning a
long alongside a double widens the long implicitly unless it is cast to object
first — the exact trap PrimitiveObjectConverter documents. A bigint id past
2^53 was bound as a float and compared wrong on the Hangfire path. The existing
test missed it because Shouldly compares numerics by value, so 42.0 satisfies
ShouldBe(42L); it now asserts the runtime type.
- A number too large for double parsed to +/-Infinity, which is not representable
in JSON, so re-serializing it threw and would have turned a previously working
202 into a 500 with an orphaned Queued job. Such a value now keeps its original
JSON token.
Verified: the new endpoint test failed before the fix (the source received a
JsonElement) and passes after; the strengthened JobParameters test fails without
the object cast.
|
|
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
1 participant
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.
What
RunReportRequest.Parameterstypes its values asobject?, soSystem.Text.Jsonmaterializes each one as aJsonElement. Nothing downstream can use that — an ADO provider rejects it outright:So every parameterized report failed on the sync and in-memory job paths. The Hangfire path happened to work, because it round-trips parameters through
JobParameters, which converts them — meaning the same request succeeded or failed depending on the job backend.Confirmed empirically before fixing: a new endpoint test showed the source receiving
System.Text.Json.JsonElementwhere astringwas expected.Values are now converted at the one boundary where they enter, reusing
PrimitiveObjectConverterrather than restating "JSON value → CLR primitive".Two fixes the equivalence exposed
JobParametersboxed every whole number as adouble. A conditional returning alongalongside adoublewidens thelongimplicitly unless it is cast toobjectfirst — the exact trapPrimitiveObjectConverteralready documents and guards. Abigintid past 2^53 was bound as a float and compared wrong on the Hangfire path. The existing test missed it because Shouldly compares numerics by value, so42.0satisfiesShouldBe(42L); it now asserts the runtime type.doubleparsed to±Infinity, which is not representable in JSON, so re-serializing it threw. Without this, the change would have turned a previously working202into a500plus an orphanedQueuedjob. Such a value now keeps its original JSON token.Verification
RunParameterBindingTests— verified to fail before the fix (source got aJsonElement) and pass after.JobParametersTests— verified to fail without theobjectcast (should be of type … System.Double).Found by a bug hunt over the API layer. The remaining findings from that hunt and the HTTP-source hunt — including two that also concern parameters (complex values still diverge by backend;
POST /sourcesproperty bags are unnormalized) — are recorded indocs/STATUS-AND-BACKLOG.md§6, since each needs a decision.