zeroshade commented on code in PR #2074:
URL: https://github.com/apache/iceberg-go/pull/2074#discussion_r4158447886
##########
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 now returns the encoded name, but `CreateView`, `UpdateView` and
`RegisterView` still put that return value into the JSON body. Before this PR
they sent the raw name.
Repro on 141d7b25 with `table.Identifier{"ns", "my view+x"}`:
- `UpdateView` sends `POST /v1/namespaces/ns/views/my%20view%2Bx` with
`"identifier":{"namespace":["ns"],"name":"my%20view%2Bx"}`
- `RegisterView` sends `"name":"my%20view%2Bx"`
- `UpdateTable` / `RegisterTable` correctly send `"name":"my view+x"`
So `CreateView`/`RegisterView` would create a view literally named
`my%20view%2Bx`, and `UpdateView`'s body identifier no longer matches its path.
Fix: do what you already did for tables. Use `ns, _, err :=` in
`CreateView`/`RegisterView` and pass `catalog.ObjectNameFromIdent(identifier)`
as the name. In `UpdateView`, rename the second return to `encodedView`, use it
only in `reqPath`, and use the raw name in `restIdentifier`.
##########
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)
+ })
Review Comment:
These only check what the split helpers return, so nothing checks that
request bodies stay raw. That gap is how the view regression got through.
Please add a `RestCatalogSuite` test with a name like `a b+c`: assert
`req.URL.EscapedPath()` ends in `a%20b%2Bc` and that the JSON `name` (or
`identifier.name`) is exactly `a b+c`. Cover at least one table call
(`UpdateTable` or `RegisterTable`) and one view call (`CreateView`,
`RegisterView` or `UpdateView`).
--
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]
