Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add API to change entries #5

Open
anarcat opened this issue Feb 10, 2017 · 10 comments
Open

add API to change entries #5

anarcat opened this issue Feb 10, 2017 · 10 comments

Comments

@anarcat
Copy link
Contributor

anarcat commented Feb 10, 2017

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
Copy link
Contributor Author

anarcat commented Feb 10, 2017

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
Copy link
Owner

Strubbl commented Feb 10, 2017

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
Copy link
Contributor Author

anarcat commented Feb 11, 2017

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
Copy link
Contributor Author

anarcat commented Feb 14, 2017

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
Copy link
Contributor Author

anarcat commented Mar 7, 2017

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

@Strubbl
Copy link
Owner

Strubbl commented Jun 19, 2017

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

@Strubbl Strubbl closed this as completed Jun 19, 2017
@anarcat
Copy link
Contributor Author

anarcat commented Jun 20, 2017

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
Copy link
Owner

Strubbl commented Jun 20, 2017

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.

@Strubbl Strubbl reopened this Jun 20, 2017
@anarcat
Copy link
Contributor Author

anarcat commented Jun 20, 2017

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
Copy link
Owner

Strubbl commented Jun 20, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants