-
Notifications
You must be signed in to change notification settings - Fork 237
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
genre search functionality #559
Conversation
commit bb26ca3 is required due to |
I did a short test with this branch in combination with chme/forked-daapd-web#6 Here is a truncated response to "api/library/genres" from my test system: {
"items": [
{
"name": "Alternative",
"album_count": 1,
"track_count": 1,
"length_ms": 230423
},
...
{
"name": "Rock/Pop",
"album_count": 1,
"track_count": 1,
"length_ms": 257105
},
...
{
"name": "'90s Alternative",
"album_count": 1,
"track_count": 1,
"length_ms": 257488
},
....
],
"total": 28,
"offset": 0,
"limit": -1
} The web interface does not properly escape the genre name and therefor only the first one "Alternative" works. The second one simply return zero items. The web interface URI is And the last one results in an sql error:
Did you consider to handle genres similar to artists/albums by generating a unique id? Maybe the "groups" table could be used with a new "type" for genres to get a persistent id for a genre. Of course this would be a lot more work ... |
src/httpd_jsonapi.c
Outdated
query_params.filter = db_mprintf("(f.genre= '%s')", genre); | ||
|
||
if (media_kind) | ||
query_params.filter = db_mprintf("(f.media_kind = %d)", media_kind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The media_kind filter replaces the filter to fetch a genre (and results in a memory leak). The filters should instead be combined to only fetch the albums of a genre that match the given media_kind.
src/httpd_jsonapi.c
Outdated
|
||
query_params.type = Q_GROUP_ALBUMS; | ||
query_params.sort = S_ALBUM; | ||
query_params.filter = db_mprintf("(f.genre= '%s')", genre); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be '%q'. Should solve the SQL injection problem with genres that contain '.
Appreciate the review. I created a dummy file with the genre Looks like I;ll have to re-factor EDIT: |
I think you should strive to do this with the existing data model and queries. As far I understand what you want to do is return all genres + albums for a particular genre. These are queries that are already supported and implemented for daap (possibly also for rsp), even with optimised indeces, as I recall. So try to use that framework. There are many ways to group albums and tracks (year, composer, type etc.), and I don't like the idea of having id's for each of these. |
Fixes to issues raised for
WebUI updated, build here: dist-search-genre-98ce8de37dfda03a766e5745e5e1219be764545b.tar.gz |
cd801a2
to
c408a67
Compare
Previous issues fixed, commits squashed.
Please review, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not possible to create REST-like endpoints for genres. I would prefer to drop the library/genre?q=
endpoint.
In order to retrieve the albums for a genre, it should already be possible to use the search endpoint (with a smart pl query): api/search?type=album&expression=genre+is+"Alternative"
.
Another possibility is to extend the library/albums
endpoint to support a "genre" query parameter (similar to the media_kind parameter) that allows filtering by genre.
I think, @ejurgensen had the Q_BROWSE_GENRES
query type in mind. Q_BROWSE_GENRES
can be used to get a list of genres. This query type only returns the genre name and no information how many artists/albums are matching a genre. Not sure how important the album and artist count information is ...
src/logger.c
Outdated
@@ -321,7 +321,7 @@ logger_init(char *file, char *domains, int severity) | |||
return -1; | |||
} | |||
|
|||
ret = fchown(fileno(logfile), runas_uid, 0); | |||
ret = fchown(fileno(logfile), runas_uid, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the effect of this change? This should at least be a separate commit and better a separate pull request.
c408a67
to
0742f32
Compare
I've been a bit dumb here - I opted for BUT we can go with With escaped genre names we can send across Does this end point work for you? On this laptop, using the |
I agree with @chme that it would be good to use the search api for this - i.e. not add a specific endpoint. If it doesn't work then it should be fixed, of course. I also agree with him about the logger.c change. The same goes for admin.html. @chme is also correct that I think the BROWSE query should be used. Genre is not a group in the data model, and the query with a join on the group table doesn't make sense to me. I suggest first doing the endpoint/UI with existing metadata, and then afterwards expand the BROWSE result if more metadata is required for a better UX. |
@whatdoineed2do In your comment the closing I can get forked-daapd to crash, if i am missing the leading |
I appears my local master has local commits (admin.html / logger.c) that were never meant to see the light of day that have crept into this feature branch. I'll revert and sync with upstream Regarding the are we recommending that we don't introduce a Adding the genre search to Adding an equivalant just give me a list of X (ie albums/artists/tracks/genres) seems to be most in line with current json i/f but I'll go with your views (after all you will review/merge/maintain). All of these listings seem to have a corresponding Implementing the get list of albums belonging to genre into the |
/api/library/genres is fine, but as far as I can tell you can use Q_BROWSE_GENRE to query the db, you don't need Q_GROUP_GENRE. You can also later add a /api/library/composers and then use Q_BROWSE_COMPOSERS ... and so on. I didn't quite understand the two paragraphs after your question, but yes for searching albums belonging to genre it seems good to use the search interface. |
thanks. Yes, just pushed the most recent fixes to this branch (wasn't 100% ready for review) that now reflects the discussion above:
Need to fix the webui but need to over issues raised in #570 |
I think this is clean for review now - appreciate the patience
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice there are some style things you should fix
src/httpd_jsonapi.c
Outdated
genre_to_json(const char* g_, const struct filecount_info* fci_) | ||
{ | ||
json_object *item = NULL; | ||
if (g_ == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent like in the rest of forked-daapd
src/httpd_jsonapi.c
Outdated
@@ -232,6 +232,23 @@ playlist_to_json(struct db_playlist_info *dbpli) | |||
return item; | |||
} | |||
|
|||
static json_object * | |||
genre_to_json(const char* g_, const struct filecount_info* fci_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the underscores? "genre" and "fci" would be good names
src/httpd_jsonapi.c
Outdated
static json_object * | ||
genre_to_json(const char* g_, const struct filecount_info* fci_) | ||
{ | ||
json_object *item = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this assignment doesn't seem to do anything
src/httpd_jsonapi.c
Outdated
@@ -330,6 +347,7 @@ fetch_artist(const char *artist_id) | |||
return artist; | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
src/httpd_jsonapi.c
Outdated
json_object *item; | ||
int ret = 0; | ||
char* bi; | ||
char* si; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why two spaces before bi and si?
naming of bi and si could be better
assignment of ret does nothing
src/httpd_jsonapi.c
Outdated
|
||
while (((ret = db_query_fetch_string_sort(query_params, &bi, &si)) == 0) && (bi)) | ||
{ | ||
struct filecount_info fci; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no mid function declarations in forked-daapd
src/httpd_jsonapi.c
Outdated
json_object *reply; | ||
json_object *items; | ||
int total; | ||
int ret = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignment with no effect
styling fixed. I still need to squash the commits - we need to agree that the front end functionality is good for you too (a tar file/drop in replacement for the urght.. hold off - the search bar needs a fix too |
The last bug I mention above is when we use the webui's search page looking for value that doesn't match any genre. The query from the ui arrives at the server: If XXX exist then this is fine. However, if XXX is not a known genre then because of uing the
but this returns NULL from Q - does this mean that the BROWSE searches ALWAYS expects a result? The web ui's listing of genre uses |
d5c6d7f
to
c10308b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took another look, I think these are the final comments from my side. When fixed, and @chme also has checked, I think it is ready to be merged.
src/httpd_jsonapi.c
Outdated
@@ -234,12 +234,32 @@ playlist_to_json(struct db_playlist_info *dbpli) | |||
return item; | |||
} | |||
|
|||
static json_object * | |||
genre_to_json(const char* genre, const struct filecount_info* fci) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent asterix placement
src/httpd_jsonapi.c
Outdated
json_object *item; | ||
int ret; | ||
char* genre; | ||
char* sort_item; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asterix
src/httpd_jsonapi.c
Outdated
qp.type = Q_COUNT_ITEMS; | ||
qp.idx_type = I_NONE; | ||
qp.filter = db_mprintf("(f.genre = '%q')", genre); | ||
db_query_start(&qp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db_filecount_get starts the query, so you don't need to do it here
src/httpd_jsonapi.c
Outdated
db_query_start(&qp); | ||
db_filecount_get(&fci, &qp); | ||
free(qp.filter); | ||
db_query_end(&qp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also not required
src/httpd_jsonapi.c
Outdated
memset(&qp, 0, sizeof(qp)); | ||
|
||
qp.type = Q_COUNT_ITEMS; | ||
qp.idx_type = I_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is memset of qp and setting consts required inside the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all fixed - memset(&qp...)
was a left over of prev review where struct was local to loop
src/httpd_jsonapi.c
Outdated
qp.idx_type = I_NONE; | ||
qp.filter = db_mprintf("(f.genre = '%q')", genre); | ||
db_query_start(&qp); | ||
db_filecount_get(&fci, &qp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was in 2minds - if it fails (and ignore the ret
code), the fci is already zero'd out so we can continue tell the client that theres 0 for album/artist/track count for that genre (even though the info is not correct due to internal library failure) but at least the client knows the system knows about the genre
I added check for ret
so now if theres a problem getting info about genre, we don't present any of its info back to client.
Thanks for the fixing up, the pr looks fine to me now. As mentioned @chme will also have to check it before merging. BTW forgot to answer your question: The browse query should work with zero results as well, and if it doesn't it is a bug. I checked the code and it looks ok, db_get_one_int() should get a row because it is a count query (so there should be a row with a "0"). Can I ask you to check again? |
The On the sql prompt we can happily run but note the return on first query when theres no match, no rows are returned.
without the
Proposed Fix
Let me know if this fix is something you are comfortable and I'll add this to the PR. |
Oh sorry, I completely missed you were using BROWSE for searching, I somehow thought it was just was for listing. Let me get back to you... |
README_JSON_API.md
Outdated
**Example** | ||
|
||
``` | ||
curl -X GET "http://localhost:3689/api/search?type=albums&expression=genre+is+\"Pop\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ending quotation mark is missing in these examples
In your original post you said the purpose of the pr was: "Add endpoints to:
But the pr now also extends the search api to return a json genre object, which contains matching genre names plus album, artist and track count. I missed that part in the above conversation, and also when reviewing, I'll admit. I also missed what the purpose of this search is, so I hope you can reiterate - is it to support some sort of future type-ahead search? You wrote above that the web ui only uses type=album, so I am unsure what type=genre is for? |
The work here was to support the following (in order of priority):
the tar file updated on the OP (https://github.com/ejurgensen/forked-daapd/files/2318320/dist-search-genre-7d5becba4a8cee675eadbdbadec194dba3386651.tar.gz) can be dropped in place for the existing To further clarify, the #559 issue only applies to the |
db_admin_get() .. admin_get() .. strdup() fetch_album() .. db_mprintf() .. strdup() fetch_artist() .. db_mprintf() .. strdup()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these memleaks!
If you could remove the count values from the genre object, that would be great. I would prefer to look at these in a separate pr. (Instead of an inner db query, i think it would be better to extend the browse queries to return the count values).
src/httpd_jsonapi.c
Outdated
|
||
item = json_object_new_object(); | ||
safe_json_add_string(item, "name", genre); | ||
json_object_object_add(item, "album_count", json_object_new_int(fci ? fci->album_count : 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the count values from the genre object for this pr. Extending the genre object with more metadata can be done in follow up pr. Genre would then only contain the "name" property.
src/httpd_jsonapi.c
Outdated
memset(&fci, 0, sizeof(fci)); | ||
|
||
qp.filter = db_mprintf("(f.genre = '%q')", genre); | ||
ret = db_filecount_get(&fci, &qp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the count values in the genre object, this inner db query can be dropped.
safe_json_add_time_from_string(jreply, "started_at", (s = db_admin_get(DB_ADMIN_START_TIME)), true); | ||
free(s); | ||
safe_json_add_time_from_string(jreply, "updated_at", (s = db_admin_get(DB_ADMIN_DB_UPDATE)), true); | ||
free(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks to everyone's friend, valgrind
fyi - there's one other leak i saw in httpd.c
's httpd_init() .. exitev = event_new(evbase_httpd, exit_efd, EV_READ, exit_cb, NULL);
but I'm not familar with libevent
to fix this It leaks 136 bytes IIRC
src/httpd_jsonapi.c
Outdated
@@ -2679,6 +2894,22 @@ jsonapi_reply_search(struct httpd_request *hreq) | |||
if (param_expression) | |||
{ | |||
expression = safe_asprintf("\"query\" { %s }", param_expression); | |||
|
|||
#ifndef ANTLR_PARSER_FIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack only avoids the parser bug in a sepcific case and will not work in cases where '+' is a valid character. So please drop this hack.
I concur with @chme, and also suggest that you move the genre search to another pr. That way we can get the "must have" merged and continue the discussion on the other stuff seperately. |
…re list only provides names
Updated as per review webui PR: chme/forked-daapd-web#6 |
…e list only provides names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
ret = fetch_genres(&query_params, items, NULL); | ||
if (ret < 0) | ||
goto error; | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor: the assignment to "total" can be done unconditionally (the if-part jumps to "error").
Merged, thanks! |
Add endpoints to:
genres
known to servergenres
, returning all albums matching agenre
modeled heavily on existing
album
code.Functionality would be coupled with update to UI that would allow users to queue music by a given genre, such as classical. Currently, users need to locate and queue individual songs/artists/albums.
Missing updates to web ui for which an issue #558 is loggedWeb ui (generated/drop-inhtdocs
files) updates on a fork:https://github.com/whatdoineed2do/forked-daapd-web/files/2258629/dist-0cd890b3128c50510c776d455adb0c61a9b3b376.tar.gzdist-search-genre-98ce8de37dfda03a766e5745e5e1219be764545b.tar.gzEDIT: 2018-08-24
Q_BROWSE_GENRE
api/library/genres
api/search
Please use in conjunction with this build to see intended use cases - web ui code NOT incl in PR)
Upon server functionality is merge, I will submit PR to https://github.com/chme/forked-daapd-web.
comments / feedback? @chme , @ejurgensen
Thanks