feat: add refresh options for on-demand dataset refresh - #21
Conversation
`RefreshDatasetAsync` could only trigger a dataset refresh using the
dataset's configured settings. Every other Spice SDK (Go, Python, Rust,
Java, TypeScript) accepts per-refresh overrides, so .NET callers had no
way to run a filtered or append-mode refresh without editing the
Spicepod.
Add a `RefreshOptions` type carrying the three overrides the runtime
accepts on `POST /v1/datasets/{name}/acceleration/refresh`:
- `RefreshSql` -> `refresh_sql`
- `RefreshMode` -> `refresh_mode` (`Full` / `Append`)
- `MaxJitter` -> `refresh_jitter_max` (e.g. "10s", "1500ms")
Unset options are omitted from the body so the runtime falls back to the
dataset configuration, matching the other SDKs.
The existing single-argument `RefreshDatasetAsync(string)` overload is
unchanged, so this is source and binary compatible. It now posts `{}`
with `application/json` instead of an empty body, which is what the
other SDKs send and what the runtime documents.
netstandard2.0 gains an explicit `System.Text.Json` reference; it is
in-box for .NET 8.0+.
CI note: the red is pre-existing, not from this PRThe A Per-job comparison on this PR — every job that got as far as running tests reports exactly the same 33 unique failures, and zero failures among the new tests:
All 22 tests added here pass on every platform, including Windows: Locally, where the Cloud secret is absent and those 33 skip instead of failing, the suite is fully green on all three frameworks: (Baseline on trunk was 115 passed / 33 skipped, so this is +22 passing and no change in skips.) The Cloud connectivity failure looks worth a separate look — it is unrelated to this change, which only touches the HTTP refresh path. |
There was a problem hiding this comment.
Pull request overview
Adds first-class support in the .NET SDK for per-refresh override settings when triggering an on-demand dataset acceleration refresh, aligning the API with other Spice SDKs and the runtime wire format.
Changes:
- Introduces
Spice.Datasets.RefreshOptions(+RefreshMode) to serialize refresh overrides (refresh_sql,refresh_mode,refresh_jitter_max) and omit unset fields. - Adds
RefreshDatasetAsync(string, RefreshOptions?)overloads throughSpiceClient→SpiceHttpClient, and changes the no-options path to POST{}withapplication/json. - Adds offline unit/integration-style tests validating serialization and the actual HTTP request body/headers; updates README docs and adds a netstandard2.0-only
System.Text.Jsonreference.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SpiceTest/RefreshOptionsTest.cs | Unit tests for RefreshOptions JSON shape, omission behavior, duration formatting, and validation. |
| SpiceTest/RefreshDatasetRequestTest.cs | Loopback HTTP test asserting the refresh request method/path/content-type/body. |
| Spice/src/SpiceClient.cs | Adds public overload RefreshDatasetAsync(string, RefreshOptions?) and routes to HTTP client. |
| Spice/src/Http/SpiceHttpClient.cs | Implements refresh POST with {}/overrides JSON body and application/json content type. |
| Spice/src/Http/ISpiceHttpClient.cs | Extends the public HTTP client interface with the new overload. |
| Spice/src/Datasets/RefreshOptions.cs | New options + enum type; JSON serialization and duration formatting logic. |
| Spice/Spice.csproj | Adds conditional System.Text.Json package reference for netstandard2.0. |
| README.md | Documents refresh overrides and usage examples; adds an options table. |
Comments suppressed due to low confidence (2)
Spice/src/Datasets/RefreshOptions.cs:99
- On the netstandard2.0 path, ThrowHelper.ThrowIfNullOrWhiteSpace throws ArgumentException even when refreshSql is null, which makes exception behavior differ across target frameworks (NET8 throws ArgumentNullException for null). Consider adding an explicit null check in the netstandard2.0 branch so callers see consistent exceptions across TFMs.
#if NET8_0_OR_GREATER
ArgumentException.ThrowIfNullOrWhiteSpace(refreshSql);
#else
ThrowHelper.ThrowIfNullOrWhiteSpace(refreshSql, nameof(refreshSql));
#endif
Spice/src/Datasets/RefreshOptions.cs:173
- FormatDuration uses TimeSpan.TotalMilliseconds (double) and casts to long, which truncates sub-millisecond values and can introduce rounding issues for large durations. Prefer integer math based on ticks to avoid double conversion/truncation surprises.
var totalMilliseconds = (long)value.TotalMilliseconds;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// Refreshes a dataset in the Spice runtime, overriding the dataset's configured | ||
| /// refresh settings for this refresh only. | ||
| /// </summary> | ||
| /// <param name="datasetName">The name of the dataset to refresh</param> | ||
| /// <param name="options">Overrides for this refresh, or null to use the dataset configuration</param> | ||
| /// <returns>A task representing the asynchronous operation</returns> | ||
| /// <exception cref="System.ArgumentException">Thrown when datasetName is null or empty</exception> | ||
| /// <exception cref="System.Net.Http.HttpRequestException">Thrown when the HTTP request fails</exception> | ||
| Task RefreshDatasetAsync(string datasetName, RefreshOptions? options); |
| /// </summary> | ||
| /// <param name="refreshSql">The refresh SQL statement.</param> | ||
| /// <returns>The current instance of <see cref="RefreshOptions"/> for method chaining.</returns> | ||
| /// <exception cref="System.ArgumentException">Thrown when refreshSql is null or whitespace.</exception> |
| if (MaxJitter.HasValue) | ||
| { | ||
| body["refresh_jitter_max"] = FormatDuration(MaxJitter.Value); | ||
| } |
| | Option | Type | Description | | ||
| | --- | --- | --- | | ||
| | `RefreshSql` | `string` | The SQL statement used for this refresh. Defaults to the dataset's `refresh_sql`. | | ||
| | `RefreshMode` | `RefreshMode` | `Full` replaces the accelerated data; `Append` adds newly returned rows. Defaults to the dataset's `refresh_mode`. | | ||
| | `MaxJitter` | `TimeSpan` | Maximum jitter added before the refresh starts. Defaults to the dataset's `refresh_jitter_max`. | |
What
Adds
RefreshOptionsso callers can override a dataset's configured refresh settings for a single on-demand refresh.Why
RefreshDatasetAsynccould only trigger a refresh using whatever was configured in the Spicepod. The runtime has always accepted per-refresh overrides onPOST /v1/datasets/{name}/acceleration/refresh, and every other Spice SDK already exposes them:DatasetRefreshRequestRefreshOptsDatasetRefreshRequestRefreshOptionsRefreshAccelerationOptionsThis was the one capability present in all five sibling SDKs and missing from .NET. Without it there is no way to run a filtered or append-mode refresh from .NET short of editing the Spicepod and redeploying.
Wire format
Maps onto the runtime's
RefreshOverrides, matching the other SDKs:RefreshSqlrefresh_sqlRefreshMode(Full/Append)refresh_mode("full"/"append")MaxJitter(TimeSpan)refresh_jitter_max("10s","1500ms")Unset options are omitted from the body so the runtime falls back to the dataset configuration.
MaxJitteris exposed as aTimeSpanrather than a duration string so the type system does the validation; it is formatted to the runtime's duration syntax on the way out.Compatibility
RefreshDatasetAsync(string)overload is kept with an unchanged signature — source and binary compatible.{}withContent-Type: application/jsoninstead of an empty body with no content type. This is what the other SDKs send; the runtime treats both as "no overrides".netstandard2.0gains an explicitSystem.Text.Jsonpackage reference. It is in-box for .NET 8.0+, so the reference is conditioned on the netstandard2.0 target only.Testing
22 new tests, all offline — no runtime or API key needed:
RefreshOptionsTest— JSON field names, omission of unset options, duration formatting, enum wire values, JSON escaping of user-supplied refresh SQL, and argument validation.RefreshDatasetRequestTest— drives the real client against a loopbackHttpListenerand asserts the method, path, content type and body actually put on the wire.Baseline on
trunkwas 115 passed / 33 skipped. The 33 skips are the pre-existing tests gated onSCP_SPICEAI_TPCH_API_KEY.Follow-ups (not in this PR)
client.Query(sql, Dictionary<string, object>)overload with named:paramplaceholders in both the self-hosted and Cloud sections. No such overload has ever existed — parameterized queries areQueryWithParamswith positional$1placeholders. Copy-pasting the documented example does not compile. Worth either adding the overload or correcting the docs.CancellationToken, and no gRPC deadline is set, so a query cannot be given a timeout or cancelled. Go and Rust both support cancellation; Java and TypeScript do not, so this is a smaller gap than the one fixed here, but it is a real one for ASP.NET Core callers wanting to propagate a request-abort token.