read directory object #7002

chrislusf · 2025-07-20T16:19:15Z

What problem are we solving?

How are we solving the problem?

How is the PR tested?

Checks

I have added unit tests if possible.
I will add related wiki document changes and link to this PR after merging.

gemini-code-assist

Summary of Changes

Hello @chrislusf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the S3 API server's functionality by enabling it to treat paths ending with a slash as 'directory objects' that can hold and serve content. This change allows for a more flexible representation of directories within the S3-compatible storage, moving beyond a simple prefix-based model to one where directories themselves can be addressable entities with associated data.

Highlights

Directory Object Handling: Introduced the ability for the S3 API server to recognize and serve content for 'directory objects' (object paths ending with a slash '/').
New Helper Functions: Added checkDirectoryObject to determine if a given object path is a valid, existing directory entry, and serveDirectoryContent to handle the HTTP response for such directory objects, including setting appropriate headers and writing their content.
API Handler Updates: Modified GetObjectHandler and HeadObjectHandler to utilize the new directory object handling logic. Previously, these handlers would return a 'Not Implemented' error for paths ending in '/', but now they will serve the directory object's content or metadata, or return NoSuchKey if the directory object does not exist.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces functionality to handle S3 directory objects, which are objects ending with a '/'. The changes add helper functions to check for and serve these directory objects and integrate them into the GetObjectHandler and HeadObjectHandler. My review focuses on improving error handling, which is critical for robustness. The new checkDirectoryObject function swallows errors, which can hide underlying problems. The GetObjectHandler and HeadObjectHandler ignore the error returned from checkDirectoryObject, which is a critical bug. The new serveDirectoryContent function has opportunities for improved readability and needs to handle potential errors when writing the response body. I've provided specific suggestions to address these points.

gemini-code-assist · 2025-07-20T16:21:19Z

weed/s3api/s3api_object_handlers.go

+	if dirEntry, _ := s3a.checkDirectoryObject(bucket, object); dirEntry != nil {
+		glog.V(2).Infof("GetObjectHandler: directory object %s/%s found, serving content", bucket, object)
+		s3a.serveDirectoryContent(w, r, dirEntry)
+		return
+	} else if strings.HasSuffix(object, "/") {
+		// Directory object but doesn't exist
+		glog.V(2).Infof("GetObjectHandler: directory object %s/%s not found", bucket, object)
+		s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey)
 		return
 	}


The error returned from s3a.checkDirectoryObject is being ignored. This is a critical issue because if an underlying error occurs (e.g., a filer connection problem), it will be silently dropped. The code will then incorrectly proceed as if the directory was simply not found. The error must be checked and handled.

dirEntry, err := s3a.checkDirectoryObject(bucket, object) if err != nil { glog.Errorf("GetObjectHandler: failed to check for directory object %s/%s: %v", bucket, object, err) s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } if dirEntry != nil { glog.V(2).Infof("GetObjectHandler: directory object %s/%s found, serving content", bucket, object) s3a.serveDirectoryContent(w, r, dirEntry) return } else if strings.HasSuffix(object, "/") { // Directory object but doesn't exist glog.V(2).Infof("GetObjectHandler: directory object %s/%s not found", bucket, object) s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) return }

gemini-code-assist · 2025-07-20T16:21:19Z

weed/s3api/s3api_object_handlers.go

+	if dirEntry, _ := s3a.checkDirectoryObject(bucket, object); dirEntry != nil {
+		glog.V(2).Infof("HeadObjectHandler: directory object %s/%s found, serving content", bucket, object)
+		s3a.serveDirectoryContent(w, r, dirEntry)
+		return
+	} else if strings.HasSuffix(object, "/") {
+		// Directory object but doesn't exist
+		glog.V(2).Infof("HeadObjectHandler: directory object %s/%s not found", bucket, object)
+		s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey)
+		return
+	}


