owncloud-archive / news

:newspaper: News app for ownCloud
GNU Affero General Public License v3.0
290 stars 106 forks source link

API 2.0 #862

Open BernhardPosselt opened 9 years ago

BernhardPosselt commented 9 years ago

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

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 8 years ago

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 commented 8 years ago

@mk-conn I like your idea.

martinrotter commented 8 years ago

+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 commented 8 years ago

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 commented 8 years ago

@jangernert sure ;)

BernhardPosselt commented 8 years ago

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 commented 8 years ago

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

BernhardPosselt commented 8 years ago

Nope, how would the update work ;)?

mk-conn commented 8 years ago

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

BernhardPosselt commented 8 years ago

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

BernhardPosselt commented 8 years ago

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 commented 8 years ago

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

GisoBartels commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@GisoBartels removed the data object nesting from the spec

GisoBartels commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

I've added additional docs on:

BernhardPosselt commented 8 years ago

Moved the API docs into a PR so it can be discussed more easily https://github.com/owncloud/news/pull/1000