laskoviymishka commented on code in PR #2074:
URL: https://github.com/apache/iceberg-go/pull/2074#discussion_r4168876007
##########
catalog/rest/rest_test.go:
##########
@@ -2773,9 +2773,50 @@ var (
}`, exampleViewMetadataJSON)
)
+func (r *RestCatalogSuite) TestUpdatePathsEncodeNamesAndBodiesRemainRaw() {
+ const objectName = "a b+c"
+
+ type updatePayload struct {
+ Identifier struct {
+ Name string `json:"name"`
+ } `json:"identifier"`
+ }
+
+ r.mux.HandleFunc("/v1/namespaces/table-ns/tables/", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal("/v1/namespaces/table-ns/tables/a%20b%2Bc",
req.URL.EscapedPath())
+
+ var payload updatePayload
+ r.Require().NoError(json.NewDecoder(req.Body).Decode(&payload))
Review Comment:
`r.Require()` here runs in the httptest handler goroutine, so a failure
calls `FailNow`/`Goexit` off the test goroutine; the response gets left
half-written and the real failure surfaces as a confusing transport EOF from
`UpdateTable` instead of the decode error. The rest of the file does this too
so it's not blocking, but a plain `r.NoError(...)` with a `return` on failure
inside the handler keeps the diagnostics honest.
##########
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:
Not blocking the merge, but this second return value is now an
encoded-for-URL name, and every body site has to independently remember to
re-derive the raw name via `ObjectNameFromIdent` rather than use it, which is
the exact shape of the round-2 regression. I'd rather the helpers couldn't hand
a caller a footgun: rename the return to `encodedName` so misuse is obvious,
give it a dedicated type, or drop the encoding here and let the path builder do
it. Fine as a follow-up.
##########
catalog/rest/rest_test.go:
##########
@@ -2773,9 +2773,50 @@ var (
}`, exampleViewMetadataJSON)
)
+func (r *RestCatalogSuite) TestUpdatePathsEncodeNamesAndBodiesRemainRaw() {
+ const objectName = "a b+c"
+
+ type updatePayload struct {
+ Identifier struct {
+ Name string `json:"name"`
+ } `json:"identifier"`
+ }
+
+ r.mux.HandleFunc("/v1/namespaces/table-ns/tables/", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal("/v1/namespaces/table-ns/tables/a%20b%2Bc",
req.URL.EscapedPath())
+
+ var payload updatePayload
+ r.Require().NoError(json.NewDecoder(req.Body).Decode(&payload))
+ r.Equal(objectName, payload.Identifier.Name)
+
+ _, err := w.Write([]byte(createTableRestExample))
+ r.Require().NoError(err)
+ })
+
+ r.mux.HandleFunc("/v1/namespaces/view-ns/views/", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal("/v1/namespaces/view-ns/views/a%20b%2Bc",
req.URL.EscapedPath())
+
+ var payload updatePayload
+ r.Require().NoError(json.NewDecoder(req.Body).Decode(&payload))
+ r.Equal(objectName, payload.Identifier.Name)
+
+ _, err := w.Write([]byte(createViewRestExample))
+ r.Require().NoError(err)
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL)
+ r.Require().NoError(err)
+
+ _, err = cat.UpdateTable(context.Background(),
table.Identifier{"table-ns", objectName}, nil, nil)
Review Comment:
This nicely pins `UpdateTable`/`UpdateView`, but `CreateTable`,
`RegisterTable` and `CommitTable` got the same `ObjectNameFromIdent` body fix
without a special-char assertion to guard it, and `CommitTable` is on the write
path. Extending this test (or the existing create/register cases) to an `a b+c`
name and asserting the raw body `name` would cover the rest of the regression
class. Follow-up is fine.
--
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]
