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

Revert "s3: remove implicit directory handling"

Browse Source 
This reverts commit 412d833700.
This commit is contained in:
Chris Lu committed 2025-12-19 21:32:52 -08:00
1 parent 412d833700
commit 91c962e5f0
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
@@ -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")
}
weed/s3api/s3api_object_handlers.go
+137
View File
@@ -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)