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

Alternative add totalCount extension #107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lachlan-00
Copy link
Member

As discussed in #16

Adda totalCount to the following

Endpoints
    getalbumlist
    getalbumlist2
    getnewestpodcasts
    getsongsbygenre
    search3

Responses
    albumList
    albumList2
    newestPodcasts
    searchResult3
    songsByGenre

note Search3 responses prefix totalCount for each object type (totalAlbumCount, totalArtistCount, totalSongCount)

add for `offset` and `count` parameters that make sense

* Endpoints
  * getalbumlist
  * getalbumlist2
  * getnewestpodcasts
  * getsimilarsongs
  * getsimilarsongs2
  * getsongsbygenre
  * gettopsongs
  * search3

* Responses
  * albumList
  * albumList2
  * newestPodcasts
  * searchResult3
  * similarSongs
  * similarSongs2
  * songsByGenre
  * topSongs

Search3 responses prefix totalCount for each object type

add note for unknown, uncalculated, unsupported values

tag [OS]

Update searchResult3.md

Update songsByGenre.md
simplified approach
Copy link

netlify bot commented Oct 17, 2024

Deploy Preview for opensubsonic ready!

Name Link
🔨 Latest commit e6ab0c0
🔍 Latest deploy log https://app.netlify.com/sites/opensubsonic/deploys/671076a723f71e00087dfafc
😎 Deploy Preview https://deploy-preview-107--opensubsonic.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@epoupon
Copy link
Member

epoupon commented Oct 17, 2024

@deluan will you actually implement totalCount for search3?

Copy link
Contributor

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As search3 has counts, search2 should also have totalArtistCount/totalAlbumCount/totalSongCount (since there are servers which also provide separate search2/search3)

@deluan
Copy link
Member

deluan commented Oct 17, 2024

@deluan will you actually implement totalCount for search3?

I haven't, but I think it makes sense to be in the spec, as long as it can be set to -1

@deluan
Copy link
Member

deluan commented Oct 17, 2024

As search3 has counts, search2 should also have totalArtistCount/totalAlbumCount/totalSongCount (since there are servers which also provide separate search2/search3)

Yeah, I agree

@dweymouth
Copy link
Member

I really think we need a parameter for the client to request totalCount to be computed or not, and it may make sense to save it for the full redesign of paginated endpoints. The problem I see is:

  • Paginated clients generally would need total count for everything, including search3 and getAlbumList2 with various combinations of filters (eg year range plus genre) so they know how many pages to show in the pagination UI control.
  • Infinite scrolling clients generally do not need total count for anything, as they fetch and append the next batch of content as the user scrolls near the bottom. Calculating total count for "expensive" things like getAlbumList2 with filters will just slow down queries unnecessarily
  • The server deciding when and when not to return -1 means paginated clients that need it can't really rely on it being present, meanwhile non-paginated clients have to suffer the perf drop whenever a server does decide to include it.

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

Reposting here a remark from the other one.
It's also missing search to be consistent with all paginating endpoints (But since it's deprecated we may probably ignore it)

But getnewestpodcasts is not a paginating end point there's no consistency to have it if we don't on other ones.

For the new param again, IMO this is going backward on the purpose of OS and would require an extension.

  1. While most server currently probably ignore unknown query parameter this is not in the spec, so sending the parameter randomly to a server will have unknown results. (If 100% of the current OS servers currently ignore unknown parameters I guess we could eventually precise this in the spec to fill that gap)
  2. All the discussions from last couple of days would have be more than enough to define the new proper endpoints.
  3. For the moment servers have mostly said that the perf impact for some endpoints is too high so in all case won't implement it.
  4. Having a parameter without an extension means that there's exactly the same issue client side to handle.

So the only "issue" with this temporary solution is the cost it triggers, and since servers would only put it when the cost is low this is not really a problem.

Now let's again take your example:

getAlbumList2 with various combinations of filters (eg year range plus genre)

With current API this is the only thing that can be done, you can't search and order, you can't sort by anything, there's so many limitations in that API that pagination with the filters is quite frankly too limited to be really usefull.

Hence the need for the new proper endpoints that are not complicated to build, it just requires someone to gather all the needs from the discussion post and start consolidating.
Exactly the same job I did for all the new fields.

