laskoviymishka commented on code in PR #1174: URL: https://github.com/apache/iceberg-go/pull/1174#discussion_r3383655756
########## catalog/rest/endpoints.go: ########## @@ -0,0 +1,173 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "fmt" + "net/http" + "strings" +) + +// ErrEndpointNotSupported indicates the server did not advertise a REST endpoint +// required by the requested operation. It wraps [ErrRESTError]. +var ErrEndpointNotSupported = fmt.Errorf("%w: endpoint not supported by server", ErrRESTError) + +// pathPrefix is the part of every endpoint template already encoded in the base +// URI (API version plus optional catalog prefix); reqPath strips it. +const pathPrefix = "/v1/{prefix}" + +// endpoint identifies a REST catalog operation by HTTP method and path template +// (e.g. "GET /v1/{prefix}/namespaces"). Servers advertise the endpoints they +// support in the config response so the client can negotiate capabilities. +// +// Each endpoint both gates capability and builds the request path (see +// [endpoint.reqPath]), making it the single source of truth for its operation. +type endpoint struct { + method string + path string +} + +func (e endpoint) String() string { return e.method + " " + e.path } + +// reqPath renders the request path relative to the base URI, substituting params +// in order for the "{...}" placeholders that follow [pathPrefix]. +func (e endpoint) reqPath(params ...string) []string { + segs := strings.Split(strings.TrimPrefix(e.path, pathPrefix+"/"), "/") + + out := make([]string, len(segs)) + p := 0 + for i, s := range segs { + if strings.HasPrefix(s, "{") && strings.HasSuffix(s, "}") { Review Comment: There's no bounds check on `params[p]`, so an under-supplied call panics at runtime rather than failing gracefully. The variadic `...string` signature gives us no compile-time arity enforcement either, so a future endpoint with an extra placeholder — or just a miscounted call site — becomes a production panic on the request hot path. I'd either return an `error` from `reqPath` and propagate it, or panic with a descriptive message naming the endpoint and the expected vs actual param count. Whichever we pick, an `init()` self-check that renders every endpoint constant with its expected arity would catch miscounts at startup instead of on first call. wdyt? ########## catalog/rest/rest.go: ########## @@ -796,7 +802,7 @@ func (r *Catalog) fetchTableCreds(ctx context.Context, ident []string, location return nil, err } - ret, err := doGet[loadCredentialsResponse](ctx, r.baseURI, []string{"namespaces", ns, "tables", tbl, "credentials"}, + ret, err := doGet[loadCredentialsResponse](ctx, r.baseURI, endpointTableCredentials.reqPath(ns, tbl), Review Comment: This is the one call site that got the `reqPath` migration but not the `check()` gate every other op received. `endpointTableCredentials` isn't in the spec's default set, so against a legacy server this fires an unnegotiated GET, the 404/405 maps to `ErrNoSuchTable`, and the real cause ("server doesn't advertise credentials") gets masked. I'd add `if err := r.endpoints.check(endpointTableCredentials); err != nil { return nil, err }` at the top, matching the other ops. ########## catalog/rest/endpoints.go: ########## @@ -0,0 +1,173 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "fmt" + "net/http" + "strings" +) + +// ErrEndpointNotSupported indicates the server did not advertise a REST endpoint +// required by the requested operation. It wraps [ErrRESTError]. Review Comment: `ErrEndpointNotSupported` wraps `ErrRESTError`, which means any caller doing `errors.Is(err, ErrRESTError)` to retry or log transport failures will also catch capability errors — semantically these are different (Java throws `UnsupportedOperationException`, PyIceberg `NotImplementedError`). Non-blocking, but I'd lean toward a standalone sentinel that doesn't satisfy `errors.Is(ErrRESTError)`. If we keep the wrap, a doc note telling callers to check `ErrEndpointNotSupported` before `ErrRESTError` would help. ########## catalog/rest/endpoints.go: ########## @@ -0,0 +1,173 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "fmt" + "net/http" + "strings" +) + +// ErrEndpointNotSupported indicates the server did not advertise a REST endpoint +// required by the requested operation. It wraps [ErrRESTError]. +var ErrEndpointNotSupported = fmt.Errorf("%w: endpoint not supported by server", ErrRESTError) + +// pathPrefix is the part of every endpoint template already encoded in the base +// URI (API version plus optional catalog prefix); reqPath strips it. +const pathPrefix = "/v1/{prefix}" + +// endpoint identifies a REST catalog operation by HTTP method and path template +// (e.g. "GET /v1/{prefix}/namespaces"). Servers advertise the endpoints they +// support in the config response so the client can negotiate capabilities. +// +// Each endpoint both gates capability and builds the request path (see +// [endpoint.reqPath]), making it the single source of truth for its operation. +type endpoint struct { + method string + path string +} + +func (e endpoint) String() string { return e.method + " " + e.path } + +// reqPath renders the request path relative to the base URI, substituting params +// in order for the "{...}" placeholders that follow [pathPrefix]. +func (e endpoint) reqPath(params ...string) []string { + segs := strings.Split(strings.TrimPrefix(e.path, pathPrefix+"/"), "/") + + out := make([]string, len(segs)) + p := 0 + for i, s := range segs { + if strings.HasPrefix(s, "{") && strings.HasSuffix(s, "}") { + out[i], p = params[p], p+1 + } else { + out[i] = s + } + } + + return out +} + +// endpointFromString parses an endpoint from its "METHOD PATH" wire form, +// erroring unless the string is exactly two whitespace-separated tokens. +func endpointFromString(s string) (endpoint, error) { + fields := strings.Fields(s) + if len(fields) != 2 { + return endpoint{}, fmt.Errorf("%w: invalid endpoint %q (expected \"METHOD PATH\")", ErrRESTError, s) + } + + return endpoint{method: fields[0], path: fields[1]}, nil +} + +// Well-known REST catalog endpoints, keyed by advertised method and path template. +var ( + endpointListNamespaces = endpoint{http.MethodGet, "/v1/{prefix}/namespaces"} + endpointLoadNamespace = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}"} + endpointNamespaceExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}"} + endpointCreateNamespace = endpoint{http.MethodPost, "/v1/{prefix}/namespaces"} + endpointUpdateNamespace = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/properties"} + endpointDeleteNamespace = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}"} + endpointCommitTransaction = endpoint{http.MethodPost, "/v1/{prefix}/transactions/commit"} + + endpointListTables = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables"} + endpointLoadTable = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointTableExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointCreateTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables"} + endpointUpdateTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointDeleteTable = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointRenameTable = endpoint{http.MethodPost, "/v1/{prefix}/tables/rename"} + endpointRegisterTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/register"} + endpointReportMetrics = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"} + endpointTableCredentials = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"} + + endpointListViews = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/views"} + endpointLoadView = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointViewExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointCreateView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/views"} + endpointUpdateView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointDeleteView = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointRenameView = endpoint{http.MethodPost, "/v1/{prefix}/views/rename"} + endpointRegisterView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/register-view"} +) + +// fallbackEndpoints is assumed when a server advertises no "endpoints" list. It +// covers every operation the client can invoke, for compatibility with servers +// that predate endpoint negotiation. +var fallbackEndpoints = []endpoint{ + // namespace and table operations + endpointListNamespaces, endpointLoadNamespace, endpointCreateNamespace, + endpointUpdateNamespace, endpointDeleteNamespace, + endpointListTables, endpointLoadTable, endpointCreateTable, + endpointUpdateTable, endpointDeleteTable, endpointRenameTable, + endpointRegisterTable, endpointReportMetrics, endpointCommitTransaction, + + // view operations + endpointListViews, endpointLoadView, endpointCreateView, + endpointUpdateView, endpointDeleteView, endpointRenameView, + + // Existence checks and view registration are included for parity with past + // behavior. When an advertised set omits a HEAD endpoint, Check*Exists still + // degrades to a GET. + endpointNamespaceExists, endpointTableExists, endpointViewExists, + endpointRegisterView, +} + +// endpointSet is the set of endpoints a catalog server supports. +type endpointSet map[endpoint]struct{} + +// newEndpointSet builds a set from a list of endpoints. +func newEndpointSet(eps []endpoint) endpointSet { + s := endpointSet{} + for _, e := range eps { + s[e] = struct{}{} + } + + return s +} + +func (s endpointSet) contains(e endpoint) bool { + _, ok := s[e] + + return ok +} + +// check returns [ErrEndpointNotSupported] if the endpoint is not in the set. A +// nil set (capabilities never negotiated) permits every endpoint. +func (s endpointSet) check(e endpoint) error { + if s == nil || s.contains(e) { + return nil + } + + return fmt.Errorf("%w: %s", ErrEndpointNotSupported, e) +} + +// resolveEndpoints builds the effective endpoint set from the advertised list. +func resolveEndpoints(advertised []string) endpointSet { + s := endpointSet{} + for _, raw := range advertised { + if e, err := endpointFromString(raw); err == nil { + s[e] = struct{}{} + } + } + // A server that advertises nothing (or only unparseable values) gets the + // backward-compatibility fallback set. Review Comment: There's a subtle asymmetry in the partial-garbage case. If the advertised list is all unparseable we fall back to the open set; but if even one entry parses, we keep just that one and silently drop the rest — yielding a tiny, highly-restricted set where most ops fail. So a server that adds an annotated entry like `"GET /v1/{prefix}/namespaces (deprecated)"` could break nearly everything while looking like a successful negotiation. I'd `slog.Warn` per dropped entry at minimum so this is debuggable in prod (right now unparseable entries vanish with no trace) — and consider treating a non-empty advertised list as authoritative rather than silently discarding. Today a meaningful new endpoint Go doesn't yet parse just disappears. ########## catalog/rest/endpoints.go: ########## @@ -0,0 +1,173 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "fmt" + "net/http" + "strings" +) + +// ErrEndpointNotSupported indicates the server did not advertise a REST endpoint +// required by the requested operation. It wraps [ErrRESTError]. +var ErrEndpointNotSupported = fmt.Errorf("%w: endpoint not supported by server", ErrRESTError) + +// pathPrefix is the part of every endpoint template already encoded in the base +// URI (API version plus optional catalog prefix); reqPath strips it. +const pathPrefix = "/v1/{prefix}" + +// endpoint identifies a REST catalog operation by HTTP method and path template +// (e.g. "GET /v1/{prefix}/namespaces"). Servers advertise the endpoints they +// support in the config response so the client can negotiate capabilities. +// +// Each endpoint both gates capability and builds the request path (see +// [endpoint.reqPath]), making it the single source of truth for its operation. +type endpoint struct { + method string + path string +} + +func (e endpoint) String() string { return e.method + " " + e.path } + +// reqPath renders the request path relative to the base URI, substituting params +// in order for the "{...}" placeholders that follow [pathPrefix]. +func (e endpoint) reqPath(params ...string) []string { + segs := strings.Split(strings.TrimPrefix(e.path, pathPrefix+"/"), "/") + + out := make([]string, len(segs)) + p := 0 + for i, s := range segs { + if strings.HasPrefix(s, "{") && strings.HasSuffix(s, "}") { + out[i], p = params[p], p+1 + } else { + out[i] = s + } + } + + return out +} + +// endpointFromString parses an endpoint from its "METHOD PATH" wire form, +// erroring unless the string is exactly two whitespace-separated tokens. +func endpointFromString(s string) (endpoint, error) { + fields := strings.Fields(s) + if len(fields) != 2 { + return endpoint{}, fmt.Errorf("%w: invalid endpoint %q (expected \"METHOD PATH\")", ErrRESTError, s) + } + + return endpoint{method: fields[0], path: fields[1]}, nil +} + +// Well-known REST catalog endpoints, keyed by advertised method and path template. +var ( + endpointListNamespaces = endpoint{http.MethodGet, "/v1/{prefix}/namespaces"} + endpointLoadNamespace = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}"} + endpointNamespaceExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}"} + endpointCreateNamespace = endpoint{http.MethodPost, "/v1/{prefix}/namespaces"} + endpointUpdateNamespace = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/properties"} + endpointDeleteNamespace = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}"} + endpointCommitTransaction = endpoint{http.MethodPost, "/v1/{prefix}/transactions/commit"} + + endpointListTables = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables"} + endpointLoadTable = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointTableExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointCreateTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables"} + endpointUpdateTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointDeleteTable = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"} + endpointRenameTable = endpoint{http.MethodPost, "/v1/{prefix}/tables/rename"} + endpointRegisterTable = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/register"} + endpointReportMetrics = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"} + endpointTableCredentials = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"} + + endpointListViews = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/views"} + endpointLoadView = endpoint{http.MethodGet, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointViewExists = endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointCreateView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/views"} + endpointUpdateView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointDeleteView = endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}/views/{view}"} + endpointRenameView = endpoint{http.MethodPost, "/v1/{prefix}/views/rename"} + endpointRegisterView = endpoint{http.MethodPost, "/v1/{prefix}/namespaces/{namespace}/register-view"} +) + +// fallbackEndpoints is assumed when a server advertises no "endpoints" list. It +// covers every operation the client can invoke, for compatibility with servers +// that predate endpoint negotiation. Review Comment: The fallback set doesn't match the spec's mandated default. The REST OpenAPI `CatalogConfig` defines exactly 14 endpoints a client MUST assume when the server sends none, and Java's `DEFAULT_ENDPOINTS` matches that exactly; the view group is a separate set merged only when `view-endpoints-supported=true` (default false). Two divergences matter here. The HEAD existence endpoints (`endpointNamespaceExists`/`TableExists`/`ViewExists`) shouldn't be in fallback at all — their *absence* is what's supposed to trigger the HEAD→GET degradation, so including them defeats the fallback we built in the Check*Exists path. And the whole view CRUD group is merged unconditionally, which tells the client a legacy non-view server supports every view op. I'd trim fallback to the 14 spec endpoints, drop the three HEAD entries, and move the view endpoints to a separate set gated by `view-endpoints-supported`. ########## catalog/rest/rest.go: ########## @@ -834,8 +840,12 @@ func (r *Catalog) listTablesPage(ctx context.Context, namespace table.Identifier if err := checkValidNamespace(namespace); err != nil { return nil, "", err } + // Unsupported listing yields an empty result rather than an error. + if r.endpoints != nil && !r.endpoints.contains(endpointListTables) { Review Comment: The nil-guard semantics are inconsistent across the file and I think it's worth unifying. `check()` already treats a nil set as "permit everything", but the list and Check*Exists paths re-implement that with an inline `r.endpoints != nil && !contains(...)`. Post-`fetchConfig` the set is never nil, so the guard here is effectively dead — and direct construction of the exported `Catalog` struct (bypassing `fetchConfig`) would hit the nil branch and silently behave differently from a negotiated catalog. I'd add a small `allowed(e) bool { return s == nil || s.contains(e) }` on `endpointSet` so these sites read `if !r.endpoints.allowed(endpointListTables)`, and let it carry the nil semantics in one place. Same pattern applies to `listNamespacesPage` and `listViewsPage`. ########## catalog/rest/rest.go: ########## @@ -1327,7 +1394,22 @@ func (r *Catalog) CheckNamespaceExists(ctx context.Context, namespace table.Iden return false, err } - err := doHead(ctx, r.baseURI, []string{"namespaces", strings.Join(namespace, namespaceSeparator)}, + // If the server does not advertise the HEAD existence endpoint, fall back to + // a GET (load) for compatibility with servers that predate it. + if r.endpoints != nil && !r.endpoints.contains(endpointNamespaceExists) { Review Comment: The `r.endpoints != nil &&` here conflicts with the fallback design. Java's condition is just `if (endpoints.contains(V1_TABLE_EXISTS))` with no nil special-case — and that works because HEAD endpoints are *absent* from their default set, so absence naturally routes to GET. Right now nil means "allow everything" in `check()` but "use HEAD" in this path, which is two meanings for the same state. If we trim the HEAD entries out of fallback (see the fallback-set comment), then `!contains(headEP)` alone handles both the nil-negotiated and legacy cases and the guard can drop the nil check. Same for `CheckTableExists` and `CheckViewExists`. One related catch: when a server omits *both* the HEAD and the GET endpoint, this fallback calls `LoadNamespaceProperties`, which re-checks its own endpoint and can return `ErrEndpointNotSupported` — which then bubbles up from a bool-returning "does it exist" call, since the branch only catches `ErrNoSuchNamespace`. Worth handling that case explicitly too. ########## catalog/rest/endpoints_test.go: ########## @@ -0,0 +1,115 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEndpointFromString(t *testing.T) { + t.Parallel() + + cases := []struct { + in string + want endpoint + wantErr bool + }{ + {in: "GET /v1/{prefix}/namespaces", want: endpoint{http.MethodGet, "/v1/{prefix}/namespaces"}}, + {in: "POST /v1/{prefix}/transactions/commit", want: endpoint{http.MethodPost, "/v1/{prefix}/transactions/commit"}}, + {in: "HEAD /v1/{prefix}/namespaces/{namespace}", want: endpoint{http.MethodHead, "/v1/{prefix}/namespaces/{namespace}"}}, + // Extra surrounding/internal whitespace is tolerated. + {in: " DELETE /v1/{prefix}/namespaces/{namespace} ", want: endpoint{http.MethodDelete, "/v1/{prefix}/namespaces/{namespace}"}}, + {in: "", wantErr: true}, + {in: "GET", wantErr: true}, + {in: "GET /a /b", wantErr: true}, + } + + for _, tc := range cases { + got, err := endpointFromString(tc.in) + if tc.wantErr { + require.Error(t, err) + assert.ErrorIs(t, err, ErrRESTError) + + continue + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } +} + +func TestEndpointRoundTripString(t *testing.T) { + t.Parallel() + Review Comment: The unit suite never actually exercises negotiation, which is the whole point of the PR. The existing `RestCatalogSuite` fixture returns no `endpoints` field, so `resolveEndpoints` always lands in fallback and `check()` can never return an error — CI would stay green even if `check()` were a no-op. I'd add a mock-based test where `/v1/config` returns a minimal endpoints list and assert the real behavior: `CreateTable` → `ErrEndpointNotSupported`, `ListNamespaces` still succeeds, and an unsupported list op returns empty rather than erroring. And separately, nothing tests `reqPath` substitution itself (e.g. `LoadTable.reqPath("ns","tbl")` == `["namespaces","ns","tables","tbl"]`, `CommitTransaction.reqPath()` == `["transactions","commit"]`, plus the too-few-params case) — given it's the most failure-prone function and a wrong result silently routes every call to the wrong URL, a table-driven test over 0/1/2/5-param endpoints would be worth adding. ########## catalog/rest/rest.go: ########## @@ -834,8 +840,12 @@ func (r *Catalog) listTablesPage(ctx context.Context, namespace table.Identifier if err := checkValidNamespace(namespace); err != nil { return nil, "", err } + // Unsupported listing yields an empty result rather than an error. + if r.endpoints != nil && !r.endpoints.contains(endpointListTables) { + return nil, "", nil Review Comment: Separate from the nil-guard idiom: silently returning an empty list when the endpoint is unsupported is a real footgun for callers. They can't distinguish "this namespace has no tables" from "the server doesn't support listing tables" — which is dangerous for sync/CDC downstreams that treat empty as authoritative. This matches Java's behavior (PyIceberg instead raises), so I'm not asking to change it. But I'd at least document it in the `ListTables`/`ListNamespaces`/`ListViews` godoc so the empty-on-unsupported contract is explicit. Same applies to the other two list pagers. -- 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]
