diff --git a/weed/s3api/iceberg/handlers_commit.go b/weed/s3api/iceberg/handlers_commit.go index 587137a11..02b50ee2b 100644 --- a/weed/s3api/iceberg/handlers_commit.go +++ b/weed/s3api/iceberg/handlers_commit.go @@ -189,7 +189,7 @@ func (s *Server) handleUpdateTable(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: CommitTable GetTable error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } diff --git a/weed/s3api/iceberg/handlers_namespace.go b/weed/s3api/iceberg/handlers_namespace.go index 2c2a937c5..0a6ff7cde 100644 --- a/weed/s3api/iceberg/handlers_namespace.go +++ b/weed/s3api/iceberg/handlers_namespace.go @@ -83,7 +83,7 @@ func (s *Server) handleListNamespaces(w http.ResponseWriter, r *http.Request) { if err != nil { glog.Infof("Iceberg: ListNamespaces error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -139,7 +139,7 @@ func (s *Server) handleCreateNamespace(w http.ResponseWriter, r *http.Request) { return } glog.Errorf("Iceberg: CreateNamespace error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -183,7 +183,7 @@ func (s *Server) handleGetNamespace(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: GetNamespace error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -332,6 +332,10 @@ func (s *Server) handleNamespaceExists(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) return } + if nameValidationError(err) { + w.WriteHeader(http.StatusBadRequest) + return + } glog.V(1).Infof("Iceberg: NamespaceExists error: %v", err) w.WriteHeader(http.StatusInternalServerError) return @@ -375,7 +379,7 @@ func (s *Server) handleDropNamespace(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: DropNamespace error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } diff --git a/weed/s3api/iceberg/handlers_table.go b/weed/s3api/iceberg/handlers_table.go index 334765b0a..acad64a01 100644 --- a/weed/s3api/iceberg/handlers_table.go +++ b/weed/s3api/iceberg/handlers_table.go @@ -69,7 +69,7 @@ func (s *Server) handleListTables(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: ListTables error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -212,7 +212,7 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) { } if !isNoSuchTableError(existsErr) { glog.V(1).Infof("Iceberg: CreateTable existence check failed for %s.%s: %v", flattenNamespacePath(namespace), tableName, existsErr) - writeError(w, http.StatusInternalServerError, "InternalServerError", existsErr.Error()) + writeManagerError(w, existsErr) return } @@ -302,7 +302,7 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: CreateTable error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -359,7 +359,7 @@ func (s *Server) handleLoadTable(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: LoadTable error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } @@ -509,7 +509,7 @@ func (s *Server) handleDropTable(w http.ResponseWriter, r *http.Request) { return } glog.V(1).Infof("Iceberg: DropTable error: %v", err) - writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) + writeManagerError(w, err) return } diff --git a/weed/s3api/iceberg/iceberg_invalid_name_test.go b/weed/s3api/iceberg/iceberg_invalid_name_test.go new file mode 100644 index 000000000..af7c5e415 --- /dev/null +++ b/weed/s3api/iceberg/iceberg_invalid_name_test.go @@ -0,0 +1,65 @@ +package iceberg + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +func TestNameValidationError(t *testing.T) { + cases := []struct { + err error + want bool + }{ + {nil, false}, + // Wrapped exactly as the s3tables manager surfaces it. + {fmt.Errorf("all filers failed, last error: invalid namespace name: only 'a-z', '0-9', and '_' are allowed"), true}, + {fmt.Errorf("invalid table name: only 'a-z', '0-9', and '_' are allowed"), true}, + {fmt.Errorf("namespace name must start with a letter or digit"), true}, + {fmt.Errorf("namespace name cannot start with reserved prefix 'aws'"), true}, + {fmt.Errorf("table name must be between 1 and 255 characters"), true}, + {fmt.Errorf("namespace not found"), false}, + {fmt.Errorf("all filers failed, last error: rpc timeout"), false}, + // Unrelated faults that merely mention a name must stay 500. + {fmt.Errorf("failed to resolve table name from index"), false}, + {fmt.Errorf("error fetching namespace name mapping"), false}, + } + for _, c := range cases { + if got := nameValidationError(c.err); got != c.want { + t.Errorf("nameValidationError(%v) = %v, want %v", c.err, got, c.want) + } + } +} + +func TestWriteManagerError(t *testing.T) { + cases := []struct { + name string + err error + wantCode int + wantType string + }{ + {"invalid name is a client error", fmt.Errorf("invalid namespace name: only 'a-z', '0-9', and '_' are allowed"), http.StatusBadRequest, "BadRequestException"}, + {"everything else is a server fault", fmt.Errorf("all filers failed, last error: connection refused"), http.StatusInternalServerError, "InternalServerError"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + rec := httptest.NewRecorder() + writeManagerError(rec, c.err) + if rec.Code != c.wantCode { + t.Fatalf("status = %d, want %d", rec.Code, c.wantCode) + } + var resp ErrorResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.Error.Type != c.wantType { + t.Fatalf("error type = %q, want %q", resp.Error.Type, c.wantType) + } + if resp.Error.Code != c.wantCode { + t.Fatalf("error code = %d, want %d", resp.Error.Code, c.wantCode) + } + }) + } +} diff --git a/weed/s3api/iceberg/utils.go b/weed/s3api/iceberg/utils.go index bc3a7be01..f01130e17 100644 --- a/weed/s3api/iceberg/utils.go +++ b/weed/s3api/iceberg/utils.go @@ -99,6 +99,41 @@ func writeError(w http.ResponseWriter, status int, errType, message string) { writeJSON(w, status, resp) } +// nameValidationError reports whether err is an S3 Tables namespace or table +// name validation failure. Such names are client input (the S3 Tables charset +// is stricter than the Iceberg REST spec), so the catalog answers 400 rather +// than treating it as a server fault. +func nameValidationError(err error) bool { + if err == nil { + return false + } + // Match the validator's own phrasings rather than a bare "namespace name"/ + // "table name" so unrelated faults (e.g. "failed to resolve table name") + // aren't misreported as client errors. Lowercased for resilience to + // capitalization changes. + msg := strings.ToLower(err.Error()) + for _, marker := range []string{ + "invalid namespace name", "namespace name must", "namespace name cannot", + "invalid table name", "table name must", "table name cannot", + } { + if strings.Contains(msg, marker) { + return true + } + } + return false +} + +// writeManagerError maps a residual s3tables manager error to a response: +// name validation failures are client errors (400); anything else is a server +// fault (500). +func writeManagerError(w http.ResponseWriter, err error) { + if nameValidationError(err) { + writeError(w, http.StatusBadRequest, "BadRequestException", err.Error()) + return + } + writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error()) +} + // getBucketFromPrefix extracts table bucket name from prefix parameter. // For now, we use the prefix as the table bucket name. //