diff --git a/weed/s3api/s3api_implicit_directory_test.go b/weed/s3api/s3api_implicit_directory_test.go deleted file mode 100644 index e7c3633fc..000000000 --- a/weed/s3api/s3api_implicit_directory_test.go +++ /dev/null @@ -1,285 +0,0 @@ -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 9b1495128..e4c15038b 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -219,62 +219,6 @@ 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 @@ -2067,34 +2011,6 @@ 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) @@ -2273,59 +2189,6 @@ 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)