-
Notifications
You must be signed in to change notification settings - Fork 307
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
Issues with 'artistinfo' CLI command #1298
Comments
I'm a little confused by what you're doing. Please include LMS logs, look a the SQL your queries generates to understand what might be wrong. Don't get me wrong, I'm really trying to be helpful. If you're doing something several other parties have done before, and you have a problem, while the others don't, it's likely something about your approach is different. In such cases it's more helpful to ask for help in the forums (https://forums.lyrion.org), than to jump to conclusions and submit bug reports. |
What I'm doing is the same thing as other Android client apps are doing: opening a cometd connection, asking the server for the home menu ( I'm also not alone (as in What is special about my case, and what this report is about, is the double request (first with page size 1, then with page size 300). My observation is that if this kind of access is broken (as described above), that's why I opened the bug report. Are you saying I shall just deal with it and work around the brokenness in the client? |
I tried to reproduce the issue. I copied/pasted your command, replaced with IDs to fit mine:
The first would return the menu items for the artist ("Add to Favs", "On TIDAL"). The second would return the first element of the artist's TIDAL entry. The third would return that artist entry, plus other items from that menu like artist playlists, albums, songs. Nothing unexpected. And only five entries, not 10. No invalid entries. Could you provide a shell or perl or python script or something where I'd only have to replace the artist ID and the server IP to test the same series of queries? |
BTW: This happens as soon as one disables full text search plugin. It looks like the server side (all of the above navigation is generated server side, and should be reproducible on any Jive device - it certain does repro in SqueezePlay) has the same expectation as I had in #1297 ... so that's clearly broken. |
I can try ... can you give me a starting point for that? IOW, what did you use? I think it needs to be some cometd based client to keep the same connection to trigger this case ... or is keeping the connection ID also possible with separate HTTP connections? |
I can't follow this path: once in the artist I don't have a "Browse" menu item. I'd immediately have their albums, plus links to music services. Can you do a screencast or something from a player UI (squeezeplay)? I used Postman to copy/paste your commands starting with the first one (and artist ID adjusted to something I have). Cometd or JSON/RPC hopefully won't make any difference. I obviously tested the latter. See https://lyrion.org/reference/cli/using-the-cli/#jsonrpcjs for how to use JSON/RPC. |
|
Oh, I think that's a menu of the Music and Artist Information plugin. Are you using it? |
I'm not sure. As mentioned, this line caches the XML feed by connection (ID), as far as I can tell with the intention of reusing it. Calling jsonrpc.js twice yields different connection IDs, as I just verified using a debug output line:
In my understanding this caching is the actual issue, because it doesn't take the paging parameters into account, so the offset=0/size=1 case is cached and later reused for the offset=0/size=300 case. I tried to fix this using a trivial approach: --- Menu/ArtistInfo.pm 2025-01-15 15:43:51.448795198 +0100
+++ Menu/ArtistInfo.pm.new 2025-01-15 15:43:20.688538241 +0100
@@ -267,7 +267,7 @@
# keep a very small cache of feeds to allow browsing into a artist info feed
# we will be called again without $url or $artistId when browsing into the feed
-tie my %cachedFeed, 'Tie::Cache::LRU', 2;
+tie my %cachedFeed, 'Tie::Cache::LRU', 20;
sub cliQuery {
main::DEBUGLOG && $log->is_debug && $log->debug('cliQuery');
@@ -297,6 +297,7 @@
my $menuContext = $request->getParam('context') || 'normal';
my $playlist_index = defined( $request->getParam('playlist_index') ) ? $request->getParam('playlist_index') : undef;
my $connectionId = $request->connectionID || '';
+ my $cacheKey = "$connectionId:$index:$quantity";
my $tags = {
menuMode => $menuMode,
@@ -314,15 +315,16 @@
my $artist = Slim::Schema->find( Contributor => $artistId );
$feed = Slim::Menu::ArtistInfo->menu( $client, $artist->url, $artist, $tags );
}
- elsif ( $cachedFeed{ $connectionId } ) {
- $feed = $cachedFeed{ $connectionId };
+ elsif ( $cachedFeed{ $cacheKey } ) {
+ $feed = $cachedFeed{ $cacheKey };
}
else {
$request->setStatusBadParams();
return;
}
- $cachedFeed{ $connectionId } = $feed if $feed;
+ $log->error("caching feed for $cacheKey");
+ $cachedFeed{ $cacheKey } = $feed if $feed;
Slim::Control::XMLBrowser::cliQuery( 'artistinfo', $feed, $request );
} ... but I got 'Bad params' response with that, so there's probably more to it that I don't follow. |
Yes, I do (but wasn't aware :-/ ) ... so please pardon me mixing it up with the sufficiently similar ArtistInfo.pm :-( OK, so now (if I understand correctly) this menu basically is a global search with search terms pre-filled. Still, some caching seems to come into play here, since while I do two requests, only one DB query seems to happen:
This log also shows the dummy response items in the second response. |
No worries, I think I have the MAI issue fixed (not released - will do so later tonight). As for the caching issue: great catch! Your suggested solution looks reasonable to me. Would you mind submitting a PR with this change for all those *Info modules where we use this approach? (AlbumInfo, ArtistInfo, FolderInfo, TrackInfo, OnlineLibrary plugin)? |
Unfortunately it doesn't work :-/ |
I am developing (yet) another Android client for LMS based on Slimbrowse/Jive protocol/cometd and am facing a caching issue with the
artistinfo
CLI command, invoked via My music -> Album artists -> (menu action) -> Artist info.In the app, when moving between slimbrowse pages, I first fetch the first item of the target page to see whether it's a slider or text box, then switch to actual paging in case it's a normal list. This apparently creates a caching issue with artistinfo.
The command flow basically looks like this when opening the context menu and going through Browse -> My music -> Albums:
The problem here is that the last call gives me incomplete results, which looks something like this (partial response):
One item is 'valid' (-> #1297), then there's 'All albums', then 288 invalid empty items follow, so the issue is two fold:
The first problem can be worked around by not doing this 'preview' request (or do it sufficiently large, which is what I am doing, but I've seen broken paths (that I can't remember right now :-( ) even with that workaround in place. In any case, the server shouldn't send those invalid entries (which I've seen also with Squeeze Ctrl), though.
Another broken case (maybe related, maybe not, I can open a separate issue for that if desired):
Albums -> context menu for album -> artist info -> browse -> my music -> albums
yields 0 results, which should be impossible given the path taken.The text was updated successfully, but these errors were encountered: