feat(catalog/rest): endpoint negotiation#1174
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds REST catalog endpoint negotiation by reading an endpoints list from /v1/config, using it to (a) build request paths from a single source of truth and (b) fail fast with a dedicated error when the server doesn’t advertise a required endpoint.
Changes:
- Extend
/v1/configparsing to include an optionalendpointsfield and resolve it into an internalendpointSet. - Centralize REST endpoint templates + request path construction in a new
endpoints.go, and gate many catalog operations via negotiated endpoint support checks. - Add unit + integration tests covering endpoint parsing, set resolution, and negotiation with the integration fixture.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
catalog/rest/rest.go |
Parses advertised endpoints and gates operations / builds paths via endpoint templates. |
catalog/rest/endpoints.go |
Introduces endpoint templates, parsing, capability set resolution, and request path generation. |
catalog/rest/endpoints_test.go |
Adds unit tests for endpoint parsing and resolution behavior. |
catalog/rest/endpoints_integration_test.go |
Adds an integration test to validate negotiation against the integration fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *Catalog) fetchTableCreds(ctx context.Context, ident []string, location string) (iceberg.Properties, error) { | ||
| ns, tbl, err := splitIdentForPath(ident) | ||
| if err != nil { | ||
| 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), | ||
| r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable}) |
| for i, s := range segs { | ||
| if strings.HasPrefix(s, "{") && strings.HasSuffix(s, "}") { | ||
| out[i], p = params[p], p+1 | ||
| } else { | ||
| out[i] = s | ||
| } | ||
| } |
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestEndpointNegotiation(t *testing.T) { | ||
| // Loading the catalog reads /v1/config and negotiates the endpoint set the | ||
| // fixture advertises. The live server uses the same wire format as our | ||
| // endpoint constants, so the negotiated set must contain the core | ||
| // operations; a format mismatch would surface here as a missing entry | ||
| // rather than as a silent fallback. | ||
| cat, err := NewCatalog(context.Background(), "rest", "http://localhost:8181") | ||
| require.NoError(t, err) | ||
|
|
||
| require.NotEmpty(t, cat.endpoints, "fixture should advertise an endpoints list") | ||
| for _, want := range []endpoint{ |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice change — routing all call sites through endpoint.reqPath is much cleaner, the per-op check() gating reads well, and the round-trip test over fallbackEndpoints is a good touch.
I’d hold before merge. Few things here:
The main one is reqPath: there’s no bounds check on params[p], so an under-supplied call panics at runtime. Since the signature is variadic, we also get no compile-time arity check. A future endpoint with one more placeholder, or just a miscounted call site, becomes a hot-path request panic. I’d either return an error or panic with a clear message, and add an init() self-check that renders every endpoint constant so miscounts fail at startup.
The other big one is spec parity. fallbackEndpoints doesn’t match the REST CatalogConfig default set: it includes the HEAD existence endpoints, whose absence is what should trigger HEAD → GET degradation, and it pulls in the full view group unconditionally where Java gates it behind view-endpoints-supported. I’d trim fallback to the spec defaults, split views out, and drop the HEAD entries.
Also, fetchTableCreds got the path migration but not the check() gate, so a legacy server now returns a confusing ErrNoSuchTable instead of a clear unsupported error.
Things I’d settle before merge:
- bounds-check / arity-guard
reqPath, plus startup self-check - gate
fetchTableCredsbehindendpointTableCredentials - trim
fallbackEndpointsto the spec default set; gate views; dropHEAD - unify nil-guard semantics, probably via
allowed() - warn instead of silently dropping unparseable advertised endpoints
- add tests for real negotiation and direct
reqPathcoverage
One small thing while to keep hygiene standarts: mind aligning the PR title with the conventional-commit style the rest of the repo uses? Something like feat(catalog/rest): endpoint negotiation.
Once those are fixed, happy to take another pass and approve.
rambleraptor
changed the title
|
@laskoviymishka wow thank you for such a thorough review! I really appreciate it. I made those changes, ptal! |
The REST Catalog sends out a list of supported endpoints via
/v1/config. This is important for features like views + server-side planning, where the client has to understand if the catalog supports certain features.I've created a list of endpoints and a mechanism for iceberg-go to error out if a particular method isn't supported by the server.
Note: This was created with the help of GenAI, since creating all of the endpoint objects is really duplicative work.