laskoviymishka commented on code in PR #1432: URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3559026652
########## catalog/rest/rest_functions_test.go: ########## @@ -0,0 +1,428 @@ +// 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_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/catalog/rest" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int). +const testFunctionMetadataJSON = `{ + "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2", + "format-version": 1, + "definitions": [ + { + "definition-id": "int", + "parameters": [{"name": "x", "type": "int"}], + "return-type": "int", + "function-type": "udf", + "versions": [ + { + "version-id": 1, + "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}], + "timestamp-ms": 1000 + } + ], + "current-version-id": 1 + } + ], + "definition-log": [ + {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]} + ] +}` + +func (r *RestCatalogSuite) TestListFunctions200() { + customPageSize := 100 + namespace := "accounting" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + for k, v := range TestHeaders { + r.Equal(v, req.Header.Values(k)) + } + + pageToken := req.URL.Query().Get("pageToken") + pageSize := req.URL.Query().Get("pageSize") + r.Equal("", pageToken) + r.Equal(strconv.Itoa(customPageSize), pageSize) + + // Function identifiers are CatalogObjectIdentifier values: flat + // arrays of hierarchy levels, not {namespace, name} objects. + json.NewEncoder(w).Encode(map[string]any{ + "identifiers": []any{ + []string{"accounting", "tax", "paid"}, + []string{"accounting", "tax", "owed"}, + }, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + // Passing in a custom page size through context + ctx := cat.SetPageSize(context.Background(), customPageSize) + + var lastErr error + functions := make([]table.Identifier, 0) + for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) { + functions = append(functions, function) Review Comment: This appends `function` before checking `err`. If the iterator ever yields `(empty-ident, err)`, the empty identifier gets appended before the error is caught — the assertion on the collected slice would then be checking corrupted data. `TestListFunctionsPaginationErrorOnSubsequentPage` gets this right (checks `err`, breaks, then appends). I'd invert this loop to match — check `err` first, append on success. ########## catalog/catalog.go: ########## @@ -39,6 +39,7 @@ import ( "github.com/apache/iceberg-go" iceinternal "github.com/apache/iceberg-go/internal" "github.com/apache/iceberg-go/table" + "github.com/apache/iceberg-go/udf" Review Comment: Putting `FunctionCatalog` in the base `catalog` package means `catalog` now imports `udf`, and since `LoadFunction` returns `*udf.UDF`, every consumer of the base package transitively depends on `udf`. That's also asymmetric with views: there's no `ViewCatalog` interface here — `LoadView`/`ListViews`/`CheckViewExists` are just methods on `*rest.Catalog`. I'd follow that precedent and keep `FunctionCatalog` local to `catalog/rest` (the `var _ catalog.FunctionCatalog` assertion in rest.go goes away with it). If we genuinely need a cross-package interface, I'd at least avoid the concrete `*udf.UDF` return type so the base package stays leaf-independent. wdyt? ########## catalog/rest/rest.go: ########## @@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v return view.New(identifier, metadata, rsp.MetadataLoc), nil } + +// Catalog implements the optional function (SQL UDF) read support defined +// by the REST spec: list and load. The function endpoints are not part of +// the spec's assumed default endpoint set, so they are only used when the +// server advertises them. +var _ catalog.FunctionCatalog = (*Catalog)(nil) + +// ListFunctions returns the function (SQL UDF) identifiers under a +// namespace, with the returned identifiers containing the information +// required to load the function via this catalog. +func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] { + return func(yield func(table.Identifier, error) bool) { + pageSize := r.getPageSize(ctx) + var pageToken string + + for { + functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize) + if err != nil { + yield(table.Identifier{}, err) + + return + } + for _, function := range functions { + if !yield(function, nil) { + return + } + } + if nextPageToken == "" { + return + } + pageToken = nextPageToken + } + } +} + +func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) { + if err := checkValidNamespace(namespace); err != nil { + return nil, "", err + } + // Unsupported listing yields an empty result rather than an error. + if !r.endpoints.allowed(endpointListFunctions) { + return nil, "", nil + } + ns := r.encodeNamespace(namespace) + path, err := endpointListFunctions.reqPath(ns) + if err != nil { + return nil, "", err + } + uri := r.baseURI.JoinPath(path...) + + v := url.Values{} + if pageSize > 0 { + v.Set("pageSize", strconv.Itoa(pageSize)) + } + if pageToken != "" { + v.Set("pageToken", pageToken) + } + + uri.RawQuery = v.Encode() + // Function identifiers are CatalogObjectIdentifier values: ordered + // hierarchy levels (the namespace levels followed by the function + // name), not the {namespace, name} object tables and views use. + type resp struct { + Identifiers [][]string `json:"identifiers"` + NextPageToken string `json:"next-page-token,omitempty"` + } + + rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace}) + if err != nil { + return nil, "", err + } + + out := make([]table.Identifier, len(rsp.Identifiers)) + for i, levels := range rsp.Identifiers { + out[i] = table.Identifier(levels) + } + + return out, rsp.NextPageToken, nil +} + +// loadFunctionResponse contains the response from loading a function. +type loadFunctionResponse struct { + MetadataLoc string `json:"metadata-location"` + RawMetadata json.RawMessage `json:"metadata"` +} + +// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded +// definitions are included in the single metadata response. +func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) { + if err := r.endpoints.check(endpointLoadFunction); err != nil { + return nil, err + } + + ns, fn, err := r.splitFunctionIdentForPath(identifier) + if err != nil { + return nil, err + } + + path, err := endpointLoadFunction.reqPath(ns, fn) + if err != nil { + return nil, err + } + + rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path, + r.cl, map[int]error{ + http.StatusNotFound: catalog.ErrNoSuchFunction, + }) + if err != nil { + return nil, err + } + + metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata) Review Comment: If a server returns `metadata-location` with no `metadata` key, `RawMetadata` is nil and this surfaces as `"unexpected end of JSON input"`, which is confusing. The spec marks metadata required so only a buggy server hits it, but a `len(rsp.RawMetadata) == 0` guard with a clearer message would be a cheap robustness win. Non-blocking. ########## catalog/rest/rest.go: ########## @@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v return view.New(identifier, metadata, rsp.MetadataLoc), nil } + +// Catalog implements the optional function (SQL UDF) read support defined +// by the REST spec: list and load. The function endpoints are not part of +// the spec's assumed default endpoint set, so they are only used when the +// server advertises them. +var _ catalog.FunctionCatalog = (*Catalog)(nil) + +// ListFunctions returns the function (SQL UDF) identifiers under a +// namespace, with the returned identifiers containing the information +// required to load the function via this catalog. +func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] { + return func(yield func(table.Identifier, error) bool) { + pageSize := r.getPageSize(ctx) + var pageToken string + + for { + functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize) + if err != nil { + yield(table.Identifier{}, err) + + return + } + for _, function := range functions { + if !yield(function, nil) { + return + } + } + if nextPageToken == "" { + return + } + pageToken = nextPageToken + } + } +} + +func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) { + if err := checkValidNamespace(namespace); err != nil { + return nil, "", err + } + // Unsupported listing yields an empty result rather than an error. + if !r.endpoints.allowed(endpointListFunctions) { + return nil, "", nil + } + ns := r.encodeNamespace(namespace) + path, err := endpointListFunctions.reqPath(ns) + if err != nil { + return nil, "", err + } + uri := r.baseURI.JoinPath(path...) + + v := url.Values{} + if pageSize > 0 { + v.Set("pageSize", strconv.Itoa(pageSize)) + } + if pageToken != "" { + v.Set("pageToken", pageToken) + } + + uri.RawQuery = v.Encode() + // Function identifiers are CatalogObjectIdentifier values: ordered + // hierarchy levels (the namespace levels followed by the function + // name), not the {namespace, name} object tables and views use. + type resp struct { + Identifiers [][]string `json:"identifiers"` + NextPageToken string `json:"next-page-token,omitempty"` + } + + rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace}) + if err != nil { + return nil, "", err + } + + out := make([]table.Identifier, len(rsp.Identifiers)) + for i, levels := range rsp.Identifiers { + out[i] = table.Identifier(levels) + } + + return out, rsp.NextPageToken, nil +} + +// loadFunctionResponse contains the response from loading a function. +type loadFunctionResponse struct { + MetadataLoc string `json:"metadata-location"` + RawMetadata json.RawMessage `json:"metadata"` +} + +// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded +// definitions are included in the single metadata response. +func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) { + if err := r.endpoints.check(endpointLoadFunction); err != nil { + return nil, err + } + + ns, fn, err := r.splitFunctionIdentForPath(identifier) + if err != nil { + return nil, err + } + + path, err := endpointLoadFunction.reqPath(ns, fn) + if err != nil { + return nil, err + } + + rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path, + r.cl, map[int]error{ + http.StatusNotFound: catalog.ErrNoSuchFunction, + }) + if err != nil { + return nil, err + } + + metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata) + if err != nil { + return nil, fmt.Errorf("failed to parse function metadata: %w", err) + } + + return udf.New(identifier, metadata, rsp.MetadataLoc), nil +} + +// CheckFunctionExists returns if the function exists. The REST spec defines +// no HEAD endpoint for functions, so existence is checked by loading the +// function; if loading is unsupported, its ErrEndpointNotSupported surfaces +// rather than a bogus "not found". +func (r *Catalog) CheckFunctionExists(ctx context.Context, identifier table.Identifier) (bool, error) { + // Validate first, as the table and view existence checks do: an invalid + // identifier surfaces as an error, so ErrNoSuchFunction below can only + // mean a server-reported "not found". + if err := catalog.ValidateFunctionIdentifier(identifier); err != nil { Review Comment: This validation is redundant — `LoadFunction` immediately calls `splitFunctionIdentForPath`, which calls `ValidateFunctionIdentifier` again, so the identifier is validated twice. The comment is also inaccurate: `CheckTableExists`/`CheckViewExists` don't do an explicit up-front validate, they call `splitIdentForPath`/`splitViewIdentForPath` directly (one pass) and then HEAD/Load. I'd drop the explicit call and the comment here — `TestLoadFunctionInvalidIdentifier` still passes because `LoadFunction` validates first. ########## catalog/catalog.go: ########## @@ -192,6 +194,30 @@ type PurgeableTable interface { PurgeTable(ctx context.Context, identifier table.Identifier) error } +// FunctionCatalog is an optional interface implemented by catalogs that +// support the Iceberg REST function (SQL UDF) endpoints. The function +// endpoints are not part of the spec's assumed default endpoint set, so +// servers must advertise them: unsupported loads report an +// endpoint-not-supported error and unsupported listings yield no results. Review Comment: This doc promises "unsupported loads report an endpoint-not-supported error," but `ErrEndpointNotSupported` lives in `catalog/rest`, not `catalog`. A caller using the documented type-assertion pattern imports only `catalog` and can't `errors.Is` against a sentinel it can't name. Either move `ErrEndpointNotSupported` up to `catalog`, or drop the endpoint-negotiation language from this doc and describe it only in the rest.go methods. This one resolves naturally if we go with the "keep it in `catalog/rest`" direction from the layering comment above. ########## catalog/rest/rest.go: ########## @@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v return view.New(identifier, metadata, rsp.MetadataLoc), nil } + +// Catalog implements the optional function (SQL UDF) read support defined +// by the REST spec: list and load. The function endpoints are not part of +// the spec's assumed default endpoint set, so they are only used when the +// server advertises them. +var _ catalog.FunctionCatalog = (*Catalog)(nil) + +// ListFunctions returns the function (SQL UDF) identifiers under a +// namespace, with the returned identifiers containing the information +// required to load the function via this catalog. +func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] { + return func(yield func(table.Identifier, error) bool) { + pageSize := r.getPageSize(ctx) + var pageToken string + + for { + functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize) + if err != nil { + yield(table.Identifier{}, err) + + return + } + for _, function := range functions { + if !yield(function, nil) { + return + } + } + if nextPageToken == "" { + return + } + pageToken = nextPageToken + } + } +} + +func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) { + if err := checkValidNamespace(namespace); err != nil { + return nil, "", err + } + // Unsupported listing yields an empty result rather than an error. + if !r.endpoints.allowed(endpointListFunctions) { + return nil, "", nil + } + ns := r.encodeNamespace(namespace) + path, err := endpointListFunctions.reqPath(ns) + if err != nil { + return nil, "", err + } + uri := r.baseURI.JoinPath(path...) + + v := url.Values{} + if pageSize > 0 { + v.Set("pageSize", strconv.Itoa(pageSize)) + } + if pageToken != "" { + v.Set("pageToken", pageToken) + } + + uri.RawQuery = v.Encode() + // Function identifiers are CatalogObjectIdentifier values: ordered + // hierarchy levels (the namespace levels followed by the function + // name), not the {namespace, name} object tables and views use. + type resp struct { + Identifiers [][]string `json:"identifiers"` + NextPageToken string `json:"next-page-token,omitempty"` + } + + rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace}) + if err != nil { + return nil, "", err + } + + out := make([]table.Identifier, len(rsp.Identifiers)) + for i, levels := range rsp.Identifiers { + out[i] = table.Identifier(levels) + } + + return out, rsp.NextPageToken, nil +} + +// loadFunctionResponse contains the response from loading a function. +type loadFunctionResponse struct { + MetadataLoc string `json:"metadata-location"` + RawMetadata json.RawMessage `json:"metadata"` +} + +// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded +// definitions are included in the single metadata response. +func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) { + if err := r.endpoints.check(endpointLoadFunction); err != nil { + return nil, err + } + + ns, fn, err := r.splitFunctionIdentForPath(identifier) + if err != nil { + return nil, err + } + + path, err := endpointLoadFunction.reqPath(ns, fn) + if err != nil { + return nil, err + } + + rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path, + r.cl, map[int]error{ + http.StatusNotFound: catalog.ErrNoSuchFunction, Review Comment: This maps every 404 to `ErrNoSuchFunction`, but the spec's GET load defines two 404 bodies — `NoSuchNamespaceException` and `NoSuchFunctionException`. The status-code-only override ignores `error.type`, so `LoadFunction` on a deleted namespace returns `ErrNoSuchFunction`, and `CheckFunctionExists` then reports `(false, nil)` for a missing namespace. Java's view/table handlers discriminate on the type string before falling through. This is a pre-existing gap for `LoadView`/`LoadTable` too, so not a blocker for this PR — but worth a look as a consistent follow-up, since the function path is a fresh chance to get it right. ########## udf/udf.go: ########## @@ -0,0 +1,58 @@ +// 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 udf + +import ( + "slices" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// UDF is a function (SQL UDF) loaded from a catalog: its catalog identifier, +// its metadata, and the location the metadata was loaded from. +type UDF struct { + identifier table.Identifier + metadata Metadata + metadataLocation string +} + +// New creates a UDF from its catalog identifier, metadata and metadata +// location. +func New(ident table.Identifier, meta Metadata, metadataLocation string) *UDF { + return &UDF{ + identifier: ident, + metadata: meta, + metadataLocation: metadataLocation, + } +} + +// Equals reports whether two UDFs have the same identifier, metadata +// location and metadata. +func (u UDF) Equals(other UDF) bool { Review Comment: `New` returns `*UDF` but `Equals` takes a value, so callers end up writing `fn.Equals(*same)` (visible in the test). Minor friction — I'd either use pointer receivers/argument to match the constructor, or drop a one-line doc noting the value-copy semantics are intentional. ########## catalog/rest/rest.go: ########## @@ -1038,6 +1039,14 @@ func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string, string, return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil } +func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string, string, error) { + if err := catalog.ValidateFunctionIdentifier(ident); err != nil { + return "", "", err + } + + return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil Review Comment: Using `catalog.TableNameFromIdent` to pull the function name works (it's the last element) but reads oddly — `LoadView` has the same wart. Not for this PR, but a type-agnostic `catalog.ObjectNameFromIdent` would clean up all three call sites. ########## catalog/rest/rest_functions_test.go: ########## @@ -0,0 +1,428 @@ +// 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_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/catalog/rest" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int). +const testFunctionMetadataJSON = `{ + "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2", + "format-version": 1, + "definitions": [ + { + "definition-id": "int", + "parameters": [{"name": "x", "type": "int"}], + "return-type": "int", + "function-type": "udf", + "versions": [ + { + "version-id": 1, + "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}], + "timestamp-ms": 1000 + } + ], + "current-version-id": 1 + } + ], + "definition-log": [ + {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]} + ] +}` + +func (r *RestCatalogSuite) TestListFunctions200() { + customPageSize := 100 + namespace := "accounting" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + for k, v := range TestHeaders { + r.Equal(v, req.Header.Values(k)) + } + + pageToken := req.URL.Query().Get("pageToken") + pageSize := req.URL.Query().Get("pageSize") + r.Equal("", pageToken) + r.Equal(strconv.Itoa(customPageSize), pageSize) + + // Function identifiers are CatalogObjectIdentifier values: flat + // arrays of hierarchy levels, not {namespace, name} objects. + json.NewEncoder(w).Encode(map[string]any{ + "identifiers": []any{ + []string{"accounting", "tax", "paid"}, + []string{"accounting", "tax", "owed"}, + }, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + // Passing in a custom page size through context + ctx := cat.SetPageSize(context.Background(), customPageSize) + + var lastErr error + functions := make([]table.Identifier, 0) + for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) { + functions = append(functions, function) + if err != nil { + lastErr = err + r.FailNow("unexpected error:", err) + } + } + + r.Equal([]table.Identifier{ + {"accounting", "tax", "paid"}, + {"accounting", "tax", "owed"}, + }, functions) + r.Require().NoError(lastErr) +} + +func (r *RestCatalogSuite) TestListFunctionsPagination() { + namespace := "accounting" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + switch req.URL.Query().Get("pageToken") { + case "": + json.NewEncoder(w).Encode(map[string]any{ + "identifiers": []any{[]string{"accounting", "add_one"}}, + "next-page-token": "token-1", + }) + case "token-1": + json.NewEncoder(w).Encode(map[string]any{ + "identifiers": []any{[]string{"accounting", "add_two"}}, + }) + default: + r.FailNow("unexpected page token") + } + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + functions := make([]table.Identifier, 0) + for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) { + r.Require().NoError(err) + functions = append(functions, function) + } + + r.Equal([]table.Identifier{ + {"accounting", "add_one"}, + {"accounting", "add_two"}, + }, functions) + + // Breaking out of the iteration stops paging without error. + for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) { + r.Require().NoError(err) + r.Equal(table.Identifier{"accounting", "add_one"}, function) + + break + } +} + +func (r *RestCatalogSuite) TestListFunctionsZeroPageSizeNotSent() { + namespace := "accounting" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + r.Equal("", req.URL.Query().Get("pageSize"), "pageSize must not be sent when set to 0") + json.NewEncoder(w).Encode(map[string]any{"identifiers": []any{}}) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + ctx := cat.SetPageSize(context.Background(), 0) + for _, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) { + r.Require().NoError(err) + } +} + +func (r *RestCatalogSuite) TestListFunctionsPaginationErrorOnSubsequentPage() { + namespace := "accounting" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + for k, v := range TestHeaders { + r.Equal(v, req.Header.Values(k)) + } + + pageToken := req.URL.Query().Get("pageToken") + + // First page succeeds + if pageToken == "" { + json.NewEncoder(w).Encode(map[string]any{ + "identifiers": []any{ + []string{namespace, "paid1"}, + []string{namespace, "paid2"}, + }, + "next-page-token": "token1", + }) + + return + } + + // Second page fails with an error + if pageToken == "token1" { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{ + "message": "Token expired or invalid", + "type": "NoSuchPageTokenException", + "code": 404, + }, + }) + + return + } + + r.FailNow("unexpected page token:", pageToken) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + functions := make([]table.Identifier, 0) + var lastErr error + for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) { + if err != nil { + lastErr = err + + break + } + functions = append(functions, function) + } + + // Check that we got the functions from the first page + r.Equal([]table.Identifier{ + {"accounting", "paid1"}, + {"accounting", "paid2"}, + }, functions) + + // Check that we got the error from the second page + r.Error(lastErr) + r.ErrorContains(lastErr, "Token expired or invalid") +} + +func (r *RestCatalogSuite) TestListFunctions404() { + namespace := "nonexistent" + r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + for k, v := range TestHeaders { + r.Equal(v, req.Header.Values(k)) + } + + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{ + "message": "The given namespace does not exist", + "type": "NoSuchNamespaceException", + "code": 404, + }, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + var lastErr error + for _, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) { + if err != nil { + lastErr = err + } + } + r.ErrorIs(lastErr, catalog.ErrNoSuchNamespace) + r.ErrorContains(lastErr, "The given namespace does not exist") +} + +func (r *RestCatalogSuite) TestLoadFunction200() { + r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + for k, v := range TestHeaders { + r.Equal(v, req.Header.Values(k)) + } + + w.Write([]byte(`{ + "metadata-location": "s3://bucket/functions/add_one/metadata/00000-x.metadata.json", + "metadata": ` + testFunctionMetadataJSON + ` + }`)) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + fn, err := cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "add_one")) + r.Require().NoError(err) + + r.Equal(table.Identifier{"accounting", "add_one"}, fn.Identifier()) + r.Equal("s3://bucket/functions/add_one/metadata/00000-x.metadata.json", fn.MetadataLocation()) + r.Equal("42fd3f91-bc10-41c1-8a52-92b57dd0a9b2", fn.Metadata().FunctionUUID().String()) + + def, ok := fn.Metadata().DefinitionByID("int") + r.Require().True(ok, "expected the int overload in the loaded metadata") + r.Equal(1, def.CurrentVersionID) + r.Len(fn.Definitions(), 1) +} + +func (r *RestCatalogSuite) TestLoadFunction404() { + r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method) + + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{ + "message": "The requested function does not exist", + "type": "NoSuchFunctionException", + "code": 404, + }, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "missing_fn")) + r.ErrorIs(err, catalog.ErrNoSuchFunction) + r.ErrorContains(err, "The requested function does not exist") +} + +func (r *RestCatalogSuite) TestLoadFunctionMalformedMetadata() { + r.mux.HandleFunc("/v1/namespaces/accounting/functions/broken_fn", func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte(`{"metadata-location": "s3://x", "metadata": {"format-version": 1}}`)) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "broken_fn")) + r.ErrorContains(err, "failed to parse function metadata") +} + +func (r *RestCatalogSuite) TestCheckFunctionExists() { + r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) { + r.Require().Equal(http.MethodGet, req.Method, "existence must be checked via GET: the spec has no HEAD endpoint") + + w.Write([]byte(`{"metadata": ` + testFunctionMetadataJSON + `}`)) + }) + r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{ + "message": "The requested function does not exist", + "type": "NoSuchFunctionException", + "code": 404, + }, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + exists, err := cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "add_one")) + r.Require().NoError(err) + r.True(exists) + + exists, err = cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "missing_fn")) + r.Require().NoError(err) + r.False(exists) +} + +func (r *RestCatalogSuite) TestLoadFunctionInvalidIdentifier() { + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + // An invalid identifier reports the function sentinel, not the table one. + _, err = cat.LoadFunction(context.Background(), table.Identifier{}) + r.ErrorIs(err, catalog.ErrNoSuchFunction) + r.NotErrorIs(err, catalog.ErrNoSuchTable) + r.ErrorContains(err, "missing namespace or invalid identifier") + + // The existence check surfaces the invalid identifier as an error, as the + // table and view existence checks do. + exists, err := cat.CheckFunctionExists(context.Background(), table.Identifier{}) + r.False(exists) + r.ErrorIs(err, catalog.ErrNoSuchFunction) +} + +func (r *RestCatalogSuite) TestListFunctionsInvalidNamespace() { + cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken)) + r.Require().NoError(err) + + var lastErr error + for _, err := range cat.ListFunctions(context.Background(), table.Identifier{}) { + if err != nil { + lastErr = err + } + } + r.Error(lastErr) Review Comment: `r.Error(lastErr)` passes for any error, including a wrong sentinel — compare `TestListTables404`, which pins `ErrorIs(ErrNoSuchNamespace)`. I'd add an `ErrorIs` here to nail down which error we expect. Same file has a couple of loops (this one, `TestListFunctions404`) that keep iterating after capturing the error instead of breaking — harmless since the iterator stops after an error, but it reads as if multiple errors could arrive. I'd break after the first for clarity while we're here. -- 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]
