laskoviymishka commented on code in PR #2074:
URL: https://github.com/apache/iceberg-go/pull/2074#discussion_r4145291750
##########
catalog/rest/rest.go:
##########
@@ -1421,9 +1421,16 @@ func (r *Catalog) nsSeparator() string {
return r.namespaceSeparator
}
+func encodeString(value string) string {
+ encoded := url.QueryEscape(value)
Review Comment:
`url.QueryEscape` is form encoding, so it renders a space as `+`. In a path
segment `+` is a literal `+` (RFC 3986 §3.3), so `my table` now goes out as
`.../tables/my+table` (it was `my%20table` before this PR), and an RFC-3986
server reads it back as the literal `my+table` and 404s, which is the exact
case we're trying to fix. Java agrees: `RESTUtil.encodeString`'s own Javadoc
says it's for form data and not for path segments, and `ResourcePaths.table()`
uses `encodePathSegment`.
I'd base this on `url.PathEscape` and then encode the one char it leaves
literal:
```go
// encodePathSegment escapes a REST path segment per RFC 3986: space -> %20,
// and + -> %2B so a name like "a+b" isn't read back as "a b" by a
// form-decoding server. Matches Java's RESTUtil.encodePathSegment.
func encodePathSegment(value string) string {
return strings.ReplaceAll(url.PathEscape(value), "+", "%2B")
}
```
That also lets us drop the `%2A`/`~` substitutions, since they only exist to
undo QueryEscape's form encoding (`PathEscape` already leaves `*` alone and
encodes `~` as `%7E`).
##########
catalog/rest/rest.go:
##########
@@ -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)),
encodeString(catalog.ObjectNameFromIdent(ident)), nil
Review Comment:
This only reaches table names. `splitViewIdentForPath` and
`splitFunctionIdentForPath` still return the raw name, and `encodeNamespace`
leaves `+` literal too, so after this we'd ship three different encodings in
one URL (table `%2B`, view/function/namespace raw `+`). Java routes all of them
through `encodePathSegment`; I'd send every identifier path segment through the
same encoder so a view named `my view` and a table named `my view` don't
disagree on the wire. The new `namespace+name` -> `namespace+name` assertion in
the test documents exactly this gap and should flip to `%2B` once the encoding
is unified.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -81,6 +81,15 @@ 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+name", ns)
+ assert.Equal(t, "table%2Bname", tbl)
+}
+
+func TestEncodeString(t *testing.T) {
+ assert.Equal(t, "+%25%26%2B%C2%A3%E2%82%AC", encodeString(" %&+£€"))
Review Comment:
This asserts one mixed input, so neither special case is actually pinned:
both `ReplaceAll` calls could be deleted and the test still passes. Whatever
encoder we land on, I'd pin the wire behavior directly (a space should come out
as `%20`, a literal `+` as `%2B`) so the encoding can't drift silently. A
table-driven form matching `TestSplitIdentForPathRequiresNamespaceAndName`
reads well here.
##########
catalog/rest/rest.go:
##########
@@ -1607,7 +1614,7 @@ func (r *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, requi
restIdentifier := identifier{
Namespace: catalog.NamespaceFromIdent(ident),
- Name: tblName,
+ Name: catalog.ObjectNameFromIdent(ident),
Review Comment:
`tblName` now holds a percent-encoded value but the name still reads like
the raw one. The path uses it correctly and the JSON body here correctly
switched to raw `ObjectNameFromIdent`, but a later dedup refactor that drops
the encoded var back into this struct would silently send percent-encoded JSON.
I'd rename it to `encodedTbl` so the encoding is visible at the use site. Same
spot in `UpdateTable`.
--
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]
