laskoviymishka commented on code in PR #2104:
URL: https://github.com/apache/iceberg-go/pull/2104#discussion_r4231828301
##########
catalog/rest/rest_internal_test.go:
##########
@@ -114,38 +114,42 @@ 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)
Review Comment:
This pins `rawName` at the unit level, but the second goal (body carries the
raw name, URL carries the encoded one) isn't checked through a real request. A
future copy-paste of `encodedName` into a payload would pass everything here.
One httptest case for CreateTable and UpdateTable with a name like `t+x/y`,
asserting `r.URL.EscapedPath()` contains `t%2Bx%2Fy` and the decoded body
`name` is `t+x/y`, would close that.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -160,6 +164,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"},
Review Comment:
I'd add an end-to-end case here, one that runs a `..` identifier through
`tableIdentifierPath` + `reqPath` and checks the final URL, not just
`encodePathSegment` in isolation. The dot handling now runs for identifiers, so
a table or namespace named `.`/`..` goes out as `%2E`/`%2E%2E` instead of a
literal that `JoinPath` would collapse to a different endpoint. That's the
behavior this PR changes, and nothing pins it past the pure function.
`{"..","t"}` and `{"ns",".."}` cover the name and namespace levels.
##########
catalog/rest/scan_planning_test.go:
##########
@@ -96,12 +96,15 @@ 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"} {
+ for _, planID := range []string{"a/b", "../tasks", "a b", "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)
+ if planID == "a+b" {
Review Comment:
I'd lift this into the table instead of branching on one input mid-loop: a
`[]struct{ id, wantSeg string }` with a `t.Run` per case. The conditional hides
the expected value in control flow and a failure won't say which id broke; it's
also the only thing locking in the `+`→`%2B` change, since the `PathUnescape`
round-trip passes for both encodings.
##########
catalog/rest/scan_planning.go:
##########
@@ -1045,30 +1044,14 @@ func (r *Catalog) scanPlanningPath(ep endpoint, ident
table.Identifier, extra ..
// 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)
+ params = append(params, identPath.encodedNamespace,
identPath.encodedName)
for _, seg := range extra {
- params = append(params, escapeOpaquePathSegment(seg))
+ params = append(params, encodePathSegment(seg))
Review Comment:
Small thing while we're here: the comment above still explains the
`..`/`.`/JoinPath rationale in terms of the deleted `escapeOpaquePathSegment`
and never names `encodePathSegment`, which now does the work. Worth a one-line
update so the comment points at the live code.
##########
catalog/rest/rest.go:
##########
@@ -1473,36 +1481,52 @@ func (r *Catalog) namespaceToQueryParam(namespace
table.Identifier) string {
return strings.Join(namespace, r.decodedNamespaceSeparator())
}
-func (r *Catalog) splitIdentForPath(ident table.Identifier) (string, string,
error) {
+type identifierPath struct {
Review Comment:
A doc comment here would lock the contract that's the whole point of the
refactor: `rawName` is for JSON bodies, `encodedNamespace`/`encodedName` are
for paths, and the validated builders return `identifierPath{}` only alongside
a non-nil error. If you want to go further, a `func (p identifierPath)
reqPath(ep endpoint) (string, error)` method would stop callers touching the
encoded fields at all. Right now three bare `string`s still let a future caller
feed `rawName` to a path or `encodedName` to a body. The method's optional; the
doc comment I'd do.
--
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]
