Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
the host — D20 — this is a nudge, not a behaviour change).

### Fixed
- **Run-time parameters now work on every job backend.** The run request types its parameter values
as `object?`, so `System.Text.Json` handed each one to the pipeline as a `JsonElement` — which no
ADO provider can bind (*"No mapping exists from object type System.Text.Json.JsonElement"*). Every
parameterized report therefore failed on the **sync** and **in-memory job** paths, while the
Hangfire path happened to work because it round-trips parameters through `JobParameters`. Values are
now converted to CLR primitives at the request boundary, so all three backends behave identically.
That equivalence also required fixing `JobParameters`, which boxed **every** whole number as a
`double`: a conditional returning `long` alongside `double` widens implicitly unless the `long` is
cast to `object` first. A `bigint` id past 2^53 was therefore bound as a float and compared wrong on
the Hangfire path. A number too large for `double` is now kept as its original JSON token rather
than becoming `±Infinity`, which is not representable in JSON and made re-serializing it throw.
- **A run-time parameter can no longer take over the engine's keyset cursor.** Execution parameters
were bound before the engine's own `@cursor`/`@pageSize`, and the binder keeps whichever name was
bound first — so a report run supplying a parameter called `cursor` pinned it for every page. The
Expand Down
93 changes: 93 additions & 0 deletions docs/STATUS-AND-BACKLOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,99 @@ need a decision.
There is no `Partial` job status. Worth confirming this is still the intent, since silent partial
data reads as a green job.

### 6. Source-pagination and API findings (2026-08-03) — deferred, each needs a decision
Two more hunts (the nine HTTP-family source packages; the AspNetCore endpoint layer) produced these.
The unambiguous ones shipped — the engine's reserved `cursor`/`pageSize` bind names, and run-time
parameters arriving as `JsonElement`. What is left changes behaviour or semantics, so it is recorded
rather than decided:

