From 83cc6b0f2d2e6ee1df3caf544305ddd373a38d54 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Wed, 26 Nov 2025 09:40:59 -0800
Subject: [PATCH] Fix compilation error and address code review suggestions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Address remaining unresolved comments:

1. **Fix compilation error**: Add missing net/url import
   - filer_multipart.go used url.PathEscape without import
   - Added "net/url" to imports

2. **Fix Location URL formatting** (all 4 occurrences):
   - Add missing slash between bucket and key
   - Use url.PathEscape for bucket names
   - Use urlPathEscape for object keys
   - Handles special characters in bucket/key names
   - Before: http://host/bucketkey
   - After: http://host/bucket/key (properly escaped)

3. **Optimize discovery loop** (O(N*M) → O(N+M)):
   - Use map for existing filers (O(1) lookup)
   - Reduces time holding write lock
   - Better performance with many filers
   - Before: Nested loop for each discovered filer
   - After: Build map once, then O(1) lookups

Changes:
- filer_multipart.go: Import net/url, fix all Location URLs
- filer_client.go: Use map for efficient filer discovery

Benefits:
- Compiles successfully
- Proper URL encoding (handles spaces, special chars)
- Faster discovery with less lock contention
- Production-ready URL formatting
---
 weed/s3api/filer_multipart.go |  7 ++++---
 weed/wdclient/filer_client.go | 17 ++++++++---------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go
index 04cace1b1..274135230 100644
--- a/weed/s3api/filer_multipart.go
+++ b/weed/s3api/filer_multipart.go
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"net/url"
 	"path/filepath"
 	"slices"
 	"sort"
@@ -403,7 +404,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
 		// The latest version information is tracked in the .versions directory metadata
 
 		output = &CompleteMultipartUploadResult{
-			Location:  aws.String(fmt.Sprintf("http://%s/%s%s", r.Host, *input.Bucket, *input.Key)),
+			Location:  aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
 			Bucket:    input.Bucket,
 			ETag:      aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
 			Key:       objectKey(input.Key),
@@ -456,7 +457,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
 
 		// Note: Suspended versioning should NOT return VersionId field according to AWS S3 spec
 		output = &CompleteMultipartUploadResult{
-			Location: aws.String(fmt.Sprintf("http://%s/%s%s", r.Host, *input.Bucket, *input.Key)),
+			Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
 			Bucket:   input.Bucket,
 			ETag:     aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
 			Key:      objectKey(input.Key),
@@ -513,7 +514,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl
 
 		// For non-versioned buckets, return response without VersionId
 		output = &CompleteMultipartUploadResult{
-			Location: aws.String(fmt.Sprintf("http://%s/%s%s", r.Host, *input.Bucket, *input.Key)),
+			Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))),
 			Bucket:   input.Bucket,
 			ETag:     aws.String("\"" + filer.ETagChunks(finalParts) + "\""),
 			Key:      objectKey(input.Key),
diff --git a/weed/wdclient/filer_client.go b/weed/wdclient/filer_client.go
index 63e2807f8..5dd0f08d7 100644
--- a/weed/wdclient/filer_client.go
+++ b/weed/wdclient/filer_client.go
@@ -264,17 +264,16 @@ func (fc *FilerClient) refreshFilerList() {
 	fc.filerAddressesMu.Lock()
 	defer fc.filerAddressesMu.Unlock()
 	
-	// Build list of new filers that aren't in our current list
+	// Build a map of existing filers for efficient O(1) lookup
+	existingFilers := make(map[pb.ServerAddress]struct{}, len(fc.filerAddresses))
+	for _, f := range fc.filerAddresses {
+		existingFilers[f] = struct{}{}
+	}
+	
+	// Find new filers - O(N+M) instead of O(N*M)
 	var newFilers []pb.ServerAddress
 	for addr := range discoveredFilers {
-		found := false
-		for _, existing := range fc.filerAddresses {
-			if existing == addr {
-				found = true
-				break
-			}
-		}
-		if !found {
+		if _, found := existingFilers[addr]; !found {
 			newFilers = append(newFilers, addr)
 		}
 	}