From d48b561d363240fffc6538c5269a4ca25a2196c5 Mon Sep 17 00:00:00 2001 From: Hamza Alqurneh Date: Wed, 4 Nov 2026 15:08:56 +0300 Subject: [PATCH] fix: client SDK release review findings --- .github/workflows/sdk-release.yml | 4 +-- SW.Bitween.Client/BitweenClient.cs | 10 +++++- SW.Bitween.Client/ExchangeQuery.cs | 12 +++++-- SW.Bitween.Client/Exchanges.cs | 9 +++-- SW.Bitween.HttpTests/BitweenClientTests.cs | 42 +++++++++++++++++++++- docs/client-sdk.md | 9 ++--- sdk/node-client/README.md | 2 +- sdk/node-client/src/index.d.ts | 4 +-- sdk/node-client/src/index.js | 19 +++++----- sdk/node-client/test/client.test.js | 7 ++++ 10 files changed, 90 insertions(+), 28 deletions(-) diff --git a/.github/workflows/sdk-release.yml b/.github/workflows/sdk-release.yml index 8effee63..dd1583ad 100644 --- a/.github/workflows/sdk-release.yml +++ b/.github/workflows/sdk-release.yml @@ -102,8 +102,8 @@ jobs: run: working-directory: sdk/node-client steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: "22" registry-url: "https://registry.npmjs.org" diff --git a/SW.Bitween.Client/BitweenClient.cs b/SW.Bitween.Client/BitweenClient.cs index 9a641460..154756c0 100644 --- a/SW.Bitween.Client/BitweenClient.cs +++ b/SW.Bitween.Client/BitweenClient.cs @@ -84,7 +84,15 @@ internal async Task PostAsync(string path, object body, CancellationToken cancel internal async Task PostForStreamAsync(string path, object body, CancellationToken cancellation) { var response = await SendAsync(() => Post(path, body), HttpCompletionOption.ResponseHeadersRead, cancellation); - return await response.Content.ReadAsStreamAsync(cancellation); + try + { + return await response.Content.ReadAsStreamAsync(cancellation); + } + catch + { + response.Dispose(); + throw; + } } static HttpRequestMessage Post(string path, object body) => diff --git a/SW.Bitween.Client/ExchangeQuery.cs b/SW.Bitween.Client/ExchangeQuery.cs index 02fd4f71..bc04e054 100644 --- a/SW.Bitween.Client/ExchangeQuery.cs +++ b/SW.Bitween.Client/ExchangeQuery.cs @@ -58,8 +58,8 @@ public class ExchangeQuery public string? ErrorContains { get; set; } /// - /// Promoted properties by key: the value contains the text, ignoring case, or with a null text, - /// the exchange promoted the key at all. Every one has to hold. + /// Promoted properties by key: the value contains the text, ignoring case, or with a null or blank + /// text, the exchange promoted the key at all. Every one has to hold. /// public IDictionary? PromotedProperties { get; set; } @@ -132,6 +132,9 @@ public sealed class ExchangeSelection /// Every exchange matches, but those in . public static ExchangeSelection Matching(ExchangeQuery query, IEnumerable? except = null) { + // No ids adds no filter, so it would select every exchange rather than none. + if (query.Ids is { Count: 0 }) + throw new ArgumentException("Ids is empty: it would match every exchange.", nameof(query)); var filters = query.ToFilters(); // An empty filter reads as no filter, and the selection would fall back to its ids, which are // none. Every attempt, not only the newest, matches everything without narrowing it. @@ -174,6 +177,9 @@ public sealed class Page { public List Result { get; set; } = []; - /// How many match. An exchange search stops counting at 10,001, meaning more than 10,000. + /// + /// How many match, or 0 when searched without counting. An exchange search stops counting at + /// 10,001, meaning more than 10,000. + /// public int TotalCount { get; set; } } diff --git a/SW.Bitween.Client/Exchanges.cs b/SW.Bitween.Client/Exchanges.cs index 9a0cf4f9..039ab6ee 100644 --- a/SW.Bitween.Client/Exchanges.cs +++ b/SW.Bitween.Client/Exchanges.cs @@ -1,4 +1,3 @@ -using System.Net; using SW.Bitween.Model; namespace SW.Bitween.Client; @@ -54,8 +53,8 @@ public Task GetRetryTreeAsync(string id, CancellationToken can bitween.GetAsync($"api/xchanges/retrytree?id={Uri.EscapeDataString(id)}", cancellation); /// - /// Runs an exchange again from its input file, and returns the new attempt's id: null when the - /// account may retry but not view exchanges, so can't look it up. With + /// Runs an exchange again from its input file, and returns the new attempt's id: null when it can't + /// look that up, because the account may retry but not view exchanges or the lookup failed. With /// , the subscription's current settings are used rather than those the /// exchange ran with. An exchange is retried at most once: one that has been is refused with /// ALREADY_RETRIED, so retry its newest attempt instead. One with an automatic retry booked @@ -68,13 +67,13 @@ public Task GetRetryTreeAsync(string id, CancellationToken can await bitween.PostAsync($"api/xchanges/{Uri.EscapeDataString(id)}/retry", new XchangeRetry { Reason = reason, Reset = reset }, cancellation); // Answered with nothing: the new attempt is the newest that says it retries this. Looking it up - // needs exchanges.view, and the retry is made by then, so a refusal mustn't read as the retry's. + // needs exchanges.view, and the retry is made by then, so a failed lookup mustn't read as the retry's. try { var retry = await SearchAsync(new ExchangeQuery { RetryFor = id }, 0, 1, false, cancellation); return retry.Result.FirstOrDefault()?.Id; } - catch (BitweenClientException e) when (e.Status == HttpStatusCode.Forbidden) + catch (Exception e) when (e is BitweenClientException or HttpRequestException) { return null; } diff --git a/SW.Bitween.HttpTests/BitweenClientTests.cs b/SW.Bitween.HttpTests/BitweenClientTests.cs index 1a69043a..aab90501 100644 --- a/SW.Bitween.HttpTests/BitweenClientTests.cs +++ b/SW.Bitween.HttpTests/BitweenClientTests.cs @@ -4,7 +4,9 @@ using System.IO.Compression; using System.Linq; using System.Net; +using System.Net.Http; using System.Net.Http.Json; +using System.Threading; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore; using SW.Bitween.Client; @@ -119,6 +121,9 @@ await fixture.InDbAsync(db => db.Set().Where(r => ids.Cont Assert.Equal(2, preview.WillRetry); Assert.Equal(3, (await bitween.Exchanges.SearchAsync(partners)).TotalCount); + // No ids would read as no filter, and select every exchange. + Assert.Throws(() => ExchangeSelection.Matching(new ExchangeQuery { Ids = [] })); + var plan = await bitween.Exchanges.BulkRetryAsync(ExchangeSelection.Of(ids.Take(2))); Assert.Equal(2, plan.WillRetry); Assert.Equal(5, (await bitween.Exchanges.SearchAsync(partners)).TotalCount); @@ -135,6 +140,26 @@ await fixture.InDbAsync(db => db.Set().Where(r => ids.Cont Assert.All(ids, id => Assert.Contains(archive.Entries, e => e.FullName.Contains(id))); } + [Fact] + public async Task Runs_a_scheduled_retry_now() + { + var (partnerId, _) = await ExchangesAsync(1); + using var bitween = await ClientAsync("exchanges.view", "exchanges.operate"); + var id = (await bitween.Exchanges.SearchAsync(new ExchangeQuery { PartnerIds = [partnerId] })).Result.Single().Id; + // Booked as a retry policy books one, an hour out so the retry job leaves it. + await fixture.InDbAsync(db => + { + db.Add(new Domain.DelayedRetry { Id = id, On = DateTime.UtcNow.AddHours(1) }); + return db.SaveChangesAsync(); + }); + var booked = new ScheduledRetryQuery { From = DateTimeOffset.UtcNow }; + Assert.Contains((await bitween.ScheduledRetries.SearchAsync(booked)).Result, r => r.Id == id); + + await bitween.ScheduledRetries.RunNowAsync(id); + Assert.Single((await bitween.Exchanges.SearchAsync(new ExchangeQuery { RetryFor = id })).Result); + Assert.DoesNotContain((await bitween.ScheduledRetries.SearchAsync(booked)).Result, r => r.Id == id); + } + [Fact] public async Task Says_what_Bitween_refused_and_why() { @@ -155,7 +180,9 @@ public async Task Says_what_Bitween_refused_and_why() // Calls made at once with a bad token share its one refusal. Sent once a call, the fifth would // lock the address out, and a good token from it with them. - using var address = fixture.Client(); + var signIns = new SignInCounter(); + using var address = fixture.App.CreateDefaultClient(new Uri("https://localhost"), signIns); + address.DefaultRequestHeaders.Add("X-Forwarded-For", $"10.{Random.Shared.Next(255)}.{Random.Shared.Next(255)}.{Random.Shared.Next(1, 255)}"); using var stranger = new BitweenClient(address, "bwt_" + new string('x', 40)); var refusals = await Task.WhenAll(Enumerable.Range(0, 10) .Select(_ => Assert.ThrowsAsync(() => stranger.Exchanges.SearchAsync()))); @@ -164,7 +191,20 @@ public async Task Says_what_Bitween_refused_and_why() Assert.Equal(HttpStatusCode.BadRequest, refused.Status); Assert.StartsWith("This token isn't valid", refused.Message); }); + Assert.Equal(1, signIns.Count); using var neighbour = new BitweenClient(address, await TokenAsync("exchanges.view")); Assert.Equal(2, (await neighbour.Exchanges.SearchAsync(new ExchangeQuery { PartnerIds = [partnerId] })).TotalCount); } + + /// Counts the calls that trade a token for a session. + sealed class SignInCounter : DelegatingHandler + { + public int Count; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellation) + { + if (request.RequestUri!.AbsolutePath == "/api/accounts/servicetoken") Interlocked.Increment(ref Count); + return base.SendAsync(request, cancellation); + } + } } diff --git a/docs/client-sdk.md b/docs/client-sdk.md index dc8c5e72..cb0b9c04 100644 --- a/docs/client-sdk.md +++ b/docs/client-sdk.md @@ -17,7 +17,7 @@ role, and issue it a token. The token, starting `bwt_`, is shown once. | To | The role needs | |---|---| | Search exchanges, read their files and retry chains, see scheduled retries, check and export | `exchanges.view` | -| Retry, bulk retry, and run a scheduled retry now | `exchanges.operate` as well | +| Retry, bulk retry, and run a scheduled retry now | `exchanges.operate` (without `exchanges.view` too, a retry can't answer with the new attempt's id) | The SDK trades the token for a session on its first call, and again whenever the session runs out or is refused, so the token is all you hand it. @@ -94,7 +94,7 @@ Types come with it. Rows have the API's camelCase names, and times are ISO strin | `Exchanges.GetAsync(id)` | `exchanges.get(id)` | One exchange, or null | | `Exchanges.ReadFileAsync(key)` | `exchanges.readFile(key)` | A file's text, by the row's `inputKey`, `outputKey` or `responseKey` | | `Exchanges.GetRetryTreeAsync(id)` | `exchanges.retryTree(id)` | Every attempt in the retry chain, oldest first | -| `Exchanges.RetryAsync(id, reset)` | `exchanges.retry(id, { reset })` | Runs it again from its input, and answers with the new attempt's id: null when the account may retry but not view, so can't look it up. `reset` uses the subscription's current settings. | +| `Exchanges.RetryAsync(id, reset)` | `exchanges.retry(id, { reset })` | Runs it again from its input, and answers with the new attempt's id: null when it can't look that up, because the account may retry but not view or the lookup failed. The retry is made either way. `reset` uses the subscription's current settings. | | `Exchanges.PreviewBulkRetryAsync(selection)` | `exchanges.previewBulkRetry(selection)` | What a bulk retry would do, changing nothing | | `Exchanges.BulkRetryAsync(selection)` | `exchanges.bulkRetry(selection)` | Retries a selection, and says what it did | | `Exchanges.CheckExportAsync(selection)` | `exchanges.checkExport(selection)` | How many the export takes, and how many are older than storage keeps files | @@ -117,7 +117,7 @@ to hold. | `CorrelationId` / `correlationId` | This correlation id | | `LatestOnly` / `latestOnly` | Only the newest attempt of each retry chain | | `ErrorContains` / `errorContains` | The error contains the text, ignoring case | -| `PromotedProperties` / `promotedProperties` | By key: the value contains the text, or with no text, the key was promoted | +| `PromotedProperties` / `promotedProperties` | By key: the value contains the text, or with no text (null or blank), the key was promoted | | `Filters` / `filters` | Filters in Bitween's own `Field:Rule:Value` form, for the rest. See [Exchanges](api-reference.md#exchanges). | A scheduled retry query takes `SubscriptionId`, `InformationTypeId`, `ErrorContains`, and `From` and @@ -126,7 +126,8 @@ A scheduled retry query takes `SubscriptionId`, `InformationTypeId`, `ErrorConta ### Selections Bulk retry and export take a selection: exchanges by id, or every exchange a query matches, however -many pages that is, with some left out. +many pages that is, with some left out. A query with an empty id list is refused rather than read as no ids at all, +which would match every exchange. ```csharp ExchangeSelection.Of(["a1b2...", "c3d4..."]) diff --git a/sdk/node-client/README.md b/sdk/node-client/README.md index 59540cc6..ac65d805 100644 --- a/sdk/node-client/README.md +++ b/sdk/node-client/README.md @@ -14,7 +14,7 @@ for (const exchange of failed.result) console.log(exchange.id, exchange.exceptio ``` The token is a service account's (Team > Service accounts in Bitween). Keep it on your server, never -in browser code. See [Client SDK](../../docs/client-sdk.md) for every call, queries, selections and errors. +in browser code. See [Client SDK](https://github.com/simplify9/Bitween-api/blob/releases/r10.0/docs/client-sdk.md) for every call, queries, selections and errors. Tests: `node --test`. They run against a stand-in Bitween; the same calls against a real one are tested in `SW.Bitween.HttpTests`, through the .NET client. diff --git a/sdk/node-client/src/index.d.ts b/sdk/node-client/src/index.d.ts index 99407184..6103e94d 100644 --- a/sdk/node-client/src/index.d.ts +++ b/sdk/node-client/src/index.d.ts @@ -21,7 +21,7 @@ export interface ExchangeQuery { latestOnly?: boolean; /** The error contains this text, ignoring case. */ errorContains?: string; - /** By key: the value contains the text, ignoring case, or with null, the exchange promoted the key. */ + /** By key: the value contains the text, ignoring case, or with null or blank text, the exchange promoted the key. */ promotedProperties?: Record; /** Filters in Bitween's own form, `Field:Rule:Value`, for what the rest doesn't cover. */ filters?: string[]; @@ -32,7 +32,7 @@ export type ExchangeSelection = { ids: string[] } | { matching: ExchangeQuery; e export interface Page { result: T[]; - /** How many match. An exchange search stops counting at 10,001, meaning more than 10,000. */ + /** How many match, or 0 when searched with `count: false`. An exchange search stops counting at 10,001, meaning more than 10,000. */ totalCount: number; } diff --git a/sdk/node-client/src/index.js b/sdk/node-client/src/index.js index 10a02855..0c28bb4b 100644 --- a/sdk/node-client/src/index.js +++ b/sdk/node-client/src/index.js @@ -65,7 +65,7 @@ function exchangeFilters(query = {}) { filters.push(values.length === 1 ? `${field}:1:${values[0]}` : `${field}:9:numeric|${values.join("|")}`); }; if (query.status != null) { - if (!(query.status in STATUS)) throw new TypeError(`status is one of ${Object.keys(STATUS).join(", ")}.`); + if (!Object.hasOwn(STATUS, query.status)) throw new TypeError(`status is one of ${Object.keys(STATUS).join(", ")}.`); filters.push(`StatusFilter:1:${STATUS[query.status]}`); } oneOf("PartnerId", query.partnerIds); @@ -88,8 +88,10 @@ function exchangeFilters(query = {}) { /** A selection as the bulk endpoints take it: `{ ids }`, or `{ matching, except }`. */ function selectionBody(selection) { - if (selection?.ids) return { ids: [...selection.ids] }; - if (!selection?.matching) throw new TypeError("A selection is { ids } or { matching, except }."); + if (!selection?.ids === !selection?.matching) throw new TypeError("A selection is { ids } or { matching, except }."); + if (selection.ids) return { ids: [...selection.ids] }; + // No ids adds no filter, so it would select every exchange rather than none. + if (selection.matching.ids?.length === 0) throw new TypeError("matching.ids is empty: it would match every exchange."); const filters = exchangeFilters(selection.matching); // An empty filter reads as no filter, and the selection would fall back to its ids, which are // none. Every attempt, not only the newest, matches everything without narrowing it. @@ -134,21 +136,20 @@ class Exchanges { } /** - * Runs an exchange again from its input file, and resolves to the new attempt's id: null when the - * account may retry but not view exchanges, so can't look it up. An exchange is retried at most + * Runs an exchange again from its input file, and resolves to the new attempt's id: null when it + * can't look that up, because the account may retry but not view exchanges or the lookup failed. An exchange is retried at most * once: one that has been is refused with ALREADY_RETRIED. One with an automatic retry booked is * refused with AUTO_RETRY_SCHEDULED; use scheduledRetries.runNow instead. */ async retry(id, { reset = false, reason = "Manual retry" } = {}) { await this.#request("POST", `api/xchanges/${encodeURIComponent(id)}/retry`, { reason, reset }); // Answered with nothing: the new attempt is the newest that says it retries this. Looking it up - // needs exchanges.view, and the retry is made by then, so a refusal mustn't read as the retry's. + // needs exchanges.view, and the retry is made by then, so a failed lookup mustn't read as the retry's. try { const retry = await this.search({ retryFor: id }, { page: 0, size: 1, count: false }); return retry.result[0]?.id ?? null; - } catch (e) { - if (e instanceof BitweenClientError && e.status === 403) return null; - throw e; + } catch { + return null; } } diff --git a/sdk/node-client/test/client.test.js b/sdk/node-client/test/client.test.js index ecaa4403..0f7df1dc 100644 --- a/sdk/node-client/test/client.test.js +++ b/sdk/node-client/test/client.test.js @@ -60,6 +60,7 @@ test("searches with the Exchanges page's filters, signed in once for calls made assert.deepStrictEqual([search.query.page, search.query.size, search.query.count], ["2", "10", "false"]); assert.deepStrictEqual([plain.filters, plain.query.page, plain.query.size, plain.query.count], [[], "0", "25", undefined]); await assert.rejects(bitween.exchanges.search({ status: "broken" }), TypeError); + await assert.rejects(bitween.exchanges.search({ status: "toString" }), TypeError); }); test("signs in again once when the session is refused", async () => { @@ -127,6 +128,9 @@ test("reads on past an old chain's retries to the exchange, and retries without // The retry is made; only looking it up is refused, so it answers with no id rather than throwing. assert.strictEqual(await bitween.exchanges.retry("x"), null); + + const { client: failing } = await fakeBitween((r) => r.method === "POST" ? [200, null] : [503, null]); + assert.strictEqual(await failing().exchanges.retry("x"), null); }); test("sends a selection as the bulk endpoints take it", async () => { @@ -136,6 +140,9 @@ test("sends a selection as the bulk endpoints take it", async () => { await bitween.exchanges.previewBulkRetry({ matching: { status: "failed", partnerIds: [7] }, except: ["c"] }, { reset: true }); await bitween.exchanges.checkExport({ matching: {} }); await assert.rejects(bitween.exchanges.bulkRetry({}), TypeError); + await assert.rejects(bitween.exchanges.bulkRetry({ ids: [], matching: { status: "failed" } }), TypeError); + // No ids would read as no filter, and match every exchange. + await assert.rejects(bitween.exchanges.bulkRetry({ matching: { ids: [] } }), TypeError); const [byId, matching, everything] = seen.requests.map((r) => [r.path, r.body]); assert.deepStrictEqual(byId, ["/api/xchanges/bulkretry", { ids: ["a", "b"], reason: "Bulk retry", reset: false }]);