s3manager Uploader returns URL encoded Location for bigger files (multipart upload)

[mtestrot]

Please fill out the sections below to help us address your issue.

Version of AWS SDK for Go?

v1.10.6

Version of Go (go version)?

go1.8 windows/amd64

What issue did you see?

The upload function from the s3manager returns an URL encoded location (UploadOutput.location) if a bigger file (>5MB) is uploaded. For smaller files (<5MB) the location is not URL encoded which in my opinion is the right behaviour.
I guess the behaviour is different for multipart uploads and singlepart uploads.

Steps to reproduce

// filesize = 5 * 1024 * 1024 Byte
uploader = s3manager.NewUploader(sess)
output, err := uploader.Upload(&s3manager.UploadInput{
Bucket: aws.String("mybucket"),
Key: aws.String("1234"),
Body: bufio.NewReader(file),
})
fmt.Println(output.Location) // should NOT be URL encoded

[xibz]

Hello @mtestrot, thank you for reaching out to us. We are aware of this issue and currently have it in our backlog, #645. We are more than willing to take PRs too! I will go ahead and bring this up in our next team meeting. For now, I'll mark this as a feature request.

[mtestrot]

Ok. Thanks for your quick reply. I didn't find the existing issue, so sorry for posting it again.

[xibz]

@mtestrot - What is your use case for the Location field? Are you doing anything with that field?

[mtestrot]

I store and pass around the location as a reference to retrieve the file later on. So everything the using application has to know is the reference in order to Http GET the object. For the using application It's unnecessary to know the construction rule for the url and it's unnecessary to know that the object is stored in S3.

[xibz]

@mtestrot - Other than the inconsistency of the escaping, does the URL work?

[mtestrot]

Yes. The URL works as soon as we do a string replace before passing it around:
reference = strings.Replace(output.Location,"%2F","/",-1)

Of course there will be a Problem if the URL contains a part which has to be URL encoded. For example if a path-segment from the S3 Object Key is build from a URL. But this is currently no use case for us. So the workaround works.

[xibz]

@mtestrot - Thank you for the information. I will go ahead and try to reproduce this on my end as well. Can you provide a code snippet trying to reuse the Location value? In particular, the section that makes the request and the section that replaces the %2F.

[mtestrot]

Hi,

I gathered some code for an isolated test case:

package awsissue

import (
	"os"
	"time"
	"fmt"
	"testing"
	"io/ioutil"
	"log"
	"math/rand"
	"github.com/aws/aws-sdk-go/aws"
	"bufio"
	"github.com/aws/aws-sdk-go/service/s3/s3manager"
	"github.com/aws/aws-sdk-go/aws/session"
	"github.com/aws/aws-sdk-go/aws/signer/v4"
	"net/http"
)

var (
	sess     = session.Must(session.NewSession())
	uploader = s3manager.NewUploader(sess)
)

func NewTempFile(size int, parentDir string, blockSize int) string {
	tmpfile, err := ioutil.TempFile(parentDir, "test")
	if err != nil {
		log.Fatal(err)
	}

	b := make([]byte, blockSize)
	for i := 0; i < size; i++ {
		for x := 0; x < blockSize; x++ {
			b[x] = byte(rand.Int())
		}

		if _, err := tmpfile.Write(b); err != nil {
			log.Fatal(err)
		}
	}
	if err := tmpfile.Close(); err != nil {
		log.Fatal(err)
	}

	return tmpfile.Name()
}

func store(filepath string) (string, error) {
	file, err := os.Open(filepath)
	defer file.Close()
	if err != nil {
		return "", err
	}

	output, err := uploader.Upload(&s3manager.UploadInput{
		Bucket: aws.String("bucket"),
		Key:    aws.String(fmt.Sprintf("a/b/%d", time.Now().UnixNano())),
		Body:   bufio.NewReader(file),
	})

	if err != nil {
		return "", err
	}

	reference := output.Location
	// Uncomment the following line to fix the issue
	//reference = strings.Replace(reference, "%2F", "/", -1) // Workaround for https://github.com/aws/aws-sdk-go/issues/1385
	return reference, nil
}

func TestLocation(t *testing.T) {
	// at least 5MB to reproduce the issue
	filepath := NewTempFile(5, "", 1024*1024)
	defer os.Remove(filepath)

	location, err := store(filepath)
	if err != nil {
		t.Fatal(err)
	}

	req, err := http.NewRequest("GET", location, nil)
	if err != nil {
		t.Fatal(err)
	}
	signer := v4.NewSigner(sess.Config.Credentials)
	_, err = signer.Sign(req, nil, "s3", *sess.Config.Region, time.Now())
	if err != nil {
		t.Fatal(err)
	}
	client := &http.Client{}
	resp, err := client.Do(req)
	if err != nil {
		t.Fatal(err)
	}
	if resp.StatusCode != 200 {
		t.Fatalf("Status was %d", resp.StatusCode)
	}
}

[xibz]

@mtestrot thank you for the example. I'll go ahead and discuss with the other SDKs to have some consistency on dealing with this issue.

[xibz]

@mtestrot - Taking a look at the code that you provided, can you instead just call get object instead of trying to build the request yourself? Why is it that you are trying to do this? What's the specific use case?

[mtestrot]

Hi,
sorry for the late response. I have been out of office.

The reason I'm building the request by myself is the reduced dependency for the GET part of the TestLocation function. In production the program which stores the S3 object is different from the program which retrieves/gets the object. So the GET part - apart from the signer - has no dependency on the sdk.