laskoviymishka commented on code in PR #2104:
URL: https://github.com/apache/iceberg-go/pull/2104#discussion_r4286666788
##########
catalog/rest/rest.go:
##########
@@ -1473,36 +1481,75 @@ func (r *Catalog) namespaceToQueryParam(namespace
table.Identifier) string {
return strings.Join(namespace, r.decodedNamespaceSeparator())
}
-func (r *Catalog) splitIdentForPath(ident table.Identifier) (string, string,
error) {
+// identifierPath keeps rawName for JSON request bodies separate from
+// encodedNamespace and encodedName for URL paths. The validated table, view,
and
+// function path builders return a zero value only alongside a non-nil error.
+type identifierPath struct {
+ encodedNamespace string
+ rawName string
+ encodedName string
+}
+
+// namespaceReqPath builds a namespace-scoped path, such as a create or
register
+// endpoint, without including the object name.
+func (p identifierPath) namespaceReqPath(ep endpoint) ([]string, error) {
+ return ep.reqPath(p.encodedNamespace)
+}
+
+// objectReqPath builds an object-scoped path using the encoded identifier
components.
+// extra contains raw opaque path segments, such as scan plan IDs. Encode each
+// segment here so slashes, plus signs, and pure-dot segments remain literal.
+func (p identifierPath) objectReqPath(ep endpoint, extra ...string) ([]string,
error) {
Review Comment:
This is the one spot where the raw/encoded contract flips inside a single
call. The struct fields arrive already encoded, but `extra` has to be raw, so a
caller that pre-encodes a plan ID silently gets `%252F`. The doc comment covers
it, but I'd rename the param to `rawExtra` so the signature follows the same
naming as `rawName`.
##########
catalog/rest/rest.go:
##########
@@ -1250,13 +1250,13 @@ func (r *Catalog) tableFromResponse(
// catalog-owned dispatcher. It is non-nil only when the client enabled
// reporting and the server advertises the endpoint (see init).
if r.metricsDispatcher != nil {
- if ns, tbl, idErr := r.splitIdentForPath(identifier); idErr !=
nil {
+ if identPath, idErr := r.tableIdentifierPath(identifier); idErr
!= nil {
// Unreachable for a valid identifier that already
loaded above, but if it
// ever regresses, log why the REST reporter was
dropped rather than let
// metrics silently vanish with no signal.
slog.Debug("iceberg: skipping REST metrics reporter,
cannot split identifier",
Review Comment:
This log still says "cannot split identifier", but nothing gets split after
the rename.
```suggestion
slog.Debug("iceberg: skipping REST metrics reporter,
cannot build identifier 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]
