Skip to content

Commit 1da91e2

Browse files
committed
test(osgen): pin perOpErrorTypeName <-> errwrap.OperationWrappers coupling
perOpErrorTypeName's hardcoded switch and errwrap.OperationWrappers' wrapper-count map are coupled by an unstated invariant: a group has a per-op aggregator type iff its catalog entry declares 2+ wrappers. Today both sides match, but nothing checks them, so a future catalog edit can desync the two without any signal -- the dispatch keeps referencing a per-op type that's no longer reachable, or worse, emits an empty type name when a 2+-wrapper group lacks a switch arm. Add a coupling test that asserts both directions: - every group naming a per-op aggregator type has 2+ wrappers in OperationWrappers - every catalog entry with 2+ wrappers has a non-empty per-op aggregator type Iterates the catalog directly rather than a duplicate list of switch arms, so a new switch arm or catalog entry is exercised automatically. Failure messages are actionable: they name the offending group, the current state, and the remediation (add wrappers, remove switch arm, or add a hand-written aggregator). Ref: opensearch-project#844 (review round 3, F7) Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
1 parent 755636d commit 1da91e2

2 files changed

Lines changed: 81 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
2424
- Enhanced cluster readiness checking for improved test reliability: `testutil.NewClient()` now includes readiness validation (health + cluster state + nodes info)
2525
- Add `Status` field (`json.RawMessage`) to `TasksGetResp`, `TasksListTask`, and `TaskCancelInfo` for polymorphic task status data; add typed status structs matching the OpenSearch API specification: `BulkByScrollTaskStatus`, `ReplicationTaskStatus`, `ResyncTaskStatus`, `PersistentTaskStatus`; add `Parse*` helpers and `BulkByScrollTaskStatusOrException` for sliced task status ([#788](https://github.com/opensearch-project/opensearch-go/issues/788))
2626
- Test parallelization support via TEST_PARALLEL environment variable (default: CPU cores - 1, minimum 1)
27+
- Add `cmd/osgen/emit.TestPerOpErrorTypeName_CatalogConsistency` to pin the catalog <-> switch coupling between `emit.PerOpErrorTypeName` and `errwrap.OperationWrappers`. Asserts three directions: every group naming a per-op aggregator type has 2+ wrappers in the catalog, every catalog entry with 2+ wrappers names a per-op aggregator type, and every group named by the switch is present in the catalog. Does not pin the runtime `emittableWrappers`/`resolveErrorWrappers` paths; today those sets coincide for the only 2+-wrapper groups (`msearch` / `msearch_template`) ([#857](https://github.com/opensearch-project/opensearch-go/pull/857))
2728
- opensearchapi/testutil package with test suite, client helpers, and JSON comparison utilities
2829
- Add typed path builders in `internal/path/` generated from the OpenAPI spec via `cmd/osgen` for compile-time URL construction safety ([#617](https://github.com/opensearch-project/opensearch-go/issues/617), [#650](https://github.com/opensearch-project/opensearch-go/issues/650))
2930
- `sync.Pool`-backed `[]byte` buffers eliminate per-request allocation churn; buffers over 4 KiB are discarded to bound pool growth

cmd/osgen/emit/frag_dispatch_test.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,86 @@ func TestDispatchFragment_Imports(t *testing.T) {
306306
}
307307
}
308308

309+
// TestPerOpErrorTypeName_CatalogConsistency pins the catalog <-> switch
310+
// coupling between [emit.PerOpErrorTypeName] and
311+
// [errwrap.OperationWrappers]. It asserts three directions, all keyed
312+
// off groups present in either side:
313+
//
314+
// 1. every group naming a per-op aggregator type has 2+ wrappers in
315+
// the catalog (otherwise the per-op type is referenced from
316+
// generated code but the catalog says only 0 or 1 wrappers exist);
317+
// 2. every catalog entry with 2+ wrappers names a per-op aggregator
318+
// type (otherwise the dispatch emits an empty per-op type name);
319+
// 3. every group named by the switch is present in the catalog at all
320+
// (otherwise a switch arm exists for a group neither loop above
321+
// would iterate, so it could go stale silently).
322+
//
323+
// What this does NOT pin: the runtime emission decision is gated by
324+
// `len(emittable) >= 2` from [DispatchFragment.emittableWrappers],
325+
// which further filters the catalog through the emission `wrappers`
326+
// map and per-wrapper `Applies` predicates, and by
327+
// `resolveErrorWrappers` (which prefers the spec extension
328+
// `x-error-responses` over the catalog). Today those sets coincide for
329+
// the only 2+-wrapper groups (`msearch` / `msearch_template`); a
330+
// future spec edit or wrapper-table change could still desync those
331+
// without this test failing. Tracked separately if it ever matters.
332+
func TestPerOpErrorTypeName_CatalogConsistency(t *testing.T) {
333+
t.Parallel()
334+
335+
// switchGroups enumerates every group named by perOpErrorTypeName's
336+
// hardcoded switch. Kept in sync with the switch by hand: when a new
337+
// arm is added there, add it here too. (3) below catches the inverse
338+
// drift -- a switch arm whose group never reaches the catalog.
339+
switchGroups := []string{
340+
errwrap.GroupMSearch,
341+
errwrap.GroupMSearchTemplate,
342+
}
343+
344+
// (1) Forward: every group the catalog names with a per-op
345+
// aggregator type must declare 2+ wrappers there.
346+
for group := range errwrap.OperationWrappers {
347+
typeName := emit.PerOpErrorTypeName(group)
348+
if typeName == "" {
349+
continue
350+
}
351+
t.Run("type_for_"+group, func(t *testing.T) {
352+
t.Parallel()
353+
require.GreaterOrEqual(t, len(errwrap.OperationWrappers[group]), 2,
354+
"group %q has per-op error type %q but only %d wrapper(s) in OperationWrappers; either add wrappers or remove the switch arm",
355+
group, typeName, len(errwrap.OperationWrappers[group]))
356+
})
357+
}
358+
359+
// (2) Reverse: every catalog entry with 2+ wrappers must name a
360+
// per-op aggregator type.
361+
for group, wrappers := range errwrap.OperationWrappers {
362+
if len(wrappers) < 2 {
363+
continue
364+
}
365+
t.Run("catalog_entry_"+group, func(t *testing.T) {
366+
t.Parallel()
367+
require.NotEmpty(t, emit.PerOpErrorTypeName(group),
368+
"group %q declares %d wrappers %v in OperationWrappers but PerOpErrorTypeName returns empty; add a switch arm and a hand-written %q-style aggregator type",
369+
group, len(wrappers), wrappers, group)
370+
})
371+
}
372+
373+
// (3) Switch-arm catalog presence: every group named by the
374+
// per-op switch must appear in OperationWrappers. A switch arm
375+
// for a group missing from the catalog is dead code: neither
376+
// loop above iterates it, so without this check it could
377+
// outlive the catalog entry that justified it.
378+
for _, group := range switchGroups {
379+
t.Run("switch_arm_in_catalog_"+group, func(t *testing.T) {
380+
t.Parallel()
381+
_, ok := errwrap.OperationWrappers[group]
382+
require.True(t, ok,
383+
"perOpErrorTypeName has a switch arm for group %q but the group is absent from errwrap.OperationWrappers; remove the arm or restore the catalog entry",
384+
group)
385+
})
386+
}
387+
}
388+
309389
// ---------------------------------------------------------------------------
310390
// PartialFailureFragment: per-Resp helper methods + aggregator
311391
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)