WIP: confluence: replace gopencils with resty/v3#723
WIP: confluence: replace gopencils with resty/v3#723mrueg wants to merge 1 commit intokovetskiy:masterfrom
Conversation
mrueg
changed the title
mrueg
changed the title
This migration replaces the unmaintained gopencils library with resty v3. Key changes include: - Updated API struct and NewAPI to use resty.Client. - Replaced custom multipart form handling with resty's native support (SetFileReader, SetFormData). - Updated all API methods to use resty's fluent request API. - Implemented resty.Logger for trace logging. - Updated error handling to utilize resty.Response and provided more detailed error output.
There was a problem hiding this comment.
Pull request overview
Migrates the Confluence client implementation from the unmaintained gopencils library to resty.dev/v3, updating request construction, multipart uploads, and error handling to use Resty’s client and fluent request API.
Changes:
- Replace
gopencils.Resourceusage withresty.ClientandR()-based requests across Confluence API methods. - Switch attachment uploads to Resty’s native multipart helpers (
SetFileReader,SetFormData). - Update tracing/debug logging and error formatting to use
resty.Response.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
go.mod |
Drops gopencils and adds resty.dev/v3 dependency. |
go.sum |
Removes gopencils checksums and adds resty.dev/v3 checksums. |
confluence/api.go |
Refactors all Confluence HTTP calls to Resty and updates logging/error handling accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| u, _ := url.Parse(api.rest.BaseURL()) | ||
| if strings.HasSuffix(u.Host, "jira.com") || strings.HasSuffix(u.Host, "atlassian.net") { |
There was a problem hiding this comment.
url.Parse(api.rest.BaseURL()) ignores the returned error. If parsing fails, u will be nil and u.Host will panic; even for partially-parsable URLs this can silently select the wrong (cloud vs server) codepath. Handle the parse error (and/or fall back to api.BaseURL) before reading u.Host.
| u, _ := url.Parse(api.rest.BaseURL()) | |
| if strings.HasSuffix(u.Host, "jira.com") || strings.HasSuffix(u.Host, "atlassian.net") { | |
| baseURL := api.rest.BaseURL() | |
| u, parseErr := url.Parse(baseURL) | |
| host := baseURL | |
| if parseErr == nil && u != nil && u.Host != "" { | |
| host = u.Host | |
| } | |
| if strings.HasSuffix(host, "jira.com") || strings.HasSuffix(host, "atlassian.net") { |
| _, err := api.rest.R(). | ||
| SetQueryParam("cql", fmt.Sprintf("user.fullname~%q", name)). | ||
| SetResult(&response). | ||
| Get("search/user") | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This request ignores the HTTP status code. With resty, non-2xx responses typically don’t surface as err, so a 401/403/etc can be misreported later as “user not found”. Capture the response (resp := ...Get(...)) and call newErrorResponseNotOK(resp) when resp.StatusCode() != http.StatusOK.
| _, err := api.rest.R(). | ||
| SetResult(&user). | ||
| Get("user/current") | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This request ignores the HTTP status code. With resty, err usually only covers transport/unmarshal errors, so authentication/permission failures (e.g., 401/403) could be returned as a nil error and an empty/partial user. Capture the response and check resp.StatusCode() == http.StatusOK (otherwise return newErrorResponseNotOK(resp)).
| func (api *API) RestrictPageUpdatesCloud( | ||
| page *PageInfo, | ||
| allowedUser string, | ||
| ) error { | ||
| user, err := api.GetCurrentUser() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var result interface{} | ||
|
|
||
| request, err := api.rest. | ||
| Res("content"). | ||
| Id(page.ID). | ||
| Res("restriction", &result). | ||
| Post([]map[string]interface{}{ | ||
| resp, err := api.rest.R(). | ||
| SetBody([]map[string]interface{}{ | ||
| { | ||
| "operation": "update", | ||
| "restrictions": map[string]interface{}{ | ||
| "user": []map[string]interface{}{ | ||
| { | ||
| "type": "known", | ||
| "accountId": user.AccountID, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| }). | ||
| SetResult(&result). | ||
| Post("content/" + page.ID + "/restriction") |
There was a problem hiding this comment.
RestrictPageUpdatesCloud ignores the allowedUser argument (it always restricts to the current user’s accountId). This is surprising given the API and differs from the server implementation. Either use allowedUser (likely by resolving it to an accountId) or rename/remove the parameter to avoid misleading callers.
Uh oh!
There was an error while loading. Please reload this page.