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

Add multivalue genre tag support #2503

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c1636ff
Add a StringList type
antlarr Apr 4, 2017
3c2c716
Use the new StringList type for genre and store it using multiple tags
antlarr Apr 4, 2017
28a508d
Make lastgenre return the genres as a list
antlarr Apr 4, 2017
1ce4c16
Allow None values to be parsed as null StringList values
antlarr Apr 4, 2017
091e126
Define a null value for StringList, parse '' correctly and use '; ' a…
antlarr Apr 4, 2017
645565c
Update tests to test the new genre as a list
antlarr Apr 4, 2017
35c8fa1
Allow to set a multivalued string to a ListMediaField and a test for it
antlarr Apr 4, 2017
6822c52
Enforce StringList values to be lists and fix some more tests accordi…
antlarr Apr 4, 2017
efe7e31
Store empty lists as null in the database
antlarr Apr 5, 2017
aa8a33a
Allow lyricist,composer(_sort),arranger fields to have multiple values
antlarr Apr 5, 2017
55a9e3d
Fix bpd plugin with the new genre as a list
antlarr Apr 5, 2017
57d682d
Accept None value in ListStorageStyle.set
antlarr Apr 5, 2017
f9de5ec
Accept [] in MP3ListStorageStyle.store
antlarr Apr 5, 2017
e41a56b
Inherit MP3PeopleStorageStyle from ListStorageStyle
antlarr Apr 5, 2017
1ac4014
Don't print fields whose value is an empty list
antlarr Apr 5, 2017
5504f45
Fix tests for lyricist, composer and arranger which are now lists.
antlarr Apr 5, 2017
1465181
Replace "if values == []:" with "if not values:"
antlarr Apr 5, 2017
63138e0
Add `multivalue_separator` config option so '; ' is not hardcoded any…
antlarr Apr 6, 2017
61411ac
Use tuples instead of lists for multivalue fields
antlarr Apr 6, 2017
53617dc
Fix E501 and a small typo in the documentation
antlarr Apr 6, 2017
9748df2
Fix E501 and E303 errors
antlarr Apr 6, 2017
2f8e854
Store/Load StringList type as json and migrate non-json values in the db
antlarr Apr 12, 2017
05f3476
Convert valid json strings to json arrays too
antlarr Apr 12, 2017
63d7780
Add JSonSubstringListQuery class and other query related changes
antlarr Apr 12, 2017
31b08c5
Implement JSonSubstringListQuery negated clause
antlarr Apr 12, 2017
818a60d
Add JSonRegexpListQuery class and parametrize the regexp uses
antlarr Apr 12, 2017
f0f3943
Change test_library since genre and composer are now json values in db
antlarr Apr 12, 2017
e5c831c
Check if sqlite supports json functions and do equivalent code in python
antlarr Apr 12, 2017
8228067
Fix documentation related to multivalue_separator
antlarr Apr 12, 2017
06f7c04
Do not fail migrating the database when iterating on new fields
antlarr Apr 17, 2017
bbd8809
Add composer_sort to the list of fields which are string lists
antlarr May 30, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions beets/autotag/mb.py
Expand Up @@ -222,10 +222,10 @@ def track_info(recording, index=None, medium=None, medium_index=None,
composer_sort.append(
artist_relation['artist']['sort-name'])
if lyricist:
info.lyricist = u', '.join(lyricist)
info.lyricist = lyricist
if composer:
info.composer = u', '.join(composer)
info.composer_sort = u', '.join(composer_sort)
info.composer = composer
info.composer_sort = composer_sort

arranger = []
for artist_relation in recording.get('artist-relation-list', ()):
Expand All @@ -234,7 +234,7 @@ def track_info(recording, index=None, medium=None, medium_index=None,
if type == 'arranger':
arranger.append(artist_relation['artist']['name'])
if arranger:
info.arranger = u', '.join(arranger)
info.arranger = arranger

info.decode()
return info
Expand Down
1 change: 1 addition & 0 deletions beets/config_default.yaml
Expand Up @@ -44,6 +44,7 @@ path_sep_replace: _
asciify_paths: false
art_filename: cover
max_filename_length: 0
multivalue_separator: '; '

plugins: []
pluginpath: []
Expand Down
75 changes: 71 additions & 4 deletions beets/dbcore/db.py
Expand Up @@ -27,10 +27,11 @@

import beets
from beets.util.functemplate import Template
from beets.util import py3_path
from beets.util import py3_path, SQLITE_HAS_JSON_EXTENSION
from beets.dbcore import types
from .query import MatchQuery, NullSort, TrueQuery
import six
import json


class DBAccessError(Exception):
Expand Down Expand Up @@ -488,6 +489,15 @@ def set_parse(self, key, string):
"""
self[key] = self._parse(key, string)

@classmethod
def _fields_col_names(cls):
"""Return the list of all fixed fields column names.

Returns the list of all fixed fields column names with table name
prepended to be used in a select expression.
"""
return [cls._table + '.' + field for field in cls._fields]


# Database controller and supporting interfaces.

Expand Down Expand Up @@ -800,6 +810,57 @@ def transaction(self):

# Schema setup and migration.

def _migrate_table_fields_content_type(self, table, fields):
"""Check that the `fields` contain data according to their type.

For now, only json values are enforced in StringList fields.
"""
for name, _type in fields.items():
if isinstance(_type, types.StringList):
if SQLITE_HAS_JSON_EXTENSION:
query = ("SELECT id, {0} FROM {1} WHERE {0} != '' AND " +
"(NOT JSON_VALID({0}) OR " +
"JSON_TYPE({0}) != 'array')").format(name, table)

with self.transaction() as tx:
try:
rows = tx.query(query)
except sqlite3.OperationalError:
rows = []
else:
query = "SELECT id, {0} FROM {1} WHERE {0} != ''".format(
name, table)

with self.transaction() as tx:
rows = []
try:
all_rows = tx.query(query)
except:
all_rows = []
for row in all_rows:
try:
value = json.loads(row[1])
except:
rows.append(row)
else:
if not isinstance(value, list):
rows.append(row)

if not rows:
continue

with self.transaction() as tx:
for _id, _value in rows:
if not _value:
continue

query = 'UPDATE {0} SET {1} =? WHERE id=?'.format(
table, name
)
new_value = _type.to_sql(_type.parse(_value))

tx.mutate(query, (new_value, _id))

def _make_table(self, table, fields):
"""Set up the schema of the database. `fields` is a mapping
from field names to `Type`s. Columns are added if necessary.
Expand All @@ -812,6 +873,7 @@ def _make_table(self, table, fields):
field_names = set(fields.keys())
if current_fields.issuperset(field_names):
# Table exists and has all the required columns.
self._migrate_table_fields_content_type(table, fields)
return

if not current_fields:
Expand All @@ -823,6 +885,7 @@ def _make_table(self, table, fields):
', '.join(columns))

else:
self._migrate_table_fields_content_type(table, fields)
# Table exists does not match the field set.
setup_sql = ''
for name, typ in fields.items():
Expand Down Expand Up @@ -861,11 +924,15 @@ def _fetch(self, model_cls, query=None, sort=None):
"""
query = query or TrueQuery() # A null query.
sort = sort or NullSort() # Unsorted.
where, subvals = query.clause()
where, subvals, new_tables = query.clause()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sampsyo should I still allow Query objects to maybe return 2-tuples just in case any external plugin still uses the old API? It's easy to check for unpack errors but it's a bit ugly, so I preferred to ask your opinion.

tables = [model_cls._table]
tables.extend(new_tables)
col_names = ', '.join(model_cls._fields_col_names())
order_by = sort.order_clause()

sql = ("SELECT * FROM {0} WHERE {1} {2}").format(
model_cls._table,
sql = ("SELECT DISTINCT {0} FROM {1} WHERE {2} {3}").format(
col_names,
', '.join(tables),
where or '1',
"ORDER BY {0}".format(order_by) if order_by else '',
)
Expand Down
118 changes: 94 additions & 24 deletions beets/dbcore/query.py
Expand Up @@ -66,11 +66,12 @@ class Query(object):
def clause(self):
"""Generate an SQLite expression implementing the query.

Return (clause, subvals) where clause is a valid sqlite
WHERE clause implementing the query and subvals is a list of
items to be substituted for ?s in the clause.
Return (clause, subvals, tables) where clause is a valid sqlite
WHERE clause implementing the query, subvals is a list of
items to be substituted for ?s in the clause and tables is a
list of FROM table names to be added to the sql statement.
"""
return None, ()
return None, (), ()

def match(self, item):
"""Check whether this query matches a given Item. Can be used to
Expand Down Expand Up @@ -101,14 +102,14 @@ def __init__(self, field, pattern, fast=True):
self.fast = fast

def col_clause(self):
return None, ()
return None, (), ()

def clause(self):
if self.fast:
return self.col_clause()
else:
# Matching a flexattr. This is a slow query.
return None, ()
return None, (), ()

@classmethod
def value_match(cls, pattern, value):
Expand All @@ -135,7 +136,7 @@ def __hash__(self):
class MatchQuery(FieldQuery):
"""A query that looks for exact matches in an item field."""
def col_clause(self):
return self.field + " = ?", [self.pattern]
return self.field + " = ?", [self.pattern], ()

@classmethod
def value_match(cls, pattern, value):
Expand All @@ -148,7 +149,7 @@ def __init__(self, field, fast=True):
super(NoneQuery, self).__init__(field, None, fast)

def col_clause(self):
return self.field + " IS NULL", ()
return self.field + " IS NULL", (), ()

@classmethod
def match(cls, item):
Expand All @@ -165,6 +166,7 @@ class StringFieldQuery(FieldQuery):
"""A FieldQuery that converts values to strings before matching
them.
"""

@classmethod
def value_match(cls, pattern, value):
"""Determine whether the value matches the pattern. The value
Expand All @@ -190,7 +192,7 @@ def col_clause(self):
search = '%' + pattern + '%'
clause = self.field + " like ? escape '\\'"
subvals = [search]
return clause, subvals
return clause, subvals, ()

@classmethod
def string_match(cls, pattern, value):
Expand Down Expand Up @@ -227,6 +229,64 @@ def string_match(cls, pattern, value):
return pattern.search(cls._normalize(value)) is not None


class JSonSubstringListQuery(SubstringQuery):
"""A query that matches a regular expression in a specific item
field which contains a json dump of a string array.
"""
_unique_id = 1

def __init__(self, field, pattern, fast, model_cls):
super(JSonSubstringListQuery, self).__init__(
field, pattern, fast and util.SQLITE_HAS_JSON_EXTENSION)
self.model_cls = model_cls

def get_unique_alias(self):
alias = 'json_' + self.field + str(JSonSubstringListQuery._unique_id)
JSonSubstringListQuery._unique_id += 1
return alias

def col_clause(self):
pattern = (self.pattern
.replace('\\', '\\\\')
.replace('%', '\\%')
.replace('_', '\\_'))
search = '%' + pattern + '%'
alias = self.get_unique_alias()
clause = alias + ".value like ? escape '\\'"
subvals = [search]
table = 'json_each({0}) as {1}'.format(self.field, alias)
return clause, subvals, (table,)

def negated_clause(self):
clause, subvals, table = self.col_clause()
_table = self.model_cls._table
primary_key = '%s.id' % (_table)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sampsyo Do you think it's worth it to add a get_primary_key_field to Model so instead of hardcoding id I call self.model_cls.get_primary_key_field() which would iterate over its fields and search which has a PRIMARY_ID type? I think that would be more elegant, but I'm not sure if it's worth the effort.

neg_clause = '{0} not in (select {1} from {2}, {3} where {4})'.format(
primary_key, primary_key, _table, ', '.join(table), clause)
return neg_clause, subvals, ()

@classmethod
def value_match(cls, pattern, _value):
return any([pattern.lower() in value.lower() for value in _value])


class JSonRegexpListQuery(RegexpQuery):
"""A query that matches a regular expression in a specific item
field which is a json value.

Raises InvalidQueryError when the pattern is not a valid regular
expression.
"""

@classmethod
def value_match(cls, pattern, _value):
"""Determine whether the value matches the pattern. The value
may have any type.
"""
return any([pattern.search(cls._normalize(value)) is not None
for value in _value])


class BooleanQuery(MatchQuery):
"""Matches a boolean field. Pattern should either be a boolean or a
string reflecting a boolean.
Expand Down Expand Up @@ -259,7 +319,7 @@ def __init__(self, field, pattern):
self.pattern = bytes(self.pattern)

def col_clause(self):
return self.field + " = ?", [self.buf_pattern]
return self.field + " = ?", [self.buf_pattern], ()


class NumericQuery(FieldQuery):
Expand Down Expand Up @@ -320,17 +380,17 @@ def match(self, item):

def col_clause(self):
if self.point is not None:
return self.field + '=?', (self.point,)
return self.field + '=?', (self.point,), ()
else:
if self.rangemin is not None and self.rangemax is not None:
return (u'{0} >= ? AND {0} <= ?'.format(self.field),
(self.rangemin, self.rangemax))
(self.rangemin, self.rangemax), ())
elif self.rangemin is not None:
return u'{0} >= ?'.format(self.field), (self.rangemin,)
return u'{0} >= ?'.format(self.field), (self.rangemin,), ()
elif self.rangemax is not None:
return u'{0} <= ?'.format(self.field), (self.rangemax,)
return u'{0} <= ?'.format(self.field), (self.rangemax,), ()
else:
return u'1', ()
return u'1', (), ()


