Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

API 2.0 #862

Open
BernhardPosselt opened this issue Sep 26, 2015 · 27 comments
Open

API 2.0 #862

BernhardPosselt opened this issue Sep 26, 2015 · 27 comments

Comments

@BernhardPosselt
Copy link
Contributor

In addition to the old APIs I'd like to modernize the API since it seems more complicated than it needs to be and a lot of REST things are used in a wrong way

The main goals are the following

  • Move the shitty version in the URL from vX-X to a semantically versioned URL, e.g. /api/2.0.0/ or remove the version from the URL entirely by sending an HTTP request header (News-Api-Version: 2.0.0)

  • Use HTTP cache headers instead of requiring you to pass a last modified date

  • Crack down on the number of URLs, e.g. marking an item as read and starred would become

    • PATCH /items/1
    {"read": true, "starred": true, "fingerprint": "some_hash"}
  • Actions on multiple resources should be handled at the correct endpoint, e.g. marking multiple items as read and starred would look like this:

    • PATCH /items
    [
      {"id": 123, "read": true, "starred": true, "fingerprint": "some_hash"}, 
      {"id": 124, "read": true, "starred": true, "fingerprint": "some_hash"}
    ]
  • Get rid of the id vs feedid + guidhash difference. I thought this was useful once, but we did not need it

  • Also implement a modified date for feeds and folders so you don't have to fetch everything all over again

  • Provide helper routes to make syncing a breeze. You normally have three usecases, those should be reflected by three routes

    • Initial sync: Fetch feeds, folders, unread + starred items: GET /api/2.0.0/sync
    • Sync local changes (no return appart from errors because the next GET request fetches all changed items anyways): POST /api/2.0.0/sync
    {"items": [{"id": 1, "read": true, "fingerprint": "some_hash"}]}

    The fingerprint is a hash over the title, url, content and enclosures and if it is the same in the current database, only the status (read/starred) will be returned to the client.

    • Checking for new contents without changes:: GET /api/2.0.0/sync + http cache headers

The response will be the changes.

Furthermore creating feeds/folders that already exist should respond with the existing object instead of just returning an error message. This makes it very easy to create folders: you just submit the contents and if you get a 409 you just use the returned contents.

Ideas? Suggestions?

@David-Development @phedlund @icewind1991 @jangernert @cosenal

@jangernert
Copy link

Sounds very nice :)
Especially the removal of guidhash. Where could have something like this be useful?

Is it really necessary to only sync the changes of the feeds and folders? At least in my experience syncing feeds and folders is super fast anyway.

@BernhardPosselt
Copy link
Contributor Author

@jangernert We used to update items by adding new items to the database so we could not use the id to identify the article (also think about the autopaging, the article would have been at the bottom) and starring items should always hit the right article

As for syncing feeds/folders: there are people with 200 feeds and 20 folders, dont ask me why :D

Since almost all API users are mobile apps, we want to reduce the payload as much as possible

@phedlund
Copy link
Contributor

The one thing I have found most cumbersome is to sync the items of a folder. First the feeds of that folder need to be retrieved and then iterated over resulting in several server calls. On the other hand adding a folder id to each item is maybe not so smart either.

@mk-conn
Copy link

mk-conn commented Jan 3, 2016

Sounds cool to get a new API :).
If I may add something to this... I quite don't understand why one has to fetch items first, iterate over it to detect which feed they belong to and than iterate over feeds to detect which folder they belong to.. For my understanding it must be the other way around... e.g.

{folders: {id: 1, name: "bla", feeds: [2,3,5]}, {id: 2, name:"fasel", feeds: []}}
// same for feeds... 

Items and feeds could/should still have the feedId/folderId. This would reduce initial load as items could be loaded upon folder/feed-request and not all at once..

@phedlund
Copy link
Contributor

phedlund commented Jan 4, 2016

@mk-conn I like your idea.

@martinrotter
Copy link

+1 For any ideas which support removing of weird parameters (guidHash, feedId) in marking multiple items as (un)starred. This is just nonsense. When you mark as (un)read, you use message ID, which is correct (what else identificator you should use, right?). When you (un)star you suddenly use guidHash. Wow.

So any client had to keep three values for each message instead of only one.

Looking forward to 2.0 API :), great work guys.

@jangernert
Copy link

Btw: I noticed that currently it is not possible to subscribe to feeds which require authentication via API. No idea if ownCloud News supports these kind of feeds in general (I am not subscribed to any). But once ownCloud News does support them, it would be nice to also be able subscribe to them via the API :)