- **The page loop has no safety net.** `ReportRunner`'s loop is `while(true)` driven purely by
`HasMore`: no page cap, no "the cursor did not change" guard, no "zero rows but still more" guard.
Every item below is only as dangerous as that. A generic guard here would bound *all* of them at
once and is probably the highest-leverage single change — but it changes termination semantics for
every source, so it needs a call.
- **`records.Count == pageSize` ends a run early when the server caps the page.** OData's `Skip`
strategy (`ODataBatchSource`) and the HTTP source's `Page`/`Offset` strategies infer "more data"
from a full page. Services that clamp `$top`/`limit` below the engine's 1000 default (Dynamics, SAP
Gateway, Business Central; many REST APIs silently reduce an over-max `limit`) return a short first
page → the run stops there and reports **`Completed`** with partial data. `Skip` is opt-in and
`NextLink` is the default, which limits blast radius. A fix means either honouring `@odata.nextLink`
in `Skip` mode too, or paging until a page returns zero rows — both change termination semantics.
- **Elasticsearch treats a partially-failed search as a short page.** ES returns **HTTP 200** with
`timed_out: true` / `_shards.failed > 0` and fewer hits; neither field is inspected, so the report
silently ends early as `Completed`. GraphQL already fails loudly on 200-with-`errors`; the ES
equivalent would be consistent, but it turns today's silent success into a hard failure.
- **HTTP `Cursor` strategy has no non-advancing-cursor guard.** If an API echoes the requested cursor
on the last page (Facebook Graph's `paging.cursors.after`, among others), `hasMore` stays true with
an identical token → the same request repeats **forever**. GraphQL (D63) and Elasticsearch both
guard this explicitly; the generic HTTP source — the most exposed, since the cursor path is
author-configured — does not.
- **`Link`-header parsing breaks on a comma inside the URL.** `HttpBatchSource` splits the header on
`,` unconditionally, but RFC 8288 permits commas in the target URI and in quoted parameters. A base
URL like `?fields=id,name` echoed into the next-page link makes `rel="next"` unparseable → paging
stops silently after page 1.
- **A relative next-page URL throws.** Both `HttpBatchSource` and `ODataBatchSource` call
`new Uri(nextUrl)` (absolute-only) on a server-supplied link; RFC 8288 and OData both permit a
relative one. Fails loudly (`UriFormatException`, opaque message) rather than silently. Salesforce
is the only package that resolves this correctly.
- **`HttpHealthProbe.CombineUrl` still has the relative-`Uri` bug — the 5th sighting of this class.**
`new Uri(baseUri, path)` drops the base's last path segment when it has no trailing slash, and a
leading `/` resets to the host root. Elasticsearch (D64), HubSpot, Airtable and Salesforce each
independently rewrote away from it — with comments naming it — but the shared helper still does it,
and `HttpSourceHealthCheck` + `ODataSourceHealthCheck` still call it: a health check can probe the
wrong URL and report a healthy source unhealthy (or vice-versa). **Fixing the shared helper by
concatenation, as the four leaf packages already do, is the cheapest win in this list.**
- **Google Sheets: three data-fidelity bugs.** (a) header cells that aren't JSON strings are dropped,
so a year-numbered column (`2024`) never binds and every row's value for it is null/zero — the
requests use `UNFORMATTED_VALUE`, which returns numeric headers as JSON numbers, while the data path
already decodes all kinds; (b) a header range that comes back without `values` caches an **empty**
index, so a misconfigured `headerRow` produces N rows of all-nulls reported as success instead of
failing loudly; (c) an interior blank row is returned as `[]` and materialized as a phantom
all-default row. (a) is the clearest and most contained.
- **HubSpot and Airtable default to a page size their API rejects.** Both send the engine's 1000
default as `limit`/`pageSize`, but both providers cap at 100 (recorded in `DECISIONS.md`), so a
source built with defaults fails its very first request until the author calls `.PageSize(100)`.
Loud, but the default configuration is non-functional. Clamping vs. failing with a clear message is
a product call.
- **API: `POST /reports/{name}/preview` is the one data-plane endpoint that doesn't scrub driver
exceptions.** It catches only `ConfigurationException`, so a bad filter value surfaces the raw
`SqlException`/`PostgresException` (host, port, database) as a 500 — its siblings all route through
`SchemaProblem`. Should be a 400 (bad filter) or the scrubbed 502 the others return.
- **API: schedule/preview write paths reach a name-validating store without the guard.**
`SetScheduleAsync`, `ClearScheduleAsync` and `ReportPreviewRunner`'s config-store probe pass the
report name straight through, so a legitimate **code-first** report whose name is outside
`^[a-zA-Z][a-zA-Z0-9_-]{0,99}$` (e.g. `sales.daily`) gets a **500** from `ArgumentException`. The
read paths already guard, which shows it is an oversight.
- **API: `GET /jobs/{id}` returns raw destination-exception text** (server paths, S3 bucket + key, AWS
error strings). The read- and write-failure paths in the same method scrub; the **upload** path does
not — and the sync endpoint deliberately suppresses the very same string, so one route hides what
the other returns verbatim.
- **API: sync mode's single-output guard ignores sectioned outputs.** `OutputCount` counts only
`Outputs`, so a report with one plain and one sectioned output passes the guard, the runner writes
two artifacts, and the caller silently receives **one** — which one decided by directory-enumeration
order. The same undercount makes `GET /reports` under-report a sectioned report's formats.
- **API: `Location`/`Content-Location` headers hardcode `/api`**, ignoring `MapNeoReports`'s
configurable prefix — under `MapNeoReports("/v2")` the 202's `Location` is a 404 for any client that
follows it.
- **Array/object run parameters still diverge by backend.** Complex parameter values are documented
out of scope for v1, but nothing rejects them: sync/in-memory hand the source a `JsonElement` (the
very thing an ADO provider can't bind) while Hangfire hands it the raw JSON text. Either reject them
at the boundary with a 400, or agree one representation — the current silence produces a driver
error at read time.
- **`POST/PUT /sources` property bags are not normalized.** `SourceRequest.Properties` is the same
caller-supplied `object?` bag as run parameters. `FileSourceRegistryStore` launders it on write and
read, but `InMemorySourceRegistryStore` stores it as-is — so with `AddInMemorySourceRegistry()` a
source created over HTTP fails later with *"requires a non-empty 'connectionString' property"*
because the value is a `JsonElement`, not a `string`. Pre-existing; the same one-line normalization
the run endpoint now does would close it.

Verified correct in the same pass (worth not re-auditing): artifact download path handling (no
caller-supplied filename reaches disk; no zip-slip; `0600` temp), job-id handling, SQL-injection
surface via run parameters, preview filter validation, list-endpoint pagination clamping, cursor
round-tripping in all nine source packages (`OpaqueCursor` is Base64-JSON, lossless), GraphQL /
HubSpot / Airtable / Salesforce termination logic, and the Elasticsearch "capture the last sort"
loop.

---

## Where the fuller context lives
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@
return group;
}

private static async Task<IResult> RunReportAsync(

Check warning on line 149 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Method has 9 parameters, which is greater than the 7 authorized.

Check warning on line 149 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Method has 9 parameters, which is greater than the 7 authorized.

Check warning on line 149 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Method has 9 parameters, which is greater than the 7 authorized.

Check warning on line 149 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Method has 9 parameters, which is greater than the 7 authorized.
string name,
string? mode,
RunReportRequest? body,
Expand All @@ -172,7 +172,7 @@
});
}