class CollectionQuery(Query):
Expand Down Expand Up @@ -360,15 +420,18 @@ def clause_with_joiner(self, joiner):
"""
clause_parts = []
subvals = []
tables = []
for subq in self.subqueries:
subq_clause, subq_subvals = subq.clause()
subq_clause, subq_subvals, _tables = subq.clause()
tables.extend(_tables)

if not subq_clause:
# Fall back to slow query.
return None, ()
return None, (), ()
clause_parts.append('(' + subq_clause + ')')
subvals += subq_subvals
clause = (' ' + joiner + ' ').join(clause_parts)
return clause, subvals
return clause, subvals, tables

def __repr__(self):
return "{0.__class__.__name__}({0.subqueries!r})".format(self)
Expand Down Expand Up @@ -452,18 +515,25 @@ def match(self, item):
class NotQuery(Query):
"""A query that matches the negation of its `subquery`, as a shorcut for
performing `not(subquery)` without using regular expressions.

Sometimes, negating a subquery is not as simple as prefixing it with 'NOT'
in the WHERE clause, so for those cases, query objects may implement a
negated_clause function that returns a negated clause expression.
"""
def __init__(self, subquery):
self.subquery = subquery

def clause(self):
clause, subvals = self.subquery.clause()
if hasattr(self.subquery, 'negated_clause'):
return self.subquery.negated_clause()

clause, subvals, tables = self.subquery.clause()
if clause:
return 'not ({0})'.format(clause), subvals
return 'not ({0})'.format(clause), subvals, tables
else:
# If there is no clause, there is nothing to negate. All the logic
# is handled by match() for slow queries.
return clause, subvals
return clause, subvals, tables

def match(self, item):
return not self.subquery.match(item)
Expand All @@ -482,7 +552,7 @@ def __hash__(self):
class TrueQuery(Query):
"""A query that always matches."""
def clause(self):
return '1', ()
return '1', (), ()

def match(self, item):
return True
Expand All @@ -491,7 +561,7 @@ def match(self, item):
class FalseQuery(Query):
"""A query that never matches."""
def clause(self):
return '0', ()
return '0', (), ()

def match(self, item):
return False
Expand Down Expand Up @@ -660,7 +730,7 @@ def col_clause(self):
else:
# Match any date.
clause = '1'
return clause, subvals
return clause, subvals, ()


class DurationQuery(NumericQuery):
Expand Down