diff --git a/catalog/rest/metrics_reporter_test.go b/catalog/rest/metrics_reporter_test.go index b901e7459..7485135b7 100644 --- a/catalog/rest/metrics_reporter_test.go +++ b/catalog/rest/metrics_reporter_test.go @@ -573,8 +573,8 @@ func TestRESTMetricsReporterBuildsPathThroughProduction(t *testing.T) { select { case req := <-received: - // The namespace levels are percent-encoded and joined by the encoded - // separator (%1F); the table segment is escaped by the URL builder. + // Namespace levels and the table name use path encoding; namespace + // levels are joined by the encoded separator (%1F). assert.Equal(t, "/v1/my-prefix/namespaces/a%20b%1Fd%20e/tables/t%20x/metrics", req.escapedPath) case <-time.After(3 * time.Second): t.Fatal("timed out waiting for the async metrics POST") diff --git a/catalog/rest/rest.go b/catalog/rest/rest.go index 2f5af16df..633b2e41c 100644 --- a/catalog/rest/rest.go +++ b/catalog/rest/rest.go @@ -1421,13 +1421,20 @@ func (r *Catalog) nsSeparator() string { return r.namespaceSeparator } +// encodePathSegment escapes a REST path segment per RFC 3986. PathEscape +// leaves plus signs literal, so encode them explicitly to avoid form decoders +// interpreting them as spaces. +func encodePathSegment(value string) string { + return strings.ReplaceAll(url.PathEscape(value), "+", "%2B") +} + // encodeNamespace URL-encodes each namespace level and joins them with the // server-advertised, URL-encoded namespace separator for use as a REST path -// segment. Mirrors RESTUtil.encodeNamespace in the Java implementation. +// segment. func (r *Catalog) encodeNamespace(namespace table.Identifier) string { encoded := make([]string, len(namespace)) for i, level := range namespace { - encoded[i] = url.PathEscape(level) + encoded[i] = encodePathSegment(level) } return strings.Join(encoded, r.nsSeparator()) @@ -1454,7 +1461,7 @@ func (r *Catalog) splitIdentForPath(ident table.Identifier) (string, string, err return "", "", err } - return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.ObjectNameFromIdent(ident), nil + return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil } func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string, string, error) { @@ -1462,7 +1469,7 @@ func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string, string, return "", "", err } - return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.ObjectNameFromIdent(ident), nil + return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil } func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string, string, error) { @@ -1470,7 +1477,7 @@ func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string, str return "", "", err } - return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.ObjectNameFromIdent(ident), nil + return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil } func (r *Catalog) CreateTable(ctx context.Context, identifier table.Identifier, schema *iceberg.Schema, opts ...catalog.CreateTableOpt) (*table.Table, error) { @@ -1478,7 +1485,7 @@ func (r *Catalog) CreateTable(ctx context.Context, identifier table.Identifier, return nil, err } - ns, tbl, err := r.splitIdentForPath(identifier) + ns, _, err := r.splitIdentForPath(identifier) if err != nil { return nil, err } @@ -1505,7 +1512,7 @@ func (r *Catalog) CreateTable(ctx context.Context, identifier table.Identifier, stagedCreate := len(cfg.StagedUpdates) > 0 payload := createTableRequest{ - Name: tbl, + Name: catalog.ObjectNameFromIdent(identifier), Schema: schema, Location: cfg.Location, PartitionSpec: cfg.PartitionSpec, @@ -1600,14 +1607,14 @@ func (r *Catalog) CommitTable(ctx context.Context, ident table.Identifier, requi return nil, "", err } - ns, tblName, err := r.splitIdentForPath(ident) + ns, encodedTbl, err := r.splitIdentForPath(ident) if err != nil { return nil, "", err } restIdentifier := identifier{ Namespace: catalog.NamespaceFromIdent(ident), - Name: tblName, + Name: catalog.ObjectNameFromIdent(ident), } type payload struct { @@ -1616,7 +1623,7 @@ func (r *Catalog) CommitTable(ctx context.Context, ident table.Identifier, requi Updates []table.Update `json:"updates"` } - path, err := endpointUpdateTable.reqPath(ns, tblName) + path, err := endpointUpdateTable.reqPath(ns, encodedTbl) if err != nil { return nil, "", err } @@ -1730,7 +1737,7 @@ func (r *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier return nil, err } - ns, tbl, err := r.splitIdentForPath(identifier) + ns, _, err := r.splitIdentForPath(identifier) if err != nil { return nil, err } @@ -1755,7 +1762,7 @@ func (r *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier } ret, err := doPost[payload, loadTableResponse](ctx, r.baseURI, path, - payload{Name: tbl, MetadataLoc: metadataLoc}, r.cl, map[int]error{ + payload{Name: catalog.ObjectNameFromIdent(identifier), MetadataLoc: metadataLoc}, r.cl, map[int]error{ http.StatusNotFound: catalog.ErrNoSuchNamespace, http.StatusConflict: catalog.ErrTableAlreadyExists, }) if err != nil { @@ -1822,21 +1829,21 @@ func (r *Catalog) UpdateTable(ctx context.Context, ident table.Identifier, requi return nil, err } - ns, tbl, err := r.splitIdentForPath(ident) + ns, encodedTbl, err := r.splitIdentForPath(ident) if err != nil { return nil, err } restIdentifier := identifier{ Namespace: catalog.NamespaceFromIdent(ident), - Name: tbl, + Name: catalog.ObjectNameFromIdent(ident), } type payload struct { Identifier identifier `json:"identifier"` Requirements []table.Requirement `json:"requirements"` Updates []table.Update `json:"updates"` } - path, err := endpointUpdateTable.reqPath(ns, tbl) + path, err := endpointUpdateTable.reqPath(ns, encodedTbl) if err != nil { return nil, err } diff --git a/catalog/rest/rest_internal_test.go b/catalog/rest/rest_internal_test.go index 880fa1e65..7b4534270 100644 --- a/catalog/rest/rest_internal_test.go +++ b/catalog/rest/rest_internal_test.go @@ -81,6 +81,43 @@ func TestSplitIdentForPathRequiresNamespaceAndName(t *testing.T) { require.NoError(t, err) assert.Equal(t, "parent%1Fnamespace", ns) assert.Equal(t, "table", tbl) + + ns, tbl, err = cat.splitIdentForPath(table.Identifier{"namespace+name", "table+name"}) + require.NoError(t, err) + assert.Equal(t, "namespace%2Bname", ns) + assert.Equal(t, "table%2Bname", tbl) + + for name, split := range map[string]func(table.Identifier) (string, string, error){ + "view": cat.splitViewIdentForPath, + "function": cat.splitFunctionIdentForPath, + } { + t.Run(name, func(t *testing.T) { + ns, object, err := split(table.Identifier{"namespace+name", name + "+name"}) + require.NoError(t, err) + assert.Equal(t, "namespace%2Bname", ns) + assert.Equal(t, name+"%2Bname", object) + }) + } +} + +func TestEncodePathSegment(t *testing.T) { + tests := []struct { + name string + value string + want string + }{ + {name: "space", value: " ", want: "%20"}, + {name: "plus", value: "+", want: "%2B"}, + {name: "percent", value: "%", want: "%25"}, + {name: "slash", value: "/", want: "%2F"}, + {name: "unicode", value: "£€", want: "%C2%A3%E2%82%AC"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, encodePathSegment(tt.value)) + }) + } } func TestLoadRegisteredCatalogRejectsInvalidAuthURL(t *testing.T) {