var parameters = body?.Parameters;
IReadOnlyDictionary<string, object?>? parameters = NormalizeParameters(body?.Parameters);

if (string.Equals(mode, "sync", StringComparison.OrdinalIgnoreCase))
{
Expand Down Expand Up @@ -1053,7 +1053,7 @@

try
{
SchemaCatalog catalog = await explorer!.GetCatalogAsync(definition!, cancellationToken).ConfigureAwait(false);

Check warning on line 1056 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1056 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1056 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1056 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

See more on https://sonarcloud.io/project/issues?id=lugarini_NeoReports&issues=AZ_JdLFI-CkDYWnchzVD&open=AZ_JdLFI-CkDYWnchzVD&pullRequest=254
return Results.Ok(ToCatalogResponse(catalog));
}
catch (Exception ex) when (ex is not OperationCanceledException)
Expand All @@ -1075,7 +1075,7 @@

try
{
TablePreview preview = await explorer!

Check warning on line 1078 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1078 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1078 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View workflow job for this annotation

GitHub Actions / SonarCloud

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

Check warning on line 1078 in src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this null-forgiving operator; the compiler already knows this expression is not null here.

See more on https://sonarcloud.io/project/issues?id=lugarini_NeoReports&issues=AZ_JdLFI-CkDYWnchzVE&open=AZ_JdLFI-CkDYWnchzVE&pullRequest=254
.PreviewTableAsync(definition!, schema ?? string.Empty, table, SchemaPreviewTop, cancellationToken)
.ConfigureAwait(false);
return Results.Ok(new TablePreviewResponse(preview.Columns, preview.Rows));
Expand Down Expand Up @@ -1186,6 +1186,28 @@
statusCode: StatusCodes.Status502BadGateway);
}

// The run request types its parameter values as `object?`, so System.Text.Json materializes 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"), 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 here
// instead, at the one boundary where they enter, so every backend behaves the same. Round-tripping
// through PrimitiveObjectConverter reuses the repo's single definition of "JSON value → CLR
// primitive" (string/long/double/bool/ISO-8601 DateTime, nested objects left as JsonElement)
// rather than restating it; parameter bags are a handful of scalars, so the cost is irrelevant.
private static readonly JsonSerializerOptions ParameterJson =
new(JsonSerializerDefaults.Web) { Converters = { new PrimitiveObjectConverter() } };

private static IReadOnlyDictionary<string, object?>? NormalizeParameters(
IReadOnlyDictionary<string, object?>? parameters)
{
if (parameters is null || parameters.Count == 0)
return parameters;

string json = JsonSerializer.Serialize(parameters, ParameterJson);
return JsonSerializer.Deserialize<Dictionary<string, object?>>(json, ParameterJson);
}

// A failed health check's raw Error is the underlying driver/IO exception message, which can echo
// connection-string fragments (host, port, database, username) or an on-disk path. Log it
// server-side and hand the caller only a generic, secret-free reason — the same stance
Expand Down
7 changes: 6 additions & 1 deletion src/Jobs/NeoReports.Jobs/JobParameters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ public static string Serialize(IReadOnlyDictionary<string, object?>? parameters)
JsonValueKind.True => true,
JsonValueKind.False => false,
JsonValueKind.String => ConvertString(value.GetString()),
JsonValueKind.Number => value.TryGetInt64(out var l) ? l : value.GetDouble(),
// The (object) cast is load-bearing — without it the conditional's static type unifies long
// and double (long widens implicitly), silently re-boxing every whole number as a double even
// when TryGetInt64 succeeded. That made this path disagree with the sync/in-memory one, which
// yields long, and it loses precision past 2^53: a bigint id bound as a float compares wrong.
// Same trap, same fix as PrimitiveObjectConverter.
JsonValueKind.Number => value.TryGetInt64(out var l) ? (object)l : value.GetDouble(),
_ => value.GetRawText(),
};

Expand Down
25 changes: 19 additions & 6 deletions src/NeoReports.Core/Configuration/PrimitiveObjectConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ public sealed class PrimitiveObjectConverter : JsonConverter<object?>
JsonTokenType.True => true,
JsonTokenType.False => false,
JsonTokenType.String => ConvertString(reader.GetString()),
// The (object) cast on the long arm is load-bearing: without it, the switch expression's
// static result type unifies long and double to double (long -> double is an implicit
// widening conversion), silently re-boxing every whole-number property as a double even
// when TryGetInt64 succeeds — so `raw is long` never once matched anywhere this value
// was consumed, for any whole number, ever.
JsonTokenType.Number => reader.TryGetInt64(out var l) ? (object)l : reader.GetDouble(),
JsonTokenType.Number => ReadNumber(ref reader),
_ => JsonDocument.ParseValue(ref reader).RootElement.Clone(),
};

