diff --git a/weed/s3api/s3api_implicit_directory_test.go b/weed/s3api/s3api_implicit_directory_test.go new file mode 100644 index 000000000..e7c3633fc --- /dev/null +++ b/weed/s3api/s3api_implicit_directory_test.go @@ -0,0 +1,285 @@ +package s3api + +import ( + "io" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" +) + +// TestImplicitDirectoryBehaviorLogic tests the core logic for implicit directory detection +// This tests the decision logic without requiring a full S3 server setup +func TestImplicitDirectoryBehaviorLogic(t *testing.T) { + tests := []struct { + name string + objectPath string + hasTrailingSlash bool + fileSize uint64 + isDirectory bool + hasChildren bool + versioningEnabled bool + shouldReturn404 bool + description string + }{ + { + name: "Implicit directory: 0-byte file with children, no trailing slash", + objectPath: "dataset", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: false, + hasChildren: true, + versioningEnabled: false, + shouldReturn404: true, + description: "Should return 404 to force s3fs LIST-based discovery", + }, + { + name: "Implicit directory: actual directory with children, no trailing slash", + objectPath: "dataset", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: true, + hasChildren: true, + versioningEnabled: false, + shouldReturn404: true, + description: "Should return 404 for directory with children", + }, + { + name: "Explicit directory request: trailing slash", + objectPath: "dataset/", + hasTrailingSlash: true, + fileSize: 0, + isDirectory: true, + hasChildren: true, + versioningEnabled: false, + shouldReturn404: false, + description: "Should return 200 for explicit directory request (trailing slash)", + }, + { + name: "Empty file: 0-byte file without children", + objectPath: "empty.txt", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: false, + hasChildren: false, + versioningEnabled: false, + shouldReturn404: false, + description: "Should return 200 for legitimate empty file", + }, + { + name: "Empty directory: 0-byte directory without children", + objectPath: "empty-dir", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: true, + hasChildren: false, + versioningEnabled: false, + shouldReturn404: false, + description: "Should return 200 for empty directory", + }, + { + name: "Regular file: non-zero size", + objectPath: "file.txt", + hasTrailingSlash: false, + fileSize: 100, + isDirectory: false, + hasChildren: false, + versioningEnabled: false, + shouldReturn404: false, + description: "Should return 200 for regular file with content", + }, + { + name: "Versioned bucket: implicit directory should return 200", + objectPath: "dataset", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: false, + hasChildren: true, + versioningEnabled: true, + shouldReturn404: false, + description: "Should return 200 for versioned buckets (skip implicit dir check)", + }, + { + name: "PyArrow directory marker: 0-byte with children", + objectPath: "dataset", + hasTrailingSlash: false, + fileSize: 0, + isDirectory: false, + hasChildren: true, + versioningEnabled: false, + shouldReturn404: true, + description: "Should return 404 for PyArrow-created directory markers", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the logic: should we return 404? + // Logic from HeadObjectHandler: + // if !versioningConfigured && !strings.HasSuffix(object, "/") { + // if isZeroByteFile || isActualDirectory { + // if hasChildren { + // return 404 + // } + // } + // } + + isZeroByteFile := tt.fileSize == 0 && !tt.isDirectory + isActualDirectory := tt.isDirectory + + shouldReturn404 := false + if !tt.versioningEnabled && !tt.hasTrailingSlash { + if isZeroByteFile || isActualDirectory { + if tt.hasChildren { + shouldReturn404 = true + } + } + } + + if shouldReturn404 != tt.shouldReturn404 { + t.Errorf("Logic mismatch for %s:\n Expected shouldReturn404=%v\n Got shouldReturn404=%v\n Description: %s", + tt.name, tt.shouldReturn404, shouldReturn404, tt.description) + } else { + t.Logf("✓ %s: correctly returns %d", tt.name, map[bool]int{true: 404, false: 200}[shouldReturn404]) + } + }) + } +} + +// TestHasChildrenLogic tests the hasChildren helper function logic +func TestHasChildrenLogic(t *testing.T) { + tests := []struct { + name string + bucket string + prefix string + listResponse *filer_pb.ListEntriesResponse + listError error + expectedResult bool + description string + }{ + { + name: "Directory with children", + bucket: "test-bucket", + prefix: "dataset", + listResponse: &filer_pb.ListEntriesResponse{ + Entry: &filer_pb.Entry{ + Name: "file.parquet", + IsDirectory: false, + }, + }, + listError: nil, + expectedResult: true, + description: "Should return true when at least one child exists", + }, + { + name: "Empty directory", + bucket: "test-bucket", + prefix: "empty-dir", + listResponse: nil, + listError: io.EOF, + expectedResult: false, + description: "Should return false when no children exist (EOF)", + }, + { + name: "Directory with leading slash in prefix", + bucket: "test-bucket", + prefix: "/dataset", + listResponse: &filer_pb.ListEntriesResponse{ + Entry: &filer_pb.Entry{ + Name: "file.parquet", + IsDirectory: false, + }, + }, + listError: nil, + expectedResult: true, + description: "Should handle leading slashes correctly", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the hasChildren logic: + // 1. It should trim leading slashes from prefix + // 2. It should list with Limit=1 + // 3. It should return true if any entry is received + // 4. It should return false if EOF is received + + hasChildren := false + if tt.listError == nil && tt.listResponse != nil { + hasChildren = true + } else if tt.listError == io.EOF { + hasChildren = false + } + + if hasChildren != tt.expectedResult { + t.Errorf("hasChildren logic mismatch for %s:\n Expected: %v\n Got: %v\n Description: %s", + tt.name, tt.expectedResult, hasChildren, tt.description) + } else { + t.Logf("✓ %s: correctly returns %v", tt.name, hasChildren) + } + }) + } +} + +// TestImplicitDirectoryEdgeCases tests edge cases in the implicit directory detection +func TestImplicitDirectoryEdgeCases(t *testing.T) { + tests := []struct { + name string + scenario string + expectation string + }{ + { + name: "PyArrow write_dataset creates 0-byte files", + scenario: "PyArrow creates 'dataset' as 0-byte file, then writes 'dataset/file.parquet'", + expectation: "HEAD dataset → 404 (has children), s3fs uses LIST → correctly identifies as directory", + }, + { + name: "Filer creates actual directories", + scenario: "Filer creates 'dataset' as actual directory with IsDirectory=true", + expectation: "HEAD dataset → 404 (has children), s3fs uses LIST → correctly identifies as directory", + }, + { + name: "Empty file edge case", + scenario: "User creates 'empty.txt' as 0-byte file with no children", + expectation: "HEAD empty.txt → 200 (no children), s3fs correctly reports as file", + }, + { + name: "Explicit directory request", + scenario: "User requests 'dataset/' with trailing slash", + expectation: "HEAD dataset/ → 200 (explicit directory request), normal directory behavior", + }, + { + name: "Versioned bucket", + scenario: "Bucket has versioning enabled", + expectation: "HEAD dataset → 200 (skip implicit dir check), versioned semantics apply", + }, + { + name: "AWS S3 compatibility", + scenario: "Only 'dataset/file.txt' exists, no marker at 'dataset'", + expectation: "HEAD dataset → 404 (object doesn't exist), matches AWS S3 behavior", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Logf("Scenario: %s", tt.scenario) + t.Logf("Expected: %s", tt.expectation) + }) + } +} + +// TestImplicitDirectoryIntegration is an integration test placeholder +// Run with: cd test/s3/parquet && make test-implicit-dir-with-server +func TestImplicitDirectoryIntegration(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + t.Skip("Integration test - run manually with: cd test/s3/parquet && make test-implicit-dir-with-server") +} + +// Benchmark for hasChildren performance +func BenchmarkHasChildrenCheck(b *testing.B) { + // This benchmark would measure the performance impact of the hasChildren check + // Expected: ~1-5ms per call (one gRPC LIST request with Limit=1) + b.Skip("Benchmark - requires full filer setup") +} diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index e4c15038b..9b1495128 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -219,6 +219,62 @@ func removeDuplicateSlashes(object string) string { return result.String() } +// hasChildren checks if a path has any child objects (is a directory with contents) +// +// This helper function is used to distinguish implicit directories from regular files or empty directories. +// An implicit directory is one that exists only because it has children, not because it was explicitly created. +// +// Implementation: +// - Lists the directory with Limit=1 to check for at least one child +// - Returns true if any child exists, false otherwise +// - Efficient: only fetches one entry to minimize overhead +// +// Used by HeadObjectHandler to implement AWS S3-compatible implicit directory behavior: +// - If a 0-byte object or directory has children → it's an implicit directory → HEAD returns 404 +// - If a 0-byte object or directory has no children → it's empty → HEAD returns 200 +// +// Examples: +// +// hasChildren("bucket", "dataset") where "dataset/file.txt" exists → true +// hasChildren("bucket", "empty-dir") where no children exist → false +// +// Performance: ~1-5ms per call (one gRPC LIST request with Limit=1) +func (s3a *S3ApiServer) hasChildren(bucket, prefix string) bool { + // Clean up prefix: remove leading slashes + cleanPrefix := strings.TrimPrefix(prefix, "/") + + // The directory to list is bucketDir + cleanPrefix + bucketDir := s3a.option.BucketsPath + "/" + bucket + fullPath := bucketDir + "/" + cleanPrefix + + // Try to list one child object in the directory + err := s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + request := &filer_pb.ListEntriesRequest{ + Directory: fullPath, + Limit: 1, + InclusiveStartFrom: true, + } + + stream, err := client.ListEntries(context.Background(), request) + if err != nil { + return err + } + + // Check if we got at least one entry + _, err = stream.Recv() + if err == io.EOF { + return io.EOF // No children + } + if err != nil { + return err + } + return nil + }) + + // If we got an entry (not EOF), then it has children + return err == nil +} + // checkDirectoryObject checks if the object is a directory object (ends with "/") and if it exists // Returns: (entry, isDirectoryObject, error) // - entry: the directory entry if found and is a directory @@ -2011,6 +2067,34 @@ func (s *simpleMasterClient) GetLookupFileIdFunction() wdclient.LookupFileIdFunc } // HeadObjectHandler handles S3 HEAD object requests +// +// Special behavior for implicit directories: +// When a HEAD request is made on a path without a trailing slash, and that path represents +// a directory with children (either a 0-byte file marker or an actual directory), this handler +// returns 404 Not Found instead of 200 OK. This behavior improves compatibility with s3fs and +// matches AWS S3's handling of implicit directories. +// +// Rationale: +// - AWS S3 typically doesn't create directory markers when files are uploaded (e.g., uploading +// "dataset/file.txt" doesn't create a marker at "dataset") +// - Some S3 clients (like PyArrow with s3fs) create directory markers, which can confuse s3fs +// - s3fs's info() method calls HEAD first; if it succeeds with size=0, s3fs incorrectly reports +// the object as a file instead of checking for children +// - By returning 404 for implicit directories, we force s3fs to fall back to LIST-based discovery, +// which correctly identifies directories by checking for children +// +// Examples: +// +// HEAD /bucket/dataset (no trailing slash, has children) → 404 Not Found (implicit directory) +// HEAD /bucket/dataset/ (trailing slash) → 200 OK (explicit directory request) +// HEAD /bucket/empty.txt (0-byte file, no children) → 200 OK (legitimate empty file) +// HEAD /bucket/file.txt (regular file) → 200 OK (normal operation) +// +// This behavior only applies to: +// - Non-versioned buckets (versioned buckets use different semantics) +// - Paths without trailing slashes (trailing slash indicates explicit directory request) +// - Objects that are either 0-byte files or actual directories +// - Objects that have at least one child (checked via hasChildren) func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request) { bucket, object := s3_constants.GetBucketAndObject(r) @@ -2189,6 +2273,59 @@ func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request return } + // Implicit Directory Handling for s3fs Compatibility + // ==================================================== + // + // Background: + // Some S3 clients (like PyArrow with s3fs) create directory markers when writing datasets. + // These can be either: + // 1. 0-byte files with directory MIME type (e.g., "application/octet-stream") + // 2. Actual directories in the filer (created by PyArrow's write_dataset) + // + // Problem: + // s3fs's info() method calls HEAD on the path. If HEAD returns 200 with size=0, + // s3fs incorrectly reports it as a file (type='file', size=0) instead of checking + // for children. This causes PyArrow to fail with "Parquet file size is 0 bytes". + // + // Solution: + // For non-versioned objects without trailing slash, if the object is a 0-byte file + // or directory AND has children, return 404 instead of 200. This forces s3fs to + // fall back to LIST-based discovery, which correctly identifies it as a directory. + // + // AWS S3 Compatibility: + // AWS S3 typically doesn't create directory markers for implicit directories, so + // HEAD on "dataset" (when only "dataset/file.txt" exists) returns 404. Our behavior + // matches this by returning 404 for implicit directories with children. + // + // Edge Cases Handled: + // - Empty files (0-byte, no children) → 200 OK (legitimate empty file) + // - Empty directories (no children) → 200 OK (legitimate empty directory) + // - Explicit directory requests (trailing slash) → 200 OK (handled earlier) + // - Versioned objects → Skip this check (different semantics) + // + // Performance: + // Only adds overhead for 0-byte files or directories without trailing slash. + // Cost: One LIST operation with Limit=1 (~1-5ms). + // + if !versioningConfigured && !strings.HasSuffix(object, "/") { + // Check if this is an implicit directory (either a 0-byte file or actual directory with children) + // PyArrow may create 0-byte files when writing datasets, or the filer may have actual directories + if objectEntryForSSE.Attributes != nil { + isZeroByteFile := objectEntryForSSE.Attributes.FileSize == 0 && !objectEntryForSSE.IsDirectory + isActualDirectory := objectEntryForSSE.IsDirectory + + if isZeroByteFile || isActualDirectory { + // Check if it has children (making it an implicit directory) + if s3a.hasChildren(bucket, object) { + // This is an implicit directory with children + // Return 404 to force clients (like s3fs) to use LIST-based discovery + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) + return + } + } + } + } + // For HEAD requests, we already have all metadata - just set headers directly totalSize := int64(filer.FileSize(objectEntryForSSE)) s3a.setResponseHeaders(w, r, objectEntryForSSE, totalSize)