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

local: add limit and offset support to json library search helpers #949

Merged
merged 3 commits into from Mar 2, 2015

Conversation

4 participants
@ali
Contributor

ali commented Jan 26, 2015

Fixes the json local library's search behavior by adding support for limit and offset parameters to search helpers. Currently, JsonLibrary.search returns all matching results, and ignores the limit and offset parameters. (see #917)


Note: LocalLibraryProvider does not pass any limit/offset arguments when searching its local backend (which has a default limit of 100 and offset of 0). When using mopidy as a mpd server, this affects how many results the list command returns:

([mpd backend]: [result of $ mpc list artist | wc -l])
mpd: 1033
mopidy (develop): 1031
mopidy (fix/local-json-limit-offset): 86

@ali ali force-pushed the ali:fix/local-json-limit-offset branch from c4cdb66 to f15006c Jan 26, 2015

@tkem

This comment has been minimized.

Member

tkem commented Feb 3, 2015

Any idea why mpd reports two more artists than current mopidy (develop)?

@ali

This comment has been minimized.

Contributor

ali commented Feb 4, 2015

Updated both mopidy (develop) and mpd's libraries and did a test. Here's the diff of $ mpc list artist | sort for mopidy and mpd:

0a1
>
239a241
> Das Racist

mpd adds a newline at the beginning (might be because I have tracks without artists, which mopidy won't show in ncmpcpp), and outputs both "Das Racist" and "Das Racist " (mopidy only outputs "Das Racist"). Curiously enough, both will output "Daughter" and "Daughter " (with 22 spaces).

@tkem

This comment has been minimized.

Member

tkem commented Feb 4, 2015

Thanks @ali for having a look at the different outputs; was just curious ;-)

@jodal jodal added this to the v0.20 - Audio cleanup part 1 milestone Feb 5, 2015

def search(tracks, query=None, uris=None):
return SearchResult(
uri='local:search', tracks=tracks[offset:offset+limit])

This comment has been minimized.

@jodal

jodal Feb 9, 2015

Member

I think the missing spaces around + here will cause a flake8 warning with the recent flake8 update (after this PR was submitted).

@@ -162,7 +185,7 @@ def search(tracks, query=None, uris=None):
else:
raise LookupError('Invalid lookup field: %s' % field)
# TODO: add local:search:<query>
return SearchResult(uri='local:search', tracks=tracks)
return SearchResult(uri='local:search', tracks=tracks[offset:offset+limit])

This comment has been minimized.

@jodal

jodal Feb 9, 2015

Member

Ditto. Should be [offset:offset + limit] to please flake8.

Filter a list of tracks where ``field`` is ``values``.
:param list tracks: a list of :class:`~mopidy.models.Track`
:param dict query: one or more queries to search for

This comment has been minimized.

@jodal

jodal Feb 9, 2015

Member

Maybe change to "one or more field/value pairs to search for"? In my head, there's a single query with multiple key-value pairs.

Filter a list of tracks where ``field`` is like ``values``.
:param list tracks: a list of :class:`~mopidy.models.Track`
:param dict query: one or more queries to search for

This comment has been minimized.

@jodal

jodal Feb 9, 2015

Member

Same goes here.

self.library = json.JsonLibrary(self.config)
def _create_tracks(self, count):
for i in xrange(count):

This comment has been minimized.

@jodal

jodal Feb 9, 2015

Member

Use range() instead of xrange() to be a bit more ready for porting to Python 3. We've already replaced xrange() everywhere else in the code base.

@jodal

This comment has been minimized.

Member

jodal commented Feb 9, 2015

Other than my nitpick comments, this looks good to me. Leaving the final word and merge to @adamcik.

@tkem

This comment has been minimized.

Member

tkem commented Feb 10, 2015

Before merging this, please consider that AFAICS this will "break" the json backend in the same way that mopidy-local-sqlite is/was "broken" (according to mopidy/mopidy-local-sqlite#45) with respect to MPD's list artists/list albums commands...

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 10, 2015

For now the only safe thing to do is sadly to fix the issue piping the offset and limit through, but ignoring it in the the json backend / search :( Or else we end up breaking the MPD list commands as Thomas points out.

Fix for unblocking the proper fix for this needs to be adding a new backend method to handle this type of SELECT DISTINCT query or removing support for the MPD commands that are using this "badly" if we are OK with just having them always return black responses.

@ali ali force-pushed the ali:fix/local-json-limit-offset branch from 304301e to 61b9379 Feb 16, 2015

ali added some commits Feb 16, 2015

Fix flake8 tests
Fixes "W503 line break before binary operator"
local: use limit and offset when searching json library
Fixes the json local library's search behavior. Uses limit and offset
arguments when returning search results.
Fix flake8 errors, prepare for Python 3 port
Fixes flake8 warnings
Reword docstring for find_exact
Use range instead of xrange in preparation for porting to Python 3

@ali ali force-pushed the ali:fix/local-json-limit-offset branch from 61b9379 to ead147f Feb 16, 2015

@ali

This comment has been minimized.

Contributor

ali commented Feb 16, 2015

@adamcik Why are the limit and offset part of the spec? (See #917)
Should they be removed, or are the results of these search functions supposed to be paginated through?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2015

limit and offset probably got added prematurely and as such nothing uses it or supports it properly :( The idea was indeed that one should do pagination. So in the case of this "breaking" the list command we could in theory work around it with actually starting to use the pagination. But that would just be broken in a new way.

So basically I've been holding off on this PR while bikesheding with myself how to get the MPD list commands of search so that this change safely be merged.

@ali

This comment has been minimized.

Contributor

ali commented Feb 20, 2015

@adamcik Is there an issue for this bug so we can discuss more?

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

#1022 is on it's way now, so once that is in we should have resolved the blocker for merging this PR.

@adamcik adamcik merged commit ead147f into mopidy:develop Mar 2, 2015

2 checks passed

Scrutinizer 6 updated code elements
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@adamcik

This comment has been minimized.

Member

adamcik commented Mar 2, 2015

Added 319c1fc on top of this so it works with the new get_distinct. Thanks for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment