Github-Mirror/seaweedfs
1
0 Fork 0
mirror of https://github.com/seaweedfs/seaweedfs.git synced 2026-10-06 06:22:05 +02:00
Code Issues Packages Projects Releases Wiki Activity

s3: remove implicit directory handling

Browse Source 
Since empty folders can now be async deleted, we no longer need special
handling for implicit directories. This change simplifies the code and
improves compatibility with S3 clients like Veeam 13.

Changes:
- Removed hasChildren() function that checked for child objects
- Removed implicit directory logic from HeadObjectHandler
- Removed unit tests for implicit directory behavior
- All folders are now treated equally

Benefits:
- Better compatibility with Veeam 13 and other S3 clients
- Simpler code without special cases
- No performance overhead from hasChildren checks
- Existing folders (both explicit with '/' and regular) continue to work
This commit is contained in:
Chris Lu committed 2025-12-19 21:19:28 -08:00
1 parent 5b86d33c3c
commit 412d833700
2 files changed
-422
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

weed/s3api/s3api_implicit_directory_test.go
-285
View File
@@ -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")
}
weed/s3api/s3api_object_handlers.go
-137
View File
@@ -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)