Skip to content

Commit

Permalink
Fix nil pointer bug caused by trying to list namespaces with no coll URL
Browse files Browse the repository at this point in the history
Previously, the code in `listHttp` didn't check whether or not the director response it had
actually contained a valid collections URL. Now it checks whether the URL is populated,
and if not returns an error with a hint.

I've also added very basic unit test coverage for this, but noticed both `listHttp` and
`statHttp` could use a lot more. Rather than do that in this bug fix PR, I'll open a followup
ticket that targets only these two functions for unit test development.
  • Loading branch information
jhiemstrawisc authored and turetske committed Nov 5, 2024
1 parent 5485ba0 commit 50db1e2
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 1 deletion.
4 changes: 4 additions & 0 deletions client/handle_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -2591,6 +2591,10 @@ func (te *TransferEngine) walkDirUpload(job *clientTransferJob, transfers []tran
// This function performs the ls command by walking through the specified collections and printing the contents of the files
func listHttp(remoteUrl *pelican_url.PelicanURL, dirResp server_structs.DirectorResponse, token *tokenGenerator) (fileInfos []FileInfo, err error) {
// Get our collection listing host
if dirResp.XPelNsHdr.CollectionsUrl == nil {
return nil, errors.Errorf("Collections URL not found in director response. Are you sure there's an origin for prefix %s that supports listings?", dirResp.XPelNsHdr.Namespace)
}

collectionsUrl := dirResp.XPelNsHdr.CollectionsUrl
log.Debugln("Collections URL: ", collectionsUrl.String())

Expand Down
58 changes: 58 additions & 0 deletions client/handle_http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (

"github.com/pelicanplatform/pelican/config"
"github.com/pelicanplatform/pelican/error_codes"
"github.com/pelicanplatform/pelican/pelican_url"
"github.com/pelicanplatform/pelican/server_structs"
"github.com/pelicanplatform/pelican/server_utils"
"github.com/pelicanplatform/pelican/test_utils"
Expand Down Expand Up @@ -930,3 +931,60 @@ func TestNewTransferEngine(t *testing.T) {
assert.NoError(t, err)
})
}

func TestListHttp(t *testing.T) {
type test struct {
name string
pUrl *pelican_url.PelicanURL
dirResp server_structs.DirectorResponse
expectedError string
}
tests := []test{
{
name: "valid-collections-url",
pUrl: &pelican_url.PelicanURL{
Scheme: "pelican",
Host: "something.com",
Path: "/foo/bar/baz",
},
dirResp: server_structs.DirectorResponse{
XPelNsHdr: server_structs.XPelNs{
RequireToken: false,
Namespace: "/foo/bar",
CollectionsUrl: &url.URL{
Scheme: "https",
Host: "collections.example.com",
},
},
},
expectedError: "no such host", // punt on setting up a real server, and accept this "success" looks like a connection error
},
{
name: "no-collections-url",
pUrl: &pelican_url.PelicanURL{
Scheme: "pelican",
Host: "something.com",
Path: "/foo/bar/baz",
},
dirResp: server_structs.DirectorResponse{
XPelNsHdr: server_structs.XPelNs{
RequireToken: false,
Namespace: "/foo/bar",
},
},
expectedError: "Collections URL not found in director response. Are you sure there's an origin for prefix /foo/bar that supports listings?",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
_, err := listHttp(test.pUrl, test.dirResp, nil)
if test.expectedError != "" {
assert.Error(t, err)
assert.Contains(t, err.Error(), test.expectedError)
} else {
assert.NoError(t, err)
}
})
}
}
2 changes: 1 addition & 1 deletion client/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func DoList(ctx context.Context, remoteObject string, options ...TransferOption)

fileInfos, err = listHttp(pUrl, dirResp, token)
if err != nil {
return nil, errors.Wrap(err, "failed to do the list")
return nil, errors.Wrap(err, "failed to perform list request")
}

return fileInfos, nil
Expand Down

0 comments on commit 50db1e2

Please sign in to comment.