The error returned from s3a.checkDirectoryObject is being ignored. This is a critical issue because if an underlying error occurs (e.g., a filer connection problem), it will be silently dropped. The code will then incorrectly proceed as if the directory was simply not found. The error must be checked and handled.

dirEntry, err := s3a.checkDirectoryObject(bucket, object) if err != nil { glog.Errorf("HeadObjectHandler: failed to check for directory object %s/%s: %v", bucket, object, err) s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } if dirEntry != nil { glog.V(2).Infof("HeadObjectHandler: directory object %s/%s found, serving content", bucket, object) s3a.serveDirectoryContent(w, r, dirEntry) return } else if strings.HasSuffix(object, "/") { // Directory object but doesn't exist glog.V(2).Infof("HeadObjectHandler: directory object %s/%s not found", bucket, object) s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) return }

gemini-code-assist · 2025-07-20T16:21:19Z

weed/s3api/s3api_object_handlers.go

+	if err != nil {
+		return nil, nil // Directory doesn't exist
+	}


The current implementation swallows any error from s3a.getEntry, treating all errors as if the directory does not exist. This can hide underlying issues, such as problems connecting to the filer. It's better to handle the 'not found' case specifically and propagate other errors. Note that the callers of this function will need to be updated to handle the returned error.

if err != nil { if err == filer_pb.ErrNotFound { return nil, nil // Directory doesn't exist, which is an expected case. } return nil, err // Propagate other errors to the caller. }

gemini-code-assist · 2025-07-20T16:21:19Z

weed/s3api/s3api_object_handlers.go

+	if len(entry.Content) > 0 {
+		w.Write(entry.Content)
+	}


The error returned by w.Write is not being checked. This could lead to silent failures if writing to the response fails (e.g., if the client closes the connection). The error should be checked and logged.

if len(entry.Content) > 0 { if _, err := w.Write(entry.Content); err != nil { glog.V(1).Infof("failed to write directory content for object %s: %v", r.URL.Path, err) } }

gemini-code-assist · 2025-07-20T16:21:20Z

weed/s3api/s3api_object_handlers.go

+	w.Header().Set("Content-Type", entry.Attributes.Mime)
+	if w.Header().Get("Content-Type") == "" {
+		w.Header().Set("Content-Type", "application/octet-stream")
+	}


This logic for setting the Content-Type header can be simplified for better readability and to avoid an unnecessary call to w.Header().Get().

contentType := entry.Attributes.Mime if contentType == "" { contentType = "application/octet-stream" } w.Header().Set("Content-Type", contentType)

Copilot

Pull Request Overview

This PR implements support for reading directory objects in the S3 API server by allowing retrieval of objects that end with "/" (forward slash). Previously, such requests would return a "not implemented" error.

Key changes:

Adds logic to detect and handle directory object requests (paths ending with "/")
Implements content serving for existing directory objects
Updates both GET and HEAD object handlers to support directory objects

Copilot · 2025-07-20T16:39:19Z

weed/s3api/s3api_object_handlers.go

+	cleanObject := strings.TrimSuffix(strings.TrimPrefix(object, "/"), "/")
+
+	if cleanObject == "" {
+		return nil, true, nil // Root level directory object, but we don't handle it


The comment suggests root level directory objects are not handled, but the function returns success (no error). This is misleading - either handle the case properly or clarify why this specific case is intentionally skipped.

Suggested change

return nil, true, nil // Root level directory object, but we don't handle it

return nil, true, nil // Root level directory object is intentionally skipped because it represents the bucket itself, which is handled elsewhere in the code.

Copilot · 2025-07-20T16:39:19Z

weed/s3api/s3api_object_handlers.go

+
+	// Write content
+	w.WriteHeader(http.StatusOK)
+	if len(entry.Content) > 0 {


Directory objects typically should not have content data. This logic assumes directories can have content, which may be incorrect for S3 directory objects that are usually zero-byte placeholder objects.

Suggested change

if len(entry.Content) > 0 {

// Ensure directory objects are treated as zero-byte placeholders

if !entry.IsDirectory && len(entry.Content) > 0 {

Copilot · 2025-07-20T16:39:20Z

weed/s3api/s3api_object_handlers.go

+	// Set content length - use FileSize for accuracy, especially for large files
+	contentLength := int64(entry.Attributes.FileSize)


Setting Content-Length based on FileSize for directory objects may be incorrect. S3 directory objects are typically zero-byte objects, and using FileSize could return unexpected values.

Suggested change

// Set content length - use FileSize for accuracy, especially for large files

contentLength := int64(entry.Attributes.FileSize)

// Set content length

var contentLength int64

if entry.IsDirectory {

contentLength = 0 // Directory objects are zero-byte objects

} else {

contentLength = int64(entry.Attributes.FileSize) // Use FileSize for regular objects

}

Copilot · 2025-07-20T16:39:20Z

weed/s3api/s3api_object_handlers.go

+	// Set content type - use stored MIME type or default
+	contentType := entry.Attributes.Mime
+	if contentType == "" {


[nitpick] S3 directory objects typically have a specific content type like 'application/x-directory' or 'httpd/unix-directory' rather than using stored MIME types or defaulting to 'application/octet-stream'.

Suggested change

// Set content type - use stored MIME type or default

contentType := entry.Attributes.Mime

if contentType == "" {

// Set content type - use directory-specific MIME type for directory objects

contentType := entry.Attributes.Mime

if entry.IsDirectory {

contentType = "application/x-directory"

} else if contentType == "" {

read directory object

70de740

gemini-code-assist bot reviewed Jul 20, 2025

View reviewed changes

address comments

3664738

chrislusf requested a review from Copilot July 20, 2025 16:30

This comment was marked as outdated.

Sign in to view

address comments

2736faf

chrislusf requested a review from Copilot July 20, 2025 16:38

Copilot AI reviewed Jul 20, 2025

View reviewed changes

chrislusf marked this pull request as ready for review July 20, 2025 16:40

chrislusf merged commit 41b5bac into master Jul 20, 2025
21 checks passed

chrislusf deleted the read-write-directory-object branch July 20, 2025 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

read directory object #7002

read directory object #7002

Uh oh!

chrislusf commented Jul 20, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 20, 2025

Uh oh!

gemini-code-assist bot Jul 20, 2025

Uh oh!

gemini-code-assist bot Jul 20, 2025

Uh oh!

gemini-code-assist bot Jul 20, 2025

Uh oh!

gemini-code-assist bot Jul 20, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 20, 2025

Uh oh!

Copilot AI Jul 20, 2025

Uh oh!

Copilot AI Jul 20, 2025

Uh oh!

Copilot AI Jul 20, 2025

Uh oh!

Uh oh!

Uh oh!

	return nil, true, nil // Root level directory object, but we don't handle it
	return nil, true, nil // Root level directory object is intentionally skipped because it represents the bucket itself, which is handled elsewhere in the code.

	if len(entry.Content) > 0 {
	// Ensure directory objects are treated as zero-byte placeholders
	if !entry.IsDirectory && len(entry.Content) > 0 {

		// Set content length - use FileSize for accuracy, especially for large files
		contentLength := int64(entry.Attributes.FileSize)

-	// Set content length - use FileSize for accuracy, especially for large files
-	contentLength := int64(entry.Attributes.FileSize)
+	// Set content length
+	var contentLength int64
+	if entry.IsDirectory {
+		contentLength = 0 // Directory objects are zero-byte objects
+	} else {
+		contentLength = int64(entry.Attributes.FileSize) // Use FileSize for regular objects
+	}

Uh oh!

read directory object #7002

read directory object #7002

Uh oh!

Conversation

chrislusf commented Jul 20, 2025

What problem are we solving?

How are we solving the problem?

How is the PR tested?

Checks

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!