news icon indicating copy to clipboard operation
news copied to clipboard

Unify signatures for "read" and "star" endpoints

Open bubelov opened this issue 4 years ago • 2 comments

Read and tick the following checkbox after you have created the issue or place an x inside the brackets ;)

Is your feature request related to a problem? Please describe.

  • Problem 1: Using different signatures makes it harder for third-party apps to support Nextcloud News API. The guidHash property is not a part of RSS nor Atom standards so many reader apps would need to alter their local DB schemas in order to be able to sync "starred" flags with Nextcloud News API.

  • Problem 2: Using different signatures makes no sense and makes it harder for devs to understand the API since the inner logic and the expected outcomes are the same (find an item by an id and change its boolean column).

Describe the solution you'd like

So, starting with the schema (psql):

\d oc_news_items
Column: id
Type: bigint
Nullable: not null

Index: "oc_news_items_pkey" PRIMARY KEY, btree (id)

It seems like id column is unique and it's enough for server to understand which news item client wants to edit.

I checked ItemService and it has the following methods:

public function star($feedId, $guidHash, $isStarred, $userId)
public function read($itemId, $isRead, $userId)

So, we can add an additional read override (or rename the method, if PHP does not allow having multiple methods sharing the same name):

public function star($feedId, $guidHash, $isStarred, $userId)
public function star($itemId, $isStarred, $userId)

public function read($itemId, $isRead, $userId)

After that, it shouldn't be hard to add an additional route while keeping the old one for backward compatibility. Same step can be applied for /multiple endpoint.

Describe alternatives you've considered

Change nothing and wait for someone to re-implement the whole API. It's labor intensive and there have been no progress for years so I wouldn't bet on that to happen any time soon.

Additional context

This issue was also raised in #628 but in the context of re-implementing the whole API.

I have very little experience with PHP and it comes from my college years which is quite a while ago but I'm ready to try to implement this feature and submit it for code review, if there are no objections to implementing it.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

bubelov avatar Sep 18 '20 06:09 bubelov

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Oct 10 '20 08:10 stale[bot]

I see no issue in what you suggest. And we are always open for PRs

Grotax avatar Oct 11 '20 09:10 Grotax