Then building the API is actually quite simple.

@dweymouth
Copy link
Member

So the only "issue" with this temporary solution is the cost it triggers, and since servers would only put it when the cost is low this is not really a problem.

This is a problem if servers choose to put it when the cost is high, and potentially a problem if servers don't put it for some requests where a paginated client required it. Maybe we have to wait to hear from a paginated client dev if it would break the usefulness for them if -1 was returned for search3 or getAlbumList2 with any filters applied.

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

For the second part again:
If it's not an extension then the client have no way to know that the server support the parameter or not.
So even if he put the parameter there's absolutely no guarantee that the field will be there or a -1 or whatever.

So client side without an extension this does not change anything at all.

For the server side from the currently exposed pov it seems they would not add the field on the costly ones and since this is a temporary solution, this may just motivate more to build the new endpoints.

Pagination with very limited filtering and no sorts, is very limiting for the clients anyway.
There's a lot more user needs for proper filtering and sorting currently. (And since it would be new endpoints it would come with proper pagination too).


And so to resume again the So client side without an extension this does not change anything at all. is also the perfect example of why the new endpoints are more useful, because it allows clients to actually build something proper and working all the times for recent OS servers, instead of hacking quick things on older endpoints that does not offer half the things a modern client would want anyway.

@deluan
Copy link
Member

deluan commented Oct 17, 2024

IMHO, we should make this an extension and remove the -1 option. If the extension is present, the server must respond correctly to the totalCount param, by adding counts to the result, even if it has performance implications. This gives control to the client and ultimately to the users.

@kgarner7
Copy link
Contributor

Rather than hoping that totalCount is not -1 (with no way of knowing), if this is the solution, I'd argue it's probably better to make it an extension with new endpoints that just return the count (e.g. getAlbumListCount). Thus, the concern about some clients with infinite scroll getting worse while clients with pagination having no guarantees can both be solved.

@deluan
Copy link
Member

deluan commented Oct 17, 2024

Well the extension could be to just allow a reportTotalCount param to the existing endpoints. It would yield the same effect of a new set of endpoints, and it is my preference, but I'm ok with both solutions. :)

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

So let's go backwards and build on broken API to have clients limited by the currently very limited API instead of adding the proper extension and endpoints allowing clients to directly build everything they need.

A getAllAlbums endpoint with sort,filter and pagination is not at all the same effect than returning a count in getAlbumList2.

Let's try to see the big picture and what this opens and what clients will be able to provide with this. The compare with the proper solution.

Then see how much work for clients and server lost to support both for 0 gains.

IMO again adding the field as temporary solution is OK. Building an extension to add something ultra limited is not OK in the grand scheme of OS.

@dweymouth
Copy link
Member

IMO again adding the field as temporary solution is OK

It's not OK for clients if they can't rely on when a server will return a valid count vs -1, and also not OK for clients if servers have to return it for expensive queries. If we document that servers only return the count for things like an unfiltered getAlbumList2, and don't add it to search endpoints, then it can be OK

@deluan
Copy link
Member

deluan commented Oct 17, 2024

A getAllAlbums endpoint with sort,filter and pagination is not at all the same effect than returning a count in getAlbumList2.

I agree and one extension (totalCount), does not exclude a proper pagination extension in the future.

EDIT:

Building an extension to add something ultra limited is not OK in the grand scheme of OS.

formPost?

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

formPost define a major way to use the API how can this be limited since it touches all the endpoints and everything ....
At this point this is more trolling than anything.

But again only reading half what I write I should maybe write less.

How adding totalCount to albums solves the need for pagination for artists, songs and genres ?

We basically define an extension for 2 endpoints that will require a lot of things for servers and clients and completely useless for artists, songs and genres?

How is this something proper for a project who's purpose is to build proper APIs and enhance things.

In a couple of weeks, we define the new proper extension and endpoints and then what server and clients redo everything ?
What was the gain in the rush of something half baked here? How does it show a proper direction of OS if we handle the API future like this ?

It's been 10 years the API is like that, but somehow now there's an ultra urgency to provide something ultra limited when there was 0 reaction to work on the proper end points?

@deluan
Copy link
Member

deluan commented Oct 17, 2024

formPost define a major way to use the API how can this be limited since it touches all the endpoints and everything ....
At this point this is more trolling than anything.

On the server it is literally a 17 loc middleware: https://github.com/navidrome/navidrome/blob/a9334b7787bd4cfe18bb329654e28b0702004127/server/subsonic/middlewares.go#L27-L43. I can't imagine it being that complicated on the client, as it is very localized.

totalCounts on the other hand, will drive different UX on the clients, and the server will have to add this to a bunch of endpoints, and possibly cache queries for better performance. It is a much more complicated implementation in all aspects.

How adding totalCount to albums solves the need for pagination for artists, songs and genres ?

It doesn't, but IMO this half-baked solution is not good enough for most applications, and it gets worse if clients need to handle different UX depending on the result of a bunch of queries in different places, instead of a upfront query to getOpensubsonicExtendios. Rememeber, it is not just hiding values from the UI, but completely different screen/navigation implementations

In a couple of weeks, we define the new proper extension and endpoints

Couple of weeks? You are kidding, right? Who is trolling who now?

It's been 10 years the API is like that, but somehow now there's an ultra urgency to provide something ultra limited when there was 0 reaction to work on the proper end points?

I actually prefer a proper extension than this thing here, I'm just trying to echo what I hear from client devs, and how this is a immediate pain for most. I'm talking about new clients (Feishin, Supersonic, Stream Audio..), and online-first clients. Most apps from 10 years ago have a outdated UI/UX and don't care about this.

Ok, that's my 96 cents. I won't say anything else. You see, my position as a server dev is very comfortable. I can do whatever the clients want/need. Just let me know.

EDIT: by the way, 1 more cent, and I'll shut up.

How does it show a proper direction of OS if we handle the API future like this ?

When we decided to not bump the version and write a new API, we were bound to have this Frankenstein, sooner or later. I think everyone was aware of that.

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

Couple of weeks

Yes it is it someone start it. We already have most needs. We can totally ignore the pagination continuation token and use the totalCount query param and a random seed param to solve the random issue. Not perfect but works without much changes.

About all the new client do they really only want pagination for albums without being able to offer the same interface to artists and songs ?

Rememebr, it is not just hiding values from the UI, but completely different screen/navigation implementations

Is exactly the reason why only having thing for albums and ultra limited does not make sense.

No sorting? No proper filtering? Only albums?
That would only solve 1/10th of the devs and users problems. When we can solve 100%.

So again what does users and clients want ? That's what we should try to solve in a proper way, not make their life more complicated with things that will change as soon as they will require the other endpoint with pagination.

I can't imagine it being that complicated on the client, as it is very localized.

it is not just hiding values from the UI, but completely different screen/navigation implementations

Both are kinda contradictory, building a complete different screen and navigation that only covers a little part of the UI is neither simple not logical, specially is they need to redo all later.

Edit:

IMO this half-baked solution is not good enough for most applications

Confirms what I say and that this is not a good idea.

On the server it is literally a 17 loc middleware

This have literally 0 relationship with the utility, globalness and purpose of the definition of an API.

@dweymouth
Copy link
Member

So again what does users and clients want ? That's what we should try to solve in a proper way, not make their life more complicated with things that will change as soon as they will require the other endpoint with pagination.

Maybe we want to hold off on totalCount entirely then until the larger pagination API is proposed?

@Tolriq
Copy link
Member

Tolriq commented Oct 17, 2024

Maybe we want to hold off on totalCount entirely then until the larger pagination API is proposed?

If some persons are against the current proposal (with the small search2 add getnewestpodcasts removal) and want an extension for this temporary proposal then yes I think it's better for OS.

I still think that the field returned on the non slow path could help some clients to start improving their UI without fully switching to the future solution. But only clients can tell (In all cases since those totals are very limited in their scope, clients should also tell more about such values being useful only here and if they'd add new pagination system for only albums)

@kgarner7
Copy link
Contributor

Hi, what's the next steps here? Are people generally good with this as a first step? Or are people willing to commit time to provide full refinements?

@Tolriq
Copy link
Member

Tolriq commented Oct 27, 2024

This is good as a first step as long as it's not an turned into an extension.
Will some server handle it and does it really bring something to clients to only have this for 1/4 of the needs that's another story.

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

Successfully merging this pull request may close these issues.

6 participants