This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 5dce5dee3 refactor(rest): Centralize identifier path encoding (#2104)
5dce5dee3 is described below
commit 5dce5dee3426a89df2bb9af4dcf9fc312afd6ae0
Author: Alessandro Nori <[email protected]>
AuthorDate: Mon Oct 19 17:16:07 2026 +0200
refactor(rest): Centralize identifier path encoding (#2104)
* Refactor REST identifier path handling
* fix(rest): Encode plus in scan plan IDs
* fix(rest): Preserve dot path segments
* test(rest): Cover path encoding review cases
* refactor(rest): Encapsulate identifier request path construction
* chore(rest): Clarify identifier path contracts
---
catalog/rest/metrics_reporter_test.go | 12 +-
catalog/rest/rest.go | 163 ++++++++++++++++----------
catalog/rest/rest_internal_test.go | 208 +++++++++++++++++++++++++++++++---
catalog/rest/rest_test.go | 4 +
catalog/rest/scan_planning.go | 34 +-----
catalog/rest/scan_planning_test.go | 47 +++++---
6 files changed, 339 insertions(+), 129 deletions(-)
diff --git a/catalog/rest/metrics_reporter_test.go
b/catalog/rest/metrics_reporter_test.go
index 3814fb639..9d9122976 100644
--- a/catalog/rest/metrics_reporter_test.go
+++ b/catalog/rest/metrics_reporter_test.go
@@ -554,15 +554,15 @@ func TestMetricsDispatcherCloseIsIdempotent(t *testing.T)
{
}
// TestRESTMetricsReporterBuildsPathThroughProduction runs the real path
-// composition — splitIdentForPath (encodeNamespace) and endpointReportMetrics
-// .reqPath — for a multi-level namespace with segments needing escaping, so a
-// regression in that composition or in the separator handling is caught rather
-// than bypassed by a hand-built path.
+// composition — tableIdentifierPath and identifierPath.objectReqPath — for a
+// multi-level namespace with segments needing escaping, so a regression in
that
+// composition or in the separator handling is caught rather than bypassed by a
+// hand-built path.
func TestRESTMetricsReporterBuildsPathThroughProduction(t *testing.T) {
c := &Catalog{namespaceSeparator: defaultNamespaceSeparator}
- ns, tbl, err := c.splitIdentForPath(table.Identifier{"a b", "d e", "t
x"})
+ identPath, err := c.tableIdentifierPath(table.Identifier{"a b", "d e",
"t x"})
require.NoError(t, err)
- path, err := endpointReportMetrics.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointReportMetrics)
require.NoError(t, err)
received := make(chan capturedRequest, 1)
diff --git a/catalog/rest/rest.go b/catalog/rest/rest.go
index 62024c49d..d0f727b92 100644
--- a/catalog/rest/rest.go
+++ b/catalog/rest/rest.go
@@ -1322,13 +1322,13 @@ func (r *Catalog) tableFromResponse(
// catalog-owned dispatcher. It is non-nil only when the client enabled
// reporting and the server advertises the endpoint (see init).
if r.metricsDispatcher != nil {
- if ns, tbl, idErr := r.splitIdentForPath(identifier); idErr !=
nil {
+ if identPath, idErr := r.tableIdentifierPath(identifier); idErr
!= nil {
// Unreachable for a valid identifier that already
loaded above, but if it
// ever regresses, log why the REST reporter was
dropped rather than let
// metrics silently vanish with no signal.
- slog.Debug("iceberg: skipping REST metrics reporter,
cannot split identifier",
+ slog.Debug("iceberg: skipping REST metrics reporter,
cannot build identifier path",
"error", idErr)
- } else if path, pErr := endpointReportMetrics.reqPath(ns, tbl);
pErr != nil {
+ } else if path, pErr :=
identPath.objectReqPath(endpointReportMetrics); pErr != nil {
slog.Debug("iceberg: skipping REST metrics reporter,
cannot build metrics path",
"error", pErr)
} else {
@@ -1362,12 +1362,12 @@ func (r *Catalog) fetchTableCreds(ctx context.Context,
ident []string, location
return nil, err
}
- ns, tbl, err := r.splitIdentForPath(ident)
+ identPath, err := r.tableIdentifierPath(ident)
if err != nil {
return nil, err
}
- path, err := endpointTableCredentials.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointTableCredentials)
if err != nil {
return nil, err
}
@@ -1511,10 +1511,18 @@ func (r *Catalog) nsSeparator() string {
}
// 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.
+// leaves plus signs and pure-dot segments literal, so encode them explicitly
to
+// avoid form decoders interpreting plus signs as spaces and JoinPath resolving
+// dot segments while cleaning the path.
func encodePathSegment(value string) string {
- return strings.ReplaceAll(url.PathEscape(value), "+", "%2B")
+ switch value {
+ case ".":
+ return "%2E"
+ case "..":
+ return "%2E%2E"
+ default:
+ return strings.ReplaceAll(url.PathEscape(value), "+", "%2B")
+ }
}
// encodeNamespace URL-encodes each namespace level and joins them with the
@@ -1545,28 +1553,67 @@ func (r *Catalog) namespaceToQueryParam(namespace
table.Identifier) string {
return strings.Join(namespace, r.decodedNamespaceSeparator())
}
-func (r *Catalog) splitIdentForPath(ident table.Identifier) (string, string,
error) {
+// identifierPath keeps rawName for JSON request bodies separate from
+// encodedNamespace and encodedName for URL paths. The validated table, view,
and
+// function path builders return a zero value only alongside a non-nil error.
+type identifierPath struct {
+ encodedNamespace string
+ rawName string
+ encodedName string
+}
+
+// namespaceReqPath builds a namespace-scoped path, such as a create or
register
+// endpoint, without including the object name.
+func (p identifierPath) namespaceReqPath(ep endpoint) ([]string, error) {
+ return ep.reqPath(p.encodedNamespace)
+}
+
+// objectReqPath builds an object-scoped path using the encoded identifier
components.
+// rawExtra contains raw opaque path segments, such as scan plan IDs. Encode
each
+// segment here so slashes, plus signs, and pure-dot segments remain literal.
+func (p identifierPath) objectReqPath(ep endpoint, rawExtra ...string)
([]string, error) {
+ params := make([]string, 0, 2+len(rawExtra))
+ params = append(params, p.encodedNamespace, p.encodedName)
+ for _, seg := range rawExtra {
+ params = append(params, encodePathSegment(seg))
+ }
+
+ return ep.reqPath(params...)
+}
+
+// newIdentifierPath builds path components from an already validated
identifier.
+func (r *Catalog) newIdentifierPath(ident table.Identifier) identifierPath {
+ rawName := catalog.ObjectNameFromIdent(ident)
+
+ return identifierPath{
+ encodedNamespace:
r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
+ rawName: rawName,
+ encodedName: encodePathSegment(rawName),
+ }
+}
+
+func (r *Catalog) tableIdentifierPath(ident table.Identifier) (identifierPath,
error) {
if err := catalog.ValidateTableIdentifier(ident); err != nil {
- return "", "", err
+ return identifierPath{}, err
}
- return r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil
+ return r.newIdentifierPath(ident), nil
}
-func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string,
string, error) {
+func (r *Catalog) viewIdentifierPath(ident table.Identifier) (identifierPath,
error) {
if err := catalog.ValidateViewIdentifier(ident); err != nil {
- return "", "", err
+ return identifierPath{}, err
}
- return r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil
+ return r.newIdentifierPath(ident), nil
}
-func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string,
string, error) {
+func (r *Catalog) functionIdentifierPath(ident table.Identifier)
(identifierPath, error) {
if err := catalog.ValidateFunctionIdentifier(ident); err != nil {
- return "", "", err
+ return identifierPath{}, err
}
- return r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil
+ return r.newIdentifierPath(ident), nil
}
func (r *Catalog) CreateTable(ctx context.Context, identifier
table.Identifier, schema *iceberg.Schema, opts ...catalog.CreateTableOpt)
(*table.Table, error) {
@@ -1574,7 +1621,7 @@ func (r *Catalog) CreateTable(ctx context.Context,
identifier table.Identifier,
return nil, err
}
- ns, _, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return nil, err
}
@@ -1601,7 +1648,7 @@ func (r *Catalog) CreateTable(ctx context.Context,
identifier table.Identifier,
stagedCreate := len(cfg.StagedUpdates) > 0
payload := createTableRequest{
- Name: catalog.ObjectNameFromIdent(identifier),
+ Name: identPath.rawName,
Schema: schema,
Location: cfg.Location,
PartitionSpec: cfg.PartitionSpec,
@@ -1610,7 +1657,7 @@ func (r *Catalog) CreateTable(ctx context.Context,
identifier table.Identifier,
Props: cfg.Properties,
}
- path, err := endpointCreateTable.reqPath(ns)
+ path, err := identPath.namespaceReqPath(endpointCreateTable)
if err != nil {
return nil, err
}
@@ -1700,14 +1747,14 @@ func (r *Catalog) CommitTable(ctx context.Context,
ident table.Identifier, requi
return nil, "", err
}
- ns, encodedTbl, err := r.splitIdentForPath(ident)
+ identPath, err := r.tableIdentifierPath(ident)
if err != nil {
return nil, "", err
}
restIdentifier := identifier{
Namespace: catalog.NamespaceFromIdent(ident),
- Name: catalog.ObjectNameFromIdent(ident),
+ Name: identPath.rawName,
}
type payload struct {
@@ -1716,7 +1763,7 @@ func (r *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, requi
Updates []table.Update `json:"updates"`
}
- path, err := endpointUpdateTable.reqPath(ns, encodedTbl)
+ path, err := identPath.objectReqPath(endpointUpdateTable)
if err != nil {
return nil, "", err
}
@@ -1830,7 +1877,7 @@ func (r *Catalog) RegisterTable(ctx context.Context,
identifier table.Identifier
return nil, err
}
- ns, _, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return nil, err
}
@@ -1849,13 +1896,13 @@ func (r *Catalog) RegisterTable(ctx context.Context,
identifier table.Identifier
return nil, fmt.Errorf("failed to initialize metrics reporter:
%w", err)
}
- path, err := endpointRegisterTable.reqPath(ns)
+ path, err := identPath.namespaceReqPath(endpointRegisterTable)
if err != nil {
return nil, err
}
ret, err := doPost[payload, loadTableResponse](ctx, r.baseURI, path,
- payload{Name: catalog.ObjectNameFromIdent(identifier),
MetadataLoc: metadataLoc}, r.cl, map[int]error{
+ payload{Name: identPath.rawName, MetadataLoc: metadataLoc},
r.cl, map[int]error{
http.StatusNotFound: catalog.ErrNoSuchNamespace,
http.StatusConflict: catalog.ErrTableAlreadyExists,
})
if err != nil {
@@ -1889,12 +1936,12 @@ func (r *Catalog) loadTableWithMode(ctx
context.Context, identifier table.Identi
return nil, err
}
- ns, tbl, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return nil, err
}
- path, err := endpointLoadTable.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointLoadTable)
if err != nil {
return nil, err
}
@@ -1929,21 +1976,21 @@ func (r *Catalog) UpdateTable(ctx context.Context,
ident table.Identifier, requi
return nil, err
}
- ns, encodedTbl, err := r.splitIdentForPath(ident)
+ identPath, err := r.tableIdentifierPath(ident)
if err != nil {
return nil, err
}
restIdentifier := identifier{
Namespace: catalog.NamespaceFromIdent(ident),
- Name: catalog.ObjectNameFromIdent(ident),
+ Name: identPath.rawName,
}
type payload struct {
Identifier identifier `json:"identifier"`
Requirements []table.Requirement `json:"requirements"`
Updates []table.Update `json:"updates"`
}
- path, err := endpointUpdateTable.reqPath(ns, encodedTbl)
+ path, err := identPath.objectReqPath(endpointUpdateTable)
if err != nil {
return nil, err
}
@@ -1978,12 +2025,12 @@ func (r *Catalog) DropTable(ctx context.Context,
identifier table.Identifier) er
return err
}
- ns, tbl, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return err
}
- path, err := endpointDeleteTable.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointDeleteTable)
if err != nil {
return err
}
@@ -2004,12 +2051,12 @@ func (r *Catalog) PurgeTable(ctx context.Context,
identifier table.Identifier) e
return err
}
- ns, tbl, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return err
}
- path, err := endpointDeleteTable.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointDeleteTable)
if err != nil {
return err
}
@@ -2272,7 +2319,7 @@ func (r *Catalog) CheckNamespaceExists(ctx
context.Context, namespace table.Iden
}
func (r *Catalog) CheckTableExists(ctx context.Context, identifier
table.Identifier) (bool, error) {
- ns, tbl, err := r.splitIdentForPath(identifier)
+ identPath, err := r.tableIdentifierPath(identifier)
if err != nil {
return false, err
}
@@ -2292,7 +2339,7 @@ func (r *Catalog) CheckTableExists(ctx context.Context,
identifier table.Identif
return true, nil
}
- path, err := endpointTableExists.reqPath(ns, tbl)
+ path, err := identPath.objectReqPath(endpointTableExists)
if err != nil {
return false, err
}
@@ -2385,12 +2432,12 @@ func (r *Catalog) DropView(ctx context.Context,
identifier table.Identifier) err
return err
}
- ns, view, err := r.splitViewIdentForPath(identifier)
+ identPath, err := r.viewIdentifierPath(identifier)
if err != nil {
return err
}
- path, err := endpointDeleteView.reqPath(ns, view)
+ path, err := identPath.objectReqPath(endpointDeleteView)
if err != nil {
return err
}
@@ -2402,7 +2449,7 @@ func (r *Catalog) DropView(ctx context.Context,
identifier table.Identifier) err
}
func (r *Catalog) CheckViewExists(ctx context.Context, identifier
table.Identifier) (bool, error) {
- ns, view, err := r.splitViewIdentForPath(identifier)
+ identPath, err := r.viewIdentifierPath(identifier)
if err != nil {
return false, err
}
@@ -2422,7 +2469,7 @@ func (r *Catalog) CheckViewExists(ctx context.Context,
identifier table.Identifi
return true, nil
}
- path, err := endpointViewExists.reqPath(ns, view)
+ path, err := identPath.objectReqPath(endpointViewExists)
if err != nil {
return false, err
}
@@ -2476,7 +2523,7 @@ func (r *Catalog) CreateView(ctx context.Context,
identifier table.Identifier, v
return nil, fmt.Errorf("%w: view version cannot be nil",
iceberg.ErrInvalidArgument)
}
- ns, _, err := r.splitViewIdentForPath(identifier)
+ identPath, err := r.viewIdentifierPath(identifier)
if err != nil {
return nil, err
}
@@ -2507,14 +2554,14 @@ func (r *Catalog) CreateView(ctx context.Context,
identifier table.Identifier, v
}
payload := createViewRequest{
- Name: catalog.ObjectNameFromIdent(identifier),
+ Name: identPath.rawName,
Location: cfg.Location,
Schema: freshSchema,
Props: cfg.Properties,
ViewVersion: version,
}
- path, err := endpointCreateView.reqPath(ns)
+ path, err := identPath.namespaceReqPath(endpointCreateView)
if err != nil {
return nil, err
}
@@ -2537,21 +2584,21 @@ func (r *Catalog) UpdateView(ctx context.Context, ident
table.Identifier, requir
return nil, err
}
- ns, encodedView, err := r.splitViewIdentForPath(ident)
+ identPath, err := r.viewIdentifierPath(ident)
if err != nil {
return nil, err
}
restIdentifier := identifier{
Namespace: catalog.NamespaceFromIdent(ident),
- Name: catalog.ObjectNameFromIdent(ident),
+ Name: identPath.rawName,
}
type payload struct {
Identifier identifier `json:"identifier"`
Requirements []view.Requirement `json:"requirements"`
Updates []view.Update `json:"updates"`
}
- path, err := endpointUpdateView.reqPath(ns, encodedView)
+ path, err := identPath.objectReqPath(endpointUpdateView)
if err != nil {
return nil, err
}
@@ -2583,7 +2630,7 @@ func (r *Catalog) RegisterView(ctx context.Context,
identifier table.Identifier,
return nil, err
}
- ns, _, err := r.splitViewIdentForPath(identifier)
+ identPath, err := r.viewIdentifierPath(identifier)
if err != nil {
return nil, err
}
@@ -2593,13 +2640,13 @@ func (r *Catalog) RegisterView(ctx context.Context,
identifier table.Identifier,
MetadataLoc string `json:"metadata-location"`
}
- path, err := endpointRegisterView.reqPath(ns)
+ path, err := identPath.namespaceReqPath(endpointRegisterView)
if err != nil {
return nil, err
}
rsp, err := doPost[payload, loadViewResponse](ctx, r.baseURI, path,
- payload{Name: catalog.ObjectNameFromIdent(identifier),
MetadataLoc: metadataLoc}, r.cl, map[int]error{
+ payload{Name: identPath.rawName, MetadataLoc: metadataLoc},
r.cl, map[int]error{
http.StatusNotFound: catalog.ErrNoSuchNamespace,
http.StatusConflict: catalog.ErrViewAlreadyExists,
})
if err != nil {
@@ -2620,12 +2667,12 @@ func (r *Catalog) LoadView(ctx context.Context,
identifier table.Identifier) (*v
return nil, err
}
- ns, v, err := r.splitViewIdentForPath(identifier)
+ identPath, err := r.viewIdentifierPath(identifier)
if err != nil {
return nil, err
}
- path, err := endpointLoadView.reqPath(ns, v)
+ path, err := identPath.objectReqPath(endpointLoadView)
if err != nil {
return nil, err
}
@@ -2793,18 +2840,18 @@ func (r *Catalog) LoadFunction(ctx context.Context,
identifier table.Identifier)
return nil, err
}
- ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ identPath, err := r.functionIdentifierPath(identifier)
if err != nil {
return nil, err
}
- return r.loadFunction(ctx, identifier, ns, fn)
+ return r.loadFunction(ctx, identifier, identPath)
}
// loadFunction fetches and parses a function after endpoint negotiation and
// identifier validation, so its ErrNoSuchFunction is always server-reported.
-func (r *Catalog) loadFunction(ctx context.Context, identifier
table.Identifier, ns, fn string) (*udf.UDF, error) {
- path, err := endpointLoadFunction.reqPath(ns, fn)
+func (r *Catalog) loadFunction(ctx context.Context, identifier
table.Identifier, identPath identifierPath) (*udf.UDF, error) {
+ path, err := identPath.objectReqPath(endpointLoadFunction)
if err != nil {
return nil, err
}
@@ -2852,12 +2899,12 @@ func (r *Catalog) CheckFunctionExists(ctx
context.Context, identifier table.Iden
return false, err
}
- ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ identPath, err := r.functionIdentifierPath(identifier)
if err != nil {
return false, err
}
- if _, err := r.loadFunction(ctx, identifier, ns, fn); err != nil {
+ if _, err := r.loadFunction(ctx, identifier, identPath); err != nil {
if errors.Is(err, catalog.ErrNoSuchFunction) {
return false, nil
}
diff --git a/catalog/rest/rest_internal_test.go
b/catalog/rest/rest_internal_test.go
index 057f3e565..a95ebff7f 100644
--- a/catalog/rest/rest_internal_test.go
+++ b/catalog/rest/rest_internal_test.go
@@ -464,7 +464,7 @@ func TestHeaderDefaultsReachSeparateTokenEndpoint(t
*testing.T) {
assert.Equal(t, "SECRET-API-KEY", tokenAPIKey, "the configured token
endpoint must still receive header.* defaults")
}
-func TestSplitIdentForPathRequiresNamespaceAndName(t *testing.T) {
+func TestIdentifierPathsRequireNamespaceAndName(t *testing.T) {
cat := &Catalog{}
for _, ident := range []table.Identifier{
@@ -476,38 +476,166 @@ func TestSplitIdentForPathRequiresNamespaceAndName(t
*testing.T) {
{"namespace", "table/name"},
{"namespace", "table\nname"},
} {
- _, _, err := cat.splitIdentForPath(ident)
+ _, err := cat.tableIdentifierPath(ident)
require.ErrorIs(t, err, catalog.ErrNoSuchTable)
}
- _, _, err := cat.splitViewIdentForPath(table.Identifier{"view"})
+ _, err := cat.viewIdentifierPath(table.Identifier{"view"})
require.ErrorIs(t, err, catalog.ErrNoSuchView)
require.NotErrorIs(t, err, catalog.ErrNoSuchTable)
- ns, tbl, err := cat.splitIdentForPath(table.Identifier{"namespace",
"table"})
+ identPath, err := cat.tableIdentifierPath(table.Identifier{"namespace",
"table"})
require.NoError(t, err)
- assert.Equal(t, "namespace", ns)
- assert.Equal(t, "table", tbl)
+ assert.Equal(t, "namespace", identPath.encodedNamespace)
+ assert.Equal(t, "table", identPath.rawName)
+ assert.Equal(t, "table", identPath.encodedName)
- ns, tbl, err = cat.splitIdentForPath(table.Identifier{"parent",
"namespace", "table"})
+ identPath, err = cat.tableIdentifierPath(table.Identifier{"parent",
"namespace", "table"})
require.NoError(t, err)
- assert.Equal(t, "parent%1Fnamespace", ns)
- assert.Equal(t, "table", tbl)
+ assert.Equal(t, "parent%1Fnamespace", identPath.encodedNamespace)
+ assert.Equal(t, "table", identPath.rawName)
+ assert.Equal(t, "table", identPath.encodedName)
- ns, tbl, err = cat.splitIdentForPath(table.Identifier{"namespace+name",
"table+name"})
+ identPath, err =
cat.tableIdentifierPath(table.Identifier{"namespace+name", "table+name"})
require.NoError(t, err)
- assert.Equal(t, "namespace%2Bname", ns)
- assert.Equal(t, "table%2Bname", tbl)
+ assert.Equal(t, "namespace%2Bname", identPath.encodedNamespace)
+ assert.Equal(t, "table+name", identPath.rawName)
+ assert.Equal(t, "table%2Bname", identPath.encodedName)
- for name, split := range map[string]func(table.Identifier) (string,
string, error){
- "view": cat.splitViewIdentForPath,
- "function": cat.splitFunctionIdentForPath,
+ for name, buildPath := range map[string]func(table.Identifier)
(identifierPath, error){
+ "view": cat.viewIdentifierPath,
+ "function": cat.functionIdentifierPath,
} {
t.Run(name, func(t *testing.T) {
- ns, object, err :=
split(table.Identifier{"namespace+name", name + "+name"})
+ identPath, err :=
buildPath(table.Identifier{"namespace+name", name + "+name"})
require.NoError(t, err)
- assert.Equal(t, "namespace%2Bname", ns)
- assert.Equal(t, name+"%2Bname", object)
+ assert.Equal(t, "namespace%2Bname",
identPath.encodedNamespace)
+ assert.Equal(t, name+"+name", identPath.rawName)
+ assert.Equal(t, name+"%2Bname", identPath.encodedName)
+ })
+ }
+}
+
+func TestIdentifierPathsRejectDotComponents(t *testing.T) {
+ cat := &Catalog{}
+
+ for _, builder := range []struct {
+ name string
+ buildPath func(table.Identifier) (identifierPath, error)
+ wantErr error
+ }{
+ {name: "table", buildPath: cat.tableIdentifierPath, wantErr:
catalog.ErrNoSuchTable},
+ {name: "view", buildPath: cat.viewIdentifierPath, wantErr:
catalog.ErrNoSuchView},
+ {name: "function", buildPath: cat.functionIdentifierPath,
wantErr: catalog.ErrNoSuchFunction},
+ } {
+ t.Run(builder.name, func(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ ident table.Identifier
+ }{
+ {name: "dot namespace", ident:
table.Identifier{".", "object"}},
+ {name: "double dot namespace", ident:
table.Identifier{"..", "object"}},
+ {name: "dot name", ident:
table.Identifier{"namespace", "."}},
+ {name: "double dot name", ident:
table.Identifier{"namespace", ".."}},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ path, err := builder.buildPath(tt.ident)
+ require.ErrorIs(t, err, builder.wantErr)
+ assert.Equal(t, identifierPath{}, path)
+ })
+ }
+ })
+ }
+}
+
+func TestIdentifierPathObjectReqPath(t *testing.T) {
+ cat := &Catalog{}
+ identPath, err := cat.tableIdentifierPath(table.Identifier{"a b+c", "d
e+f", "t x+y%2F"})
+ require.NoError(t, err)
+ baseURI := mustParseURL(t, "https://catalog.example/base/v1/prefix";)
+
+ for _, tt := range []struct {
+ name string
+ ep endpoint
+ extra []string
+ wantSuffix string
+ }{
+ {name: "load table", ep: endpointLoadTable, wantSuffix:
"/tables/t%20x%2By%252F"},
+ {name: "update table", ep: endpointUpdateTable, wantSuffix:
"/tables/t%20x%2By%252F"},
+ {name: "delete table", ep: endpointDeleteTable, wantSuffix:
"/tables/t%20x%2By%252F"},
+ {name: "table exists", ep: endpointTableExists, wantSuffix:
"/tables/t%20x%2By%252F"},
+ {name: "credentials", ep: endpointTableCredentials, wantSuffix:
"/tables/t%20x%2By%252F/credentials"},
+ {name: "metrics", ep: endpointReportMetrics, wantSuffix:
"/tables/t%20x%2By%252F/metrics"},
+ {name: "plan scan", ep: endpointPlanTableScan, wantSuffix:
"/tables/t%20x%2By%252F/plan"},
+ {name: "fetch tasks", ep: endpointFetchScanTasks, wantSuffix:
"/tables/t%20x%2By%252F/tasks"},
+ {name: "fetch plan", ep: endpointFetchPlanResult, extra:
[]string{"a+b/c%2F"}, wantSuffix: "/tables/t%20x%2By%252F/plan/a%2Bb%2Fc%252F"},
+ {name: "cancel plan", ep: endpointCancelPlanning, extra:
[]string{".."}, wantSuffix: "/tables/t%20x%2By%252F/plan/%2E%2E"},
+ {name: "load view", ep: endpointLoadView, wantSuffix:
"/views/t%20x%2By%252F"},
+ {name: "update view", ep: endpointUpdateView, wantSuffix:
"/views/t%20x%2By%252F"},
+ {name: "delete view", ep: endpointDeleteView, wantSuffix:
"/views/t%20x%2By%252F"},
+ {name: "view exists", ep: endpointViewExists, wantSuffix:
"/views/t%20x%2By%252F"},
+ {name: "load function", ep: endpointLoadFunction, wantSuffix:
"/functions/t%20x%2By%252F"},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ path, err := identPath.objectReqPath(tt.ep, tt.extra...)
+ require.NoError(t, err)
+ assert.Equal(t,
"/base/v1/prefix/namespaces/a%20b%2Bc%1Fd%20e%2Bf"+tt.wantSuffix,
+ baseURI.JoinPath(path...).EscapedPath())
+ })
+ }
+}
+
+func TestIdentifierPathNamespaceReqPath(t *testing.T) {
+ cat := &Catalog{}
+ identPath, err := cat.tableIdentifierPath(table.Identifier{"a b+c", "d
e+f", "t x+y"})
+ require.NoError(t, err)
+ baseURI := mustParseURL(t, "https://catalog.example/base/v1/prefix";)
+
+ for _, tt := range []struct {
+ name string
+ ep endpoint
+ wantSuffix string
+ }{
+ {name: "create table", ep: endpointCreateTable, wantSuffix:
"/tables"},
+ {name: "register table", ep: endpointRegisterTable, wantSuffix:
"/register"},
+ {name: "create view", ep: endpointCreateView, wantSuffix:
"/views"},
+ {name: "register view", ep: endpointRegisterView, wantSuffix:
"/register-view"},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ path, err := identPath.namespaceReqPath(tt.ep)
+ require.NoError(t, err)
+ assert.Equal(t,
"/base/v1/prefix/namespaces/a%20b%2Bc%1Fd%20e%2Bf"+tt.wantSuffix,
+ baseURI.JoinPath(path...).EscapedPath())
+ })
+ }
+}
+
+func TestIdentifierPathMethodsRejectWrongParameterCount(t *testing.T) {
+ cat := &Catalog{}
+ identPath, err := cat.tableIdentifierPath(table.Identifier{"namespace",
"table"})
+ require.NoError(t, err)
+
+ for _, tt := range []struct {
+ name string
+ buildPath func() ([]string, error)
+ }{
+ {name: "object method on namespace endpoint", buildPath: func()
([]string, error) {
+ return identPath.objectReqPath(endpointCreateTable)
+ }},
+ {name: "namespace method on object endpoint", buildPath: func()
([]string, error) {
+ return identPath.namespaceReqPath(endpointLoadTable)
+ }},
+ {name: "missing plan id", buildPath: func() ([]string, error) {
+ return identPath.objectReqPath(endpointFetchPlanResult)
+ }},
+ {name: "unexpected extra segment", buildPath: func() ([]string,
error) {
+ return identPath.objectReqPath(endpointLoadTable,
"extra")
+ }},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ path, err := tt.buildPath()
+ require.ErrorIs(t, err, ErrRESTError)
+ assert.Nil(t, path)
})
}
}
@@ -522,6 +650,9 @@ func TestEncodePathSegment(t *testing.T) {
{name: "plus", value: "+", want: "%2B"},
{name: "percent", value: "%", want: "%25"},
{name: "slash", value: "/", want: "%2F"},
+ {name: "dot", value: ".", want: "%2E"},
+ {name: "double dot", value: "..", want: "%2E%2E"},
+ {name: "dots in name", value: "a..b", want: "a..b"},
{name: "unicode", value: "£€", want: "%C2%A3%E2%82%AC"},
}
@@ -2172,6 +2303,47 @@ func TestCheckValidNamespaceRejectsDecodedSeparator(t
*testing.T) {
}
}
+func TestLoadNamespacePropertiesPreservesDotSegments(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ namespace table.Identifier
+ wantPath string
+ }{
+ {name: "dot", namespace: table.Identifier{"."}, wantPath:
"/v1/namespaces/%2E"},
+ {name: "double dot", namespace: table.Identifier{".."},
wantPath: "/v1/namespaces/%2E%2E"},
+ {name: "nested dots", namespace: table.Identifier{"..", "."},
wantPath: "/v1/namespaces/%2E%2E%1F%2E"},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ mux := http.NewServeMux()
+ mux.HandleFunc("/v1/config", func(w
http.ResponseWriter, r *http.Request) {
+ assert.NoError(t,
json.NewEncoder(w).Encode(map[string]any{
+ "defaults": map[string]any{},
"overrides": map[string]any{},
+ }))
+ })
+ mux.HandleFunc("/v1/namespaces/", func(w
http.ResponseWriter, r *http.Request) {
+ if !assert.Equal(t, http.MethodGet, r.Method)
|| !assert.Equal(t, tt.wantPath, r.URL.EscapedPath()) {
+ return
+ }
+ assert.NoError(t,
json.NewEncoder(w).Encode(map[string]any{
+ "namespace": tt.namespace,
+ "properties":
iceberg.Properties{"owner": "data-team"},
+ }))
+ })
+
+ srv := httptest.NewServer(mux)
+ t.Cleanup(srv.Close)
+
+ cat, err := NewCatalog(context.Background(), "rest",
srv.URL)
+ require.NoError(t, err)
+ t.Cleanup(func() { assert.NoError(t, cat.Close()) })
+
+ props, err :=
cat.LoadNamespaceProperties(context.Background(), tt.namespace)
+ require.NoError(t, err)
+ assert.Equal(t, iceberg.Properties{"owner":
"data-team"}, props)
+ })
+ }
+}
+
func TestNamespaceSeparatorFromConfig(t *testing.T) {
mux := http.NewServeMux()
srv := httptest.NewServer(mux)
diff --git a/catalog/rest/rest_test.go b/catalog/rest/rest_test.go
index 4af1ad224..d04c958de 100644
--- a/catalog/rest/rest_test.go
+++ b/catalog/rest/rest_test.go
@@ -3231,6 +3231,8 @@ func (r *RestCatalogSuite)
TestRequestPathsEncodeNamesAndBodiesRemainRaw() {
}
r.mux.HandleFunc("/v1/namespaces/create-ns/tables", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal("/v1/namespaces/create-ns/tables",
req.URL.EscapedPath())
+
var payload namedPayload
if !decodePayload(req, &payload) {
return
@@ -3240,6 +3242,8 @@ func (r *RestCatalogSuite)
TestRequestPathsEncodeNamesAndBodiesRemainRaw() {
})
r.mux.HandleFunc("/v1/namespaces/register-ns/register", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal("/v1/namespaces/register-ns/register",
req.URL.EscapedPath())
+
var payload namedPayload
if !decodePayload(req, &payload) {
return
diff --git a/catalog/rest/scan_planning.go b/catalog/rest/scan_planning.go
index 3ae6b6d70..920d9ce01 100644
--- a/catalog/rest/scan_planning.go
+++ b/catalog/rest/scan_planning.go
@@ -29,7 +29,6 @@ import (
"maps"
"math/rand/v2"
"net/http"
- "net/url"
"runtime"
"slices"
"strconv"
@@ -1033,40 +1032,13 @@ func (r *Catalog) abandonPlan(ctx context.Context,
ident table.Identifier, planI
_ = r.CancelPlanning(cctx, ident, planID)
}
-func (r *Catalog) scanPlanningPath(ep endpoint, ident table.Identifier, extra
...string) ([]string, error) {
- ns, tbl, err := r.splitIdentForPath(ident)
+func (r *Catalog) scanPlanningPath(ep endpoint, ident table.Identifier,
rawExtra ...string) ([]string, error) {
+ identPath, err := r.tableIdentifierPath(ident)
if err != nil {
return nil, err
}
- // extra carries opaque, server-issued path segments (the plan-id). The
spec
- // puts no character restriction on them, and baseURI.JoinPath
path.Cleans its
- // arguments — so an unescaped id containing '/' would split into
segments and
- // a '..'/'.' segment would resolve to a different endpoint (e.g. ".."
on the
- // plan path lands on .../tasks). Escape each as a single literal path
segment.
- params := make([]string, 0, 2+len(extra))
- params = append(params, ns, tbl)
- for _, seg := range extra {
- params = append(params, escapeOpaquePathSegment(seg))
- }
-
- return ep.reqPath(params...)
-}
-
-// escapeOpaquePathSegment percent-encodes an opaque string so it survives
-// url.URL.JoinPath as a single literal path segment. url.PathEscape handles
'/',
-// '%', spaces, etc., but leaves a pure-dot segment ("." or "..") unescaped,
which
-// JoinPath's path.Clean would then resolve as a dot-segment; encode those
dots so
-// the segment is preserved verbatim.
-func escapeOpaquePathSegment(s string) string {
- switch escaped := url.PathEscape(s); escaped {
- case ".":
- return "%2E"
- case "..":
- return "%2E%2E"
- default:
- return escaped
- }
+ return identPath.objectReqPath(ep, rawExtra...)
}
func scanPlanningHeaders(idempotencyKey, accessDelegation *string,
includeIdempotency bool) (map[string]string, error) {
diff --git a/catalog/rest/scan_planning_test.go
b/catalog/rest/scan_planning_test.go
index 30eb2bc49..498050a4d 100644
--- a/catalog/rest/scan_planning_test.go
+++ b/catalog/rest/scan_planning_test.go
@@ -96,22 +96,37 @@ func TestScanPlanningEscapesOpaquePlanID(t *testing.T) {
// literal segment; otherwise "a/b" splits and ".." / "../tasks"
resolve to a
// different endpoint. Each must survive JoinPath as the final
.../plan/ segment
// and round-trip.
- for _, planID := range []string{"a/b", "../tasks", "a b", "a%2Fb", ".",
"..", "plan-123"} {
- path, err := cat.scanPlanningPath(endpointFetchPlanResult,
table.Identifier{"db", "tbl"}, planID)
- require.NoErrorf(t, err, "plan-id %q", planID)
-
- seg := path[len(path)-1]
- assert.NotContainsf(t, seg, "/", "plan-id %q split into
multiple segments: %q", planID, seg)
-
- decoded, err := url.PathUnescape(seg)
- require.NoErrorf(t, err, "plan-id %q", planID)
- assert.Equalf(t, planID, decoded, "plan-id %q did not
round-trip", planID)
-
- // The escaped segment must survive JoinPath's path.Clean as
the last
- // .../plan/ segment (no split, no traversal to a different
endpoint).
- escaped := cat.baseURI.JoinPath(path...).EscapedPath()
- assert.Truef(t, strings.HasSuffix(escaped, "/plan/"+seg),
- "plan-id %q was mangled by JoinPath: %s", planID,
escaped)
+ for _, tt := range []struct {
+ name string
+ id string
+ wantSeg string
+ }{
+ {name: "slash", id: "a/b", wantSeg: "a%2Fb"},
+ {name: "traversal", id: "../tasks", wantSeg: "..%2Ftasks"},
+ {name: "space", id: "a b", wantSeg: "a%20b"},
+ {name: "plus", id: "a+b", wantSeg: "a%2Bb"},
+ {name: "percent", id: "a%2Fb", wantSeg: "a%252Fb"},
+ {name: "dot", id: ".", wantSeg: "%2E"},
+ {name: "double dot", id: "..", wantSeg: "%2E%2E"},
+ {name: "plain", id: "plan-123", wantSeg: "plan-123"},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ path, err :=
cat.scanPlanningPath(endpointFetchPlanResult, table.Identifier{"db", "tbl"},
tt.id)
+ require.NoError(t, err)
+
+ seg := path[len(path)-1]
+ assert.Equal(t, tt.wantSeg, seg)
+
+ decoded, err := url.PathUnescape(seg)
+ require.NoError(t, err)
+ assert.Equal(t, tt.id, decoded)
+
+ // The escaped segment must survive JoinPath's
path.Clean as the last
+ // .../plan/ segment (no split, no traversal to a
different endpoint).
+ escaped := cat.baseURI.JoinPath(path...).EscapedPath()
+ assert.Truef(t, strings.HasSuffix(escaped,
"/plan/"+tt.wantSeg),
+ "plan-id %q was mangled by JoinPath: %s",
tt.id, escaped)
+ })
}
}