@BernhardPosselt
Copy link
Contributor Author

@jangernert sure ;)

@BernhardPosselt
Copy link
Contributor Author

The thing about feeds that require authentication is that we would need to save the credentials in plain text (which is not desirable at all) and OAuth is probably not very common.

@mk-conn
Copy link

mk-conn commented Mar 4, 2016

@BernhardPosselt Is it not possible to store credentials encrypted somehow?

@BernhardPosselt
Copy link
Contributor Author

Nope, how would the update work ;)?

@mk-conn
Copy link

mk-conn commented Mar 6, 2016

Hm, maybe I don't really get the problem.. but during/before update just decrypt credentials?

@BernhardPosselt
Copy link
Contributor Author

If you can just decrypt them then you've achieved nothing but more complexity ;)

@BernhardPosselt
Copy link
Contributor Author

BernhardPosselt commented Apr 26, 2016

I've had some time to work on the spec which is no located in the docs folder, feedback would be awesome:
https://github.com/owncloud/news/tree/master/docs/developer/External-Api.md
Not everything is specified yet.

@BernhardPosselt
Copy link
Contributor Author

BernhardPosselt commented Apr 29, 2016

Ok, finished the spec, please comment on https://github.com/owncloud/news/tree/master/docs/developer/External-Api.md

@GisoBartels
Copy link

Great work, the new API looks very promising.

Though one point irritates me a bit. The "data" wrapper object seems to be unnecessary and complicates parsing.
From a RESTful point of view, I would expect to get the the object (or collection) I queried, instead of a wrapper object with additional but empty properties. Also a holder object indicating the type is not needed, as I already know which type I have queried (except for the sync resource, which has mixed types).
And I think it is enough to indicate an error with the HTTP status code, so the client knows it has to handle a special response body.

If you want to make the API even more RESTful, you could consider following the HATEOAS principle (i.e. by using JSON-LD).

@BernhardPosselt
Copy link
Contributor Author

Getting rid of the data object is reasonable, however as an api user how would you distinguish between the different validation errors? Most of the status codes are transport related. Current solution is to only add an error object for http 400

@BernhardPosselt
Copy link
Contributor Author

As for hateoas: I think this is not needed for a simple sync api and would require me to implement a lot more routes than actually needed. If we later on discover that we need it, we can always build on the current proposal

@GisoBartels
Copy link

GisoBartels commented May 8, 2016

Which kind of validation do you mean?
If the request body's validation fails HTTP 400, 415 or 422 with a detailed error message in body would be reasonable.

@BernhardPosselt
Copy link
Contributor Author

Did you take a look at the feed creation route? There are multiple things that can go wrong: website does not exist, http basic auth does not work, URL does not exist etc. An error message is fine but sometimes you want to match an error code to show a custom translated message (eg client has a different locale than the server)

PS: 415 is basically about content payload not being able to be handled and not validation, 422 is better but part of WebDAV and would also be used in more than one error case. What I'm trying to say is that you can't use http status codes for everything

@BernhardPosselt
Copy link
Contributor Author

Another use case for error codes would be to show the user and password form again if feed creation failed due to authentication issues.

@BernhardPosselt
Copy link
Contributor Author

@GisoBartels removed the data object nesting from the spec

@GisoBartels
Copy link

I didn't mean to use a different HTTP status code for each error or to omit the additional error code. That's fine.
I meant for a HTTP 2xx code, I expect something went successful, i.e. a feed was created and I get the result. In this case the feed as payload, so no need for an error object.
In case of an error (4xx/5xx), I expect a payload with details about the error, but not a feed object.
And for my use case, I don't really care about which HTTP error codes are used for the errors.

@GisoBartels
Copy link

What do you think about the second wrapper object?
I.e. having "folder" property holding the actual folder object instead of being the root object.
Is it necessary to indicate the type here? Maybe the resource itself is enough?

@BernhardPosselt
Copy link
Contributor Author

Both are specified like you mentioned already ;)

Yes wrapping the stuff in a folder property is the way to go since it's extensible

@BernhardPosselt
Copy link
Contributor Author

I've added additional docs on:

  • how the API level should be detected
  • how you find out if items were deleted and how to react to it
  • what you need to do after you've called a delete/patch/post route for feeds and folders with return codes 200 or 404

@BernhardPosselt
Copy link
Contributor Author

BernhardPosselt commented May 29, 2016

Moved the API docs into a PR so it can be discussed more easily #1000

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

No branches or pull requests

6 participants