From b3c32345fec6346d8a61da60c4dfc640935884c1 Mon Sep 17 00:00:00 2001 From: Hamza Alqurneh Date: Wed, 4 Nov 2026 16:02:42 +0300 Subject: [PATCH] fix: client SDK second review findings --- SW.Bitween.Client/ExchangeQuery.cs | 7 ++++--- SW.Bitween.Client/Exchanges.cs | 2 +- SW.Bitween.HttpTests/BitweenClientTests.cs | 24 +++++++++++++++------- docs/client-sdk.md | 4 ++-- sdk/node-client/src/index.js | 5 +++-- sdk/node-client/test/client.test.js | 3 ++- 6 files changed, 29 insertions(+), 16 deletions(-) diff --git a/SW.Bitween.Client/ExchangeQuery.cs b/SW.Bitween.Client/ExchangeQuery.cs index bc04e054..69a930bd 100644 --- a/SW.Bitween.Client/ExchangeQuery.cs +++ b/SW.Bitween.Client/ExchangeQuery.cs @@ -132,9 +132,10 @@ 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)); + // An empty list adds no filter, so it would widen the selection rather than narrow it. + if (query.Ids is { Count: 0 } || query.PartnerIds is { Count: 0 } || query.SubscriptionIds is { Count: 0 } + || query.InformationTypeIds is { Count: 0 }) + throw new ArgumentException("An id list is empty: it would widen the selection, not narrow it.", 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. diff --git a/SW.Bitween.Client/Exchanges.cs b/SW.Bitween.Client/Exchanges.cs index 039ab6ee..6729d553 100644 --- a/SW.Bitween.Client/Exchanges.cs +++ b/SW.Bitween.Client/Exchanges.cs @@ -73,7 +73,7 @@ await bitween.PostAsync($"api/xchanges/{Uri.EscapeDataString(id)}/retry", var retry = await SearchAsync(new ExchangeQuery { RetryFor = id }, 0, 1, false, cancellation); return retry.Result.FirstOrDefault()?.Id; } - catch (Exception e) when (e is BitweenClientException or HttpRequestException) + catch (Exception e) when (e is not OperationCanceledException || !cancellation.IsCancellationRequested) { return null; } diff --git a/SW.Bitween.HttpTests/BitweenClientTests.cs b/SW.Bitween.HttpTests/BitweenClientTests.cs index aab90501..2178545c 100644 --- a/SW.Bitween.HttpTests/BitweenClientTests.cs +++ b/SW.Bitween.HttpTests/BitweenClientTests.cs @@ -121,8 +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. + // An empty list would read as no filter, and widen the selection. Assert.Throws(() => ExchangeSelection.Matching(new ExchangeQuery { Ids = [] })); + Assert.Throws(() => ExchangeSelection.Matching(new ExchangeQuery { Status = ExchangeStatus.Failed, PartnerIds = [] })); var plan = await bitween.Exchanges.BulkRetryAsync(ExchangeSelection.Of(ids.Take(2))); Assert.Equal(2, plan.WillRetry); @@ -184,8 +185,10 @@ public async Task Says_what_Bitween_refused_and_why() 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()))); + var calls = Enumerable.Range(0, 10) + .Select(_ => Assert.ThrowsAsync(() => stranger.Exchanges.SearchAsync())).ToList(); + signIns.Open.SetResult(); + var refusals = await Task.WhenAll(calls); Assert.All(refusals, refused => { Assert.Equal(HttpStatusCode.BadRequest, refused.Status); @@ -196,15 +199,22 @@ public async Task Says_what_Bitween_refused_and_why() Assert.Equal(2, (await neighbour.Exchanges.SearchAsync(new ExchangeQuery { PartnerIds = [partnerId] })).TotalCount); } - /// Counts the calls that trade a token for a session. + /// Counts the calls that trade a token for a session, and holds them until opened. sealed class SignInCounter : DelegatingHandler { public int Count; - protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellation) + /// Set once every call that should share a sign-in has been made. + public readonly TaskCompletionSource Open = new(TaskCreationOptions.RunContinuationsAsynchronously); + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellation) { - if (request.RequestUri!.AbsolutePath == "/api/accounts/servicetoken") Interlocked.Increment(ref Count); - return base.SendAsync(request, cancellation); + if (request.RequestUri!.AbsolutePath == "/api/accounts/servicetoken") + { + Interlocked.Increment(ref Count); + await Open.Task; + } + return await base.SendAsync(request, cancellation); } } } diff --git a/docs/client-sdk.md b/docs/client-sdk.md index cb0b9c04..bc5c8c73 100644 --- a/docs/client-sdk.md +++ b/docs/client-sdk.md @@ -126,8 +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. A query with an empty id list is refused rather than read as no ids at all, -which would match every exchange. +many pages that is, with some left out. A query with an empty list of ids, partners, subscriptions or information types +is refused: an empty list adds no filter, so it would widen the selection rather than narrow it. ```csharp ExchangeSelection.Of(["a1b2...", "c3d4..."]) diff --git a/sdk/node-client/src/index.js b/sdk/node-client/src/index.js index 0c28bb4b..e5cc1f41 100644 --- a/sdk/node-client/src/index.js +++ b/sdk/node-client/src/index.js @@ -90,8 +90,9 @@ function exchangeFilters(query = {}) { function selectionBody(selection) { 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."); + // An empty list adds no filter, so it would widen the selection rather than narrow it. + for (const key of ["ids", "partnerIds", "subscriptionIds", "informationTypeIds"]) + if (selection.matching[key]?.length === 0) throw new TypeError(`matching.${key} is empty: it would widen the selection, not narrow it.`); 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. diff --git a/sdk/node-client/test/client.test.js b/sdk/node-client/test/client.test.js index 0f7df1dc..04ed7fa1 100644 --- a/sdk/node-client/test/client.test.js +++ b/sdk/node-client/test/client.test.js @@ -141,8 +141,9 @@ test("sends a selection as the bulk endpoints take it", async () => { 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. + // An empty list would read as no filter, and widen the selection. await assert.rejects(bitween.exchanges.bulkRetry({ matching: { ids: [] } }), TypeError); + await assert.rejects(bitween.exchanges.bulkRetry({ matching: { status: "failed", partnerIds: [] } }), 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 }]);