Strubbl / wallabago

Go wrapper for the Wallabag API
GNU General Public License v3.0
11 stars 5 forks source link

add API to change entries #5

Open anarcat opened 7 years ago

anarcat commented 7 years ago

I would need to set read status and similar settings through the API. This is not currently possible as we can only do GET requests. I would need to do PATCH requests. As a workaround, I have implemented a patch that allows me to generate new access tokens in #4 - but that's not ideal.

anarcat commented 7 years ago

In wallabako, I did this through a more generic doAPI request:

// doAPI sends an arbitrary API call to the Wallabag API, getting a
// new token in the process. it returns the body of the response in
// bytes and any possible errors returned by the API, particularly if
// the returned status code is not 200.
func doAPI(method string, url string, body io.Reader) (data []byte, err error) {
    // this is copied from getBodyOfAPIURL(), should probably be
    // factored out

    client := &http.Client{}
    req, err := http.NewRequest(method, url, body)
    req.Header.Add("Authorization", wallabago.GetAuthTokenHeader())
    resp, err := client.Do(req)
    if err != nil {
        return data, err
    }
    defer resp.Body.Close()
    data, err = ioutil.ReadAll(resp.Body)
    if resp.StatusCode != 200 {
        return data, fmt.Errorf("error from the API: %s", resp.Status)
    }
    return data, err
}

(Notice how I pass up the errors instead of printing them right away? That would be a welcome addition elsewhere in the API, btw...)

It's not very pretty, but it works - i use it like this to mark articles as read:

func markAsRead(id int) (err error) {
    log.Printf("marking entry %d as read", id)
    tmp := map[string]string{"archive": "1"}
    body, _ := json.Marshal(tmp)
    _, err = doAPI("PATCH", wallabago.Config.WallabagURL+"/api/entries/"+strconv.Itoa(id)+".json", bytes.NewBuffer(body))
    return err
}
Strubbl commented 7 years ago

This looks good to me. Now, the doAPI() needs to be integrated in the utils and the getBodyOfAPIURL() and postToAPI() need to be reworked to make use of the new doAPI().

Passing up the errors, since this a lib, absolutely makes sense.

I could do that. But not today anymore. If you want, i would appreciate a pull request.

anarcat commented 7 years ago

yes. i think this all needs to be reshuffled... #4 is necessary insofar as I need to build my own requests - if the API was more flexible, i wouldn't quite need it as much (but i still think it's useful to have it there)...

honestly, at this point, i would rather let you finish the job, since i see you are already refactoring the code and adding functionality... i don't want another conflicting change. ;)

anarcat commented 7 years ago

note - the doAPI code changed slightly... i would also recommend to call the function just Do() instead of DoAPI as the API part is quite implicity... :)

anarcat commented 7 years ago

the doAPI code changed again, for the record, just some small changes to return value semantics (avoid crashing on error, for example).

Strubbl commented 7 years ago

different types of http methods should be possible since commit 3fd8d89196dc47a260329c485c30025f61179724

anarcat commented 7 years ago

thanks! i cannot, however, directly use the API as-is, as there are a few things missing:

  1. i set a content-type header when sending the PATCH request, otherwise the query is ignored (see https://gitlab.com/anarcat/wallabako/merge_requests/2): req.Header.Add("Content-Type", "application/json")
  2. i have debugging statements to allow tracing the HTTP requests if debugging is enabled:
    debugln("method, url, body:", method, url, body)
    dump, err := httputil.DumpRequestOut(req, true)
    if err != nil {
        return data, err
    }
    debugf("sending request: %q", dump)
// debugln will log the given arguments using log.Printf only if
// debugging (config.Debug) is enabled
func debugf(fmt string, args ...interface{}) {
    if config.Debug {
        log.Printf(fmt, args...)
    }
}
  1. I return the errors instead of printing error messages, which allows me to handle those in various ways from the API callers:
    resp, err := client.Do(req)
    if err != nil {
        return data, err
    }
  1. i do not check the return value of the ReadAll call - presumably i found that was unnecessary and/or would not trickle up useful error messages to the user, as other checks will appropriately fail. maybe that's an oversight on my part, i forgot

should i open separate issues or pull requests for those?

thanks!

Strubbl commented 7 years ago

If you could open a PR, that would be cool. Otherwise i would try to integrate your feedback. I am reopening this issue until we have it working as expected.

anarcat commented 7 years ago

the easiest is probably step 3, for which i can send a PR. for step 1 and 2, i'm not sure what to do: maybe a callback for debug output? for headings, i guess we could pass a list or something... ideas?

Strubbl commented 7 years ago

for 1 i suggest adding a contentType parameter to the APICall(). If i have some time again i am going to implement the DELETE, PUT and PATCH calls, too. Maybe there is a better common way i did not think about yet.

for 2 i have no idea yet

for 3 i welcome your PR

regarding 4 i would leave the code as it currently is.