-
Notifications
You must be signed in to change notification settings - Fork 6
Description
Preliminary checks
- I am using the latest version, or the latest version that corresponds to my Oxide installation.
- There is no open issue that reports the same problem.
What was the expected behaviour
The expected behavior is that the Oxide API client will respect the full path of the provided base URL. This can be achieved using URL.JoinPath, but I wanted to open this issue instead of a pull request to discuss the desired behavior first.
Here's a test case showing the desired behavior and how the current code currently fails.
func TestURLPath(t *testing.T) {
tt := map[string]struct {
baseURL string
relativePath string
expectedFinalURL string
}{
"api behind a context path": {
baseURL: "https://api.oxide.computer/some/api/path",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks",
},
"api behind a context path with trailing slash": {
baseURL: "https://api.oxide.computer/some/api/path/",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks",
},
"api behind a context path with query parameters": {
baseURL: "https://api.oxide.computer/some/api/path?foo=bar",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks?foo=bar",
},
"api at the root path": {
baseURL: "https://api.oxide.computer",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks",
},
"api at the root path with trailing slash": {
baseURL: "https://api.oxide.computer/",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks",
},
"api at the root path with query parameters": {
baseURL: "https://api.oxide.computer?foo=bar",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks?foo=bar",
},
}
for testName, testCase := range tt {
t.Run(testName, func(t *testing.T) {
baseURL, err := parseBaseURL(testCase.baseURL)
if err != nil {
t.Fatalf("failed parsing base url: %v", err)
}
finalURL := resolveRelative(baseURL, testCase.relativePath)
assert.Equal(t, testCase.expectedFinalURL, finalURL)
})
}
}What is the current behaviour and what actions did you take to get there
A call to NewClient calls parseBaseURL to verify whether the passed Oxide API host is a valid URL.
Line 91 in 20c490d
| host, err := parseBaseURL(host) |
Each API method of this client calls resolveRelative to build the final URL for its API request.
Lines 11 to 20 in 20c490d
| // resolveRelative combines a url base with a relative path. | |
| func resolveRelative(basestr, relstr string) string { | |
| u, _ := url.Parse(basestr) | |
| rel, _ := url.Parse(relstr) | |
| u = u.ResolveReference(rel) | |
| us := u.String() | |
| us = strings.Replace(us, "%7B", "{", -1) | |
| us = strings.Replace(us, "%7D", "}", -1) | |
| return us | |
| } |
Under the hood, resolveRelative uses URL.ResolveReference to build the final URL. When URL.ResolveReference is passed a path with a leading /, the resulting final URL is an absolute URL from the root of the base URL. This means that users of this Oxide API client cannot have their API served from any URL path other than the root.
For example, one cannot use a base URL of https://api.oxide.computer/some/api/path because an API call to /v1/system/hardware/disks would result in a final URL of https://api.oxide.computer/v1/system/hardware/disks, dropping the /some/api/path part of the URL.
Go SDK version
latest
Operating system
Fedora 39
Anything else you would like to add?
No response