diff --git a/internal/http/client.go b/internal/http/client.go index 6c5222839..00bcb8238 100644 --- a/internal/http/client.go +++ b/internal/http/client.go @@ -66,6 +66,25 @@ func InitClientWithProfile(cfg *ini.File, profileOverride string) { } } +// FetchObjectsParallel fetches one object per id, ten requests at a time. +// +// The result is INDEX-ALIGNED with ids, and stays aligned when ignoreErrors is +// set: an id whose fetch failed keeps its slot, holding the zero value of T. That +// is the contract, not an oversight, and compacting the result would be a silent +// breaking change — seven call sites in internal/services/browser pair +// objects[i] with a name they hold in a parallel slice, so dropping a hole there +// would attach one region's details to another region's name. +// +// What that leaves each caller to do depends on T, and measured across the 30 +// call sites all three cases are already handled: +// +// - ignoreErrors false: a failure returns an error and no holes exist (2 sites). +// - T is a map or a struct: the hole is nil and has to be skipped explicitly +// (20 sites do, several of them through FetchExpandedArray, which filters). +// - T is a slice: the hole is a nil slice, which ranges as empty (8 sites). +// +// So it is safe to leave alone, and worth saying so: the padding looks like a bug +// until you know who depends on it. func FetchObjectsParallel[T any](path string, ids []any, ignoreErrors bool) ([]T, error) { var ( parallelRequests = 10 diff --git a/internal/http/client_test.go b/internal/http/client_test.go index b48f1aef4..467207090 100644 --- a/internal/http/client_test.go +++ b/internal/http/client_test.go @@ -65,3 +65,36 @@ func TestFetchObjectsParallel_IgnoredErrorLogging(t *testing.T) { td.Cmp(t, strings.Contains(run(true), "error fetching"), true, "ignored errors must be logged when debug is enabled") } + +// The index alignment of FetchObjectsParallel is a contract that seven call sites +// in internal/services/browser depend on: they pair objects[i] with a name held +// in a parallel slice, so a result compacted to drop failures would attach one +// region's details to another region's name. +// +// The padding reads like a bug — a slice with nil holes in it — which is exactly +// why it needs a test. This one fails if anyone makes the result dense. +func TestFetchObjectsParallelKeepsFailedItemsInPlace(t *testing.T) { + httpmock.Activate(t) + client, err := ovh.NewClient("ovh-eu", "k", "s", "c") + td.Require(t).CmpNoError(err) + saved := Client + Client = client + t.Cleanup(func() { Client = saved }) + + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/auth/time", + httpmock.NewStringResponder(200, "0")) + for _, id := range []string{"a", "c"} { + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/thing/"+id, + httpmock.NewStringResponder(200, `{"name":"`+id+`"}`)) + } + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/thing/b", + httpmock.NewStringResponder(500, `{"message":"nope"}`)) + + objects, err := FetchObjectsParallel[map[string]any]("/thing/%s", []any{"a", "b", "c"}, true) + + td.Require(t).CmpNoError(err, "ignoreErrors means the batch still succeeds") + td.Require(t).Cmp(len(objects), 3, "one slot per id, however many failed") + td.Cmp(t, objects[0]["name"], "a") + td.CmpNil(t, objects[1], "the failed id keeps its slot, holding the zero value") + td.Cmp(t, objects[2]["name"], "c", "and the ids after it are not shifted up") +}