feat: add virtual-hosted style support #37
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request attempts to add support for virtual-hosted style URLs in S3 operations (addressing issue #36), which would allow using bucket names as part of the hostname instead of the path. However, the implementation has critical flaws that prevent it from working as intended.
Changes:
- Added
UsePathStyleboolean field to the S3 struct (defaulting to true for backward compatibility) - Added
SetPathStyle()andSetVirtualHostedStyle()methods to configure URL style - Modified bucket validation logic across 24+ functions to conditionally require bucket names
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| simples3.go | Added UsePathStyle field and setter methods; improved trailing slash handling |
| helpers.go | Added documentation for virtual-hosted style but implementation missing |
| tagging.go | Modified bucket validation in 3 tagging functions |
| object.go | Modified bucket validation in 5 object operation functions |
| multipart.go | Modified bucket validation in 6 multipart upload functions |
| list.go | Modified bucket validation in 2 list functions |
| bucket.go | Modified bucket validation in 7 bucket operation functions |
Comments suppressed due to low confidence (1)
helpers.go:41
- The getURL function does not implement virtual-hosted style URL construction despite the documentation claiming it does. The function always constructs URLs the same way regardless of the UsePathStyle flag value.
For virtual-hosted style to work, when UsePathStyle is false and no custom Endpoint is set, the URL should be constructed as "https://{bucket}.s3.{region}.amazonaws.com/{key}" instead of "https://s3.{region}.amazonaws.com/{bucket}/{key}". The current implementation always uses the same URIFormat regardless of the UsePathStyle setting.
You need to modify this function to check the UsePathStyle flag and construct URLs differently based on its value. For virtual-hosted style, the bucket should be part of the hostname, not the path.
func (s3 *S3) getURL(path string, args ...string) (uri string) {
if len(args) > 0 {
path += "/" + strings.Join(args, "/")
}
// need to encode special characters in the path part of the URL
encodedPath := encodePath(path)
if len(s3.Endpoint) > 0 {
uri = s3.Endpoint + "/" + encodedPath
} else {
uri = fmt.Sprintf(s3.URIFormat, s3.Region, encodedPath)
}
return uri
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Validate input | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| if input.Bucket == "" && s3.UsePathStyle { | |
| if input.Bucket == "" { |
| func (s3 *S3) PutObjectTagging(input PutObjectTaggingInput) error { | ||
| // Validate required fields | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
The validation logic is inverted. This condition checks if the bucket is empty AND UsePathStyle is true, then returns an error saying "bucket name is required". However, the logic should be: when UsePathStyle is true (path-style), bucket is always required in the input. When UsePathStyle is false (virtual-hosted style), the bucket should be in the Endpoint, not in the input parameter.
The correct validation should be:
- If UsePathStyle is true AND input.Bucket is empty, return error (path-style requires bucket in path)
- If UsePathStyle is false, bucket can be empty in the input (it should be in the Endpoint for virtual-hosted style)
This same logic error appears throughout all the validation changes in this PR.
| func (s3 *S3) DeleteObjects(input DeleteObjectsInput) (DeleteObjectsOutput, error) { | ||
| // Validate required fields | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| func (s3 *S3) PutObjectAcl(input PutObjectAclInput) error { | ||
| // Validate input | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| if input.Bucket == "" && s3.UsePathStyle { | |
| if input.Bucket == "" { |
| // FileUploadMultipart handles the entire multipart upload workflow | ||
| func (s3 *S3) FileUploadMultipart(input MultipartUploadInput) (MultipartUploadOutput, error) { | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| // InitiateMultipartUpload initiates a multipart upload and returns an upload ID | ||
| func (s3 *S3) InitiateMultipartUpload(input InitiateMultipartUploadInput) (InitiateMultipartUploadOutput, error) { | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| // UploadPart uploads a single part for a multipart upload | ||
| func (s3 *S3) UploadPart(input UploadPartInput) (UploadPartOutput, error) { | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| // CompleteMultipartUpload completes a multipart upload | ||
| func (s3 *S3) CompleteMultipartUpload(input CompleteMultipartUploadInput) (CompleteMultipartUploadOutput, error) { | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| func (s3 *S3) GetBucketVersioning(bucket string) (GetBucketVersioningOutput, error) { | ||
| // Validate input | ||
| if bucket == "" { | ||
| if bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
| func (s3 *S3) PutBucketLifecycle(input PutBucketLifecycleInput) error { | ||
| // Validate input | ||
| if input.Bucket == "" { | ||
| if input.Bucket == "" && s3.UsePathStyle { |
There was a problem hiding this comment.
Same validation logic error. The condition should be if input.Bucket == "" && s3.UsePathStyle to require bucket only when using path-style URLs.
|
Copilot seems to have gone mad, generating a bunch of contradictory suggestions; just ignore it. |
1 participant
fix #36