zeroshade commented on PR #1174:
URL: https://github.com/apache/iceberg-go/pull/1174#issuecomment-4684191320
A few small follow-ups from re-reviewing the negotiation tests and
`resolveEndpoints`:
**1. Integration test doesn't actually prove negotiation (re: Copilot's
still-open comment on `endpoints_integration_test.go:40`)**
Copilot already flagged that `require.NotEmpty(t, cat.endpoints, ...)`
always passes, since `resolveEndpoints` never returns an empty set. Worth
extending that point: the per-endpoint loop underneath it doesn't close the gap
either, because every endpoint it checks (`endpointListNamespaces`,
`endpointCreateNamespace`, `endpointListTables`, `endpointCreateTable`,
`endpointLoadTable`, `endpointDeleteTable`, `endpointCommitTransaction`) is
part of `defaultEndpoints`. So if the live fixture advertised no `endpoints`
field, the client would silently fall back, every `contains` check would still
pass, and the test's stated goal — "a format mismatch would surface here ...
rather than as a silent fallback" — wouldn't actually hold.
To genuinely prove the set came off the wire rather than from fallback,
assert `contains` on at least one endpoint that is **not** in
`defaultEndpoints` — e.g. `endpointTableExists` (HEAD),
`endpointTableCredentials`, or one of the view endpoints — whichever the
fixture advertises.
**2. Duplicated endpoint tables (nit)**
`allEndpoints` (`[]endpoint` in `endpoints.go`) and `allEndpointStrings`
(`[]string` in `rest_test.go`) encode the same set twice and can drift
independently. Consider deriving the test strings from the constants via
`.String()` (e.g. build `allEndpointStrings` from `allEndpoints`) so there's a
single source of truth.
**3. Partial-parse semantics in `resolveEndpoints` (nit)**
When the advertised list mixes parseable and unparseable entries, the
parseable ones are kept and the rest dropped (now logged via `slog.Warn` — 👍).
The edge case worth documenting: a single garbled or annotated entry — e.g.
`"GET /v1/{prefix}/namespaces (deprecated)"` — alongside otherwise-valid
entries yields a tiny, highly-restricted set where most ops are gated off,
while still looking like a successful negotiation. A short doc note on
`resolveEndpoints` stating that a non-empty advertised list is treated as
authoritative (so parse failures reduce capability) would make that contract
explicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
