Skip to content

Commit

Permalink
fix: Build the URL using url.JoinPath instead of path.Join (#249)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

There is a weird bug in setting a URL path directly, where it will
escape some characters, but not all.

This [go playground](https://go.dev/play/p/QnMDXnU9lDq) example shows
it. The unescaped value is used directly, the escaped value double
escapes some of the characters.

This PR uses url.PathJoin instead, which is intended to handle URLs.

## Short description of the changes
- Update the URL builder to use url.JoinPath and return any errors
- Update Transmission to use updated builder func and handle error like
before
- Update tests to handle errors
  • Loading branch information
MikeGoldsmith authored Jun 13, 2024
1 parent 44b3e71 commit 9f4f37c
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 11 deletions.
14 changes: 5 additions & 9 deletions transmission/transmission.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"io/ioutil"
"net/http"
"net/url"
"path"
"runtime"
"strings"
"sync"
Expand Down Expand Up @@ -399,7 +398,7 @@ func (b *batchAgg) fireBatch(events []*Event) {
}
}

url, err := url.Parse(apiHost)
url, err := buildRequestURL(apiHost, dataset)
if err != nil {
end := time.Now().UTC()
if b.testNower != nil {
Expand All @@ -421,9 +420,6 @@ func (b *batchAgg) fireBatch(events []*Event) {
return
}

// build the HTTP request
url.Path = buildReqestPath(url.Path, dataset)

// One retry allowed for connection timeouts.
var resp *http.Response
for try := 0; try < 2; try++ {
Expand All @@ -437,9 +433,9 @@ func (b *batchAgg) fireBatch(events []*Event) {
// Pass the underlying bytes.Reader to http.Request so that
// GetBody and ContentLength fields are populated on Request.
// See https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/net/http/request.go;l=898
req, err = http.NewRequest("POST", url.String(), &reader.Reader)
req, err = http.NewRequest("POST", url, &reader.Reader)
} else {
req, err = http.NewRequest("POST", url.String(), reqBody)
req, err = http.NewRequest("POST", url, reqBody)
}
req.Header.Set("Content-Type", contentType)
if zipped {
Expand Down Expand Up @@ -743,6 +739,6 @@ type nower interface {
Now() time.Time
}

func buildReqestPath(existingPath, dataset string) string {
return path.Join(existingPath, "/1/batch", url.PathEscape(dataset))
func buildRequestURL(apiHost, dataset string) (string, error) {
return url.JoinPath(apiHost, "/1/batch", url.PathEscape(dataset))
}
5 changes: 3 additions & 2 deletions transmission/transmission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1395,7 +1395,8 @@ func TestBuildRequestPath(t *testing.T) {
}

for _, tc := range testCases {
path := buildReqestPath("", tc.datasetName)
assert.Equal(t, tc.expectedPath, path)
url, err := buildRequestURL("http://fakeHost:8080", tc.datasetName)
assert.NoError(t, err)
assert.Equal(t, "http://fakeHost:8080"+tc.expectedPath, url)
}
}

0 comments on commit 9f4f37c

Please sign in to comment.