laskoviymishka commented on code in PR #2074:
URL: https://github.com/apache/iceberg-go/pull/2074#discussion_r4164711927
##########
catalog/rest/rest.go:
##########
@@ -1454,31 +1461,31 @@ 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
Review Comment:
The second return quietly changed meaning here: it used to be the raw object
name, now it's a path-encoded segment, but the name and signature didn't move,
so nothing signals it's path-only. That's exactly what let the view payloads
below pick up the encoded value by accident. I'd have the helpers return the
raw name and ns and move the `encodePathSegment` call to the `reqPath` site (or
return a small `{ns, raw, encoded}` struct), so no caller is ever handed an
encoded string that still looks like a name.
##########
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){
Review Comment:
These assert only what the split helpers return, so the real invariant
(encoded in the path, raw in the body) isn't covered, which is why the view bug
above stays green. I'd add an httptest round-trip for at least a view create
(and a table create) with a name like `a b+c` that checks both
`r.URL.EscapedPath()` and the decoded JSON `name`. That pins the contract so a
re-wire can't regress it silently.
##########
catalog/rest/rest.go:
##########
@@ -1454,31 +1461,31 @@ 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) {
if err := catalog.ValidateViewIdentifier(ident); err != nil {
return "", "", err
}
- return r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
catalog.ObjectNameFromIdent(ident), nil
+ return r.encodeNamespace(catalog.NamespaceFromIdent(ident)),
encodePathSegment(catalog.ObjectNameFromIdent(ident)), nil
Review Comment:
This return now feeds `CreateView`, `UpdateView` and `RegisterView` straight
into their JSON `name`, so a view named `v+1` or `my view` gets
created/looked-up as `v%2B1` / `my%20view`, a silent wrong-name write and
strictly worse than before the PR for views. The table sites were fixed to
re-derive the raw name via `catalog.ObjectNameFromIdent`, but these three were
missed. Same fix: use `catalog.ObjectNameFromIdent(identifier)` for the payload
name and take the encoded return with `_` where it's only needed for the path.
--
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]