Expand Down Expand Up @@ -61,6 +56,24 @@ public override void Write(Utf8JsonWriter writer, object? value, JsonSerializerO
}
}

private static object ReadNumber(ref Utf8JsonReader reader)
{
// The (object) cast is load-bearing: returned from a conditional alongside a double, a long
// would widen implicitly and every whole number would come back boxed as a double — so
// `raw is long` never matched anywhere this value was consumed, for any whole number, ever.
if (reader.TryGetInt64(out var l))
return (object)l;

// A magnitude past double's range parses to ±Infinity, which is not representable in JSON:
// re-serializing the value then throws ("positive and negative infinity cannot be written as
// valid JSON"), turning a readable document into an unwritable one. Keep the token itself in
// that case — the same shape nested values already use — so it round-trips verbatim.
if (reader.TryGetDouble(out var d) && double.IsFinite(d))
return d;

return JsonDocument.ParseValue(ref reader).RootElement.Clone();
}

private static object? ConvertString(string? text)
{
if (text is null)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
using System.Net.Http.Json;
using System.Text.Json;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using NeoReports.Abstractions;
using NeoReports.Core.Building;
using NeoReports.Core.DependencyInjection;
using Shouldly;
using Xunit;
using static NeoReports.Core.Building.ReportColumns;
using static NeoReports.Formats.Csv.Format;

namespace NeoReports.AspNetCore.IntegrationTests;

/// <summary>
/// Run-time parameters posted to the run endpoint must reach the source as CLR values. The request
/// body types them as <c>object?</c>, and <c>System.Text.Json</c> materializes an untyped value as a
/// <see cref="JsonElement"/> — which no ADO provider can bind (`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 (it
/// round-trips parameters through <c>JobParameters</c>, which converts them).
/// </summary>
public class RunParameterBindingTests
{
private static readonly JsonSerializerOptions Json = new(JsonSerializerDefaults.Web);

/// <summary>Records the parameters the pipeline handed the source on the first read.</summary>
private sealed class CapturingSource : IBatchSource<Sale>
{
public IReadOnlyDictionary<string, object?>? Captured { get; private set; }

public ReportSchema Schema { get; } = new(new[] { new ReportColumn("Id", ColumnType.Integer) });

public Task<BatchResult<Sale>> ReadBatchAsync(BatchContext context, CancellationToken cancellationToken)
{
Captured ??= context.Execution.Parameters;
return Task.FromResult(BatchResult<Sale>.Empty);
}
}

[Fact]
public async Task Run_time_parameters_reach_the_source_as_clr_values_not_json_elements()
{
var source = new CapturingSource();
using var host = await TestApp.StartAsync(services =>
services.AddReport<Sale>("params", b => b
.From(source)
.Column(v => v.Id, "ID")
.To(Csv())));
var client = host.GetTestClient();

var response = await client.PostAsJsonAsync(
"/api/reports/params/run?mode=sync",
new { parameters = new Dictionary<string, object?> { ["tenant"] = "acme", ["count"] = 42 } },
Json);

response.IsSuccessStatusCode.ShouldBeTrue();
source.Captured.ShouldNotBeNull();
// The exact CLR shapes an ADO provider can bind — a JsonElement here would throw at bind time.
source.Captured!["tenant"].ShouldBeOfType<string>().ShouldBe("acme");
source.Captured!["count"].ShouldBeOfType<long>().ShouldBe(42L);
}
}
8 changes: 6 additions & 2 deletions tests/NeoReports.Jobs.UnitTests/JobParametersTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ public void Round_trips_primitive_values()
var back = JobParameters.Deserialize(json);

back["name"].ShouldBe("sales");
back["count"].ShouldBe(42L);
back["ratio"].ShouldBe(1.5);
// ShouldBe alone can't police this: Shouldly compares numerics by value, so a double 42.0
// satisfies ShouldBe(42L) and the whole-number-boxed-as-double bug slipped through. Assert
// the runtime type — a provider binds an integer column by the CLR type it is handed, and a
// double past 2^53 silently loses precision.
back["count"].ShouldBeOfType<long>().ShouldBe(42L);
back["ratio"].ShouldBeOfType<double>().ShouldBe(1.5);
back["active"].ShouldBe(true);
back["missing"].ShouldBeNull();
}
Expand Down