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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
the host — D20 — this is a nudge, not a behaviour change).

### Fixed
- **The report-preview endpoint no longer leaks driver exceptions, and answers 400 where it meant to.**
It caught only `ConfigurationException`, so a bad filter value surfaced the raw `SqlException`/
`PostgresException`/`OracleException` — naming the host, port and database — as an unhandled 500
(with a full stack trace on a host running in Development). It now routes those through the same
scrubbed 502 its sibling schema endpoints use. Separately, a **code-first** report whose name a
config store cannot hold (`sales.daily`, say — code-first names are unrestricted) made the preview
runner throw while probing that store, another 500; such a name is now recognised as definitively
not-dynamic, giving the intended 400 explaining that typed reports have no filterable source.
- **Sectioned outputs are counted as outputs.** `CompiledReport.OutputCount` (and `OutputFormats`)
omitted them, so a report with one plain and one sectioned output looked single-output: the API's
sync mode — which supports single-output reports only — accepted it, the runner wrote both files,
Expand Down
9 changes: 6 additions & 3 deletions docs/STATUS-AND-BACKLOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,14 @@ rather than decided:
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
- ~~**API: `POST /reports/{name}/preview` is the one data-plane endpoint that doesn't scrub driver
exceptions.**~~ **FIXED** (routes through `SchemaProblem` like its siblings). Original description: 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.**
- **API: schedule/preview write paths reach a name-validating store without the guard.** The
**preview** half is **FIXED** (a name no config store can hold is now treated as not-dynamic, so the
endpoint returns its intended 400). `SetScheduleAsync`/`ClearScheduleAsync` still 500 — original
description:
`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
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 Down Expand Up @@ -262,6 +262,15 @@
{
return Results.BadRequest(new { error = ex.Message });
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
// Preview opens a live connection and runs SQL, so a bad filter value surfaces the raw
// driver exception — which names the host, port and database. Every sibling endpoint that
// touches a source routes failures through SchemaProblem (logged server-side, generic to
// the caller); this one used to let them escape as an unhandled 500, which on a host
// running in Development also renders the full stack trace.
return SchemaProblem(http, ex, name, "Could not run the report preview.");
}
}

private static IReadOnlyList<PreviewFilter> ParseFilters(IReadOnlyList<PreviewFilterRequest>? filters)
Expand Down Expand Up @@ -1053,7 +1062,7 @@

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

Check warning on line 1065 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 1065 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 1065 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 1065 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_Jrv24z7KtnRIWnm-d&open=AZ_Jrv24z7KtnRIWnm-d&pullRequest=258
return Results.Ok(ToCatalogResponse(catalog));
}
catch (Exception ex) when (ex is not OperationCanceledException)
Expand All @@ -1075,7 +1084,7 @@

try
{
TablePreview preview = await explorer!

Check warning on line 1087 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 1087 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 1087 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 1087 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_Jrv24z7KtnRIWnm-e&open=AZ_Jrv24z7KtnRIWnm-e&pullRequest=258
.PreviewTableAsync(definition!, schema ?? string.Empty, table, SchemaPreviewTop, cancellationToken)
.ConfigureAwait(false);
return Results.Ok(new TablePreviewResponse(preview.Columns, preview.Rows));
Expand Down
6 changes: 6 additions & 0 deletions src/NeoReports.Core/Preview/ReportPreviewRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ public static async Task<PreviewResult> PreviewAsync(
return await PreviewUnfilteredAsync(report, pageSize, execution, services, cancellationToken).ConfigureAwait(false);

IReportConfigStore? configStore = services.GetService<IReportConfigStore>();
// Check the name is one a config store could even hold before probing it: the file-backed
// store validates its argument and throws for anything outside the dynamic-name pattern, and
// a code-first report is under no such restriction (a name like "sales.daily" is legal). That
// threw out of the endpoint as an unhandled ArgumentException — a 500 — instead of the clear
// "typed report" message below. A name the store cannot hold is definitively not dynamic.
bool isDynamic = configStore is not null
&& DynamicReportName.IsValid(report.Name)
&& await configStore.ExistsAsync(report.Name, cancellationToken).ConfigureAwait(false);

if (!isDynamic)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,50 @@ public async Task PageSize_is_capped_to_the_request()
body.GetProperty("hasMore").GetBoolean().ShouldBeTrue();
}

[Fact]
public async Task A_driver_failure_during_preview_is_scrubbed_and_not_returned_to_the_caller()
{
// Preview opens a live connection and runs SQL, so a driver exception is reachable from a
// request. Its message routinely echoes the connection target; it must never reach the caller.
using Microsoft.Extensions.Hosting.IHost host =
await StartWithDynamicReportAsync("fake-sql", new ThrowingFilterTranslator("fake-sql"));
var client = host.GetTestClient();

var response = await PostJsonAsync(client, "/api/reports/dyn/preview",
"""{ "filters": [ { "column": "Id", "operator": "Equals", "value": 1 } ] }""");

response.StatusCode.ShouldBe(HttpStatusCode.BadGateway);
var body = await response.Content.ReadAsStringAsync();
body.ShouldNotContain("db.internal");
body.ShouldNotContain("payroll");
body.ShouldNotContain("Login failed");
}

[Fact]
public async Task Filters_on_a_typed_report_whose_name_the_config_store_cannot_hold_return_400()
{
// A code-first report's name is only checked for non-blank, so "sales.daily" is legal — but a
// config store validates its argument against the dynamic-name pattern and throws for it. The
// preview runner probed the store before deciding the report was typed, so that ArgumentException
// escaped as a 500 instead of the clear "typed report" 400 this endpoint means to return.
// A real (file-backed) config store must be registered, otherwise the runner short-circuits on
// `configStore is null` and never reaches the name check this test is about.
using var host = await TestApp.StartAsync(services =>
{
services.AddDynamicReports(o => o.Directory = _configDir);
services.AddReport<Sale>("sales.daily", b => b
.From(new InMemorySource(rows: 3, pageSize: 10))
.Column(v => v.Id, "Id")
.To(NeoReports.Formats.Csv.Format.Csv()));
});
var client = host.GetTestClient();

var response = await PostJsonAsync(client, "/api/reports/sales.daily/preview",
"""{ "filters": [ { "column": "Id", "operator": "Equals", "value": 1 } ] }""");

response.StatusCode.ShouldBe(HttpStatusCode.BadRequest);
}

[Fact]
public async Task Filters_on_a_typed_report_return_400()
{
Expand Down Expand Up @@ -329,3 +373,19 @@ public bool TryTranslate(
return true;
}
}

/// <summary>Translator that fails the way a real provider does — an exception whose message names the
/// connection target — so the endpoint's scrubbing can be asserted.</summary>
public sealed class ThrowingFilterTranslator : IFilterTranslator
{
public ThrowingFilterTranslator(string type) => Type = type;

public string Type { get; }

public bool TryTranslate(
IReadOnlyDictionary<string, object?> properties, IReadOnlyList<PreviewFilter> filters, ReportSchema schema,
out IReadOnlyDictionary<string, object?> propertyOverrides,
out IReadOnlyDictionary<string, object?> parameters) =>
throw new InvalidOperationException(
"Login failed for user 'sa'. Server=db.internal:1433;Database=payroll");
}