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

Crash when sorting by nonexistent field #1734

Closed
sampsyo opened this Issue Nov 20, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@sampsyo
Copy link
Member

sampsyo commented Nov 20, 2015

Executing the query string "--" exposes a bug in the query parser.

$ beet ls -- --
Traceback (most recent call last):
  File "/home/asampson/.local/bin/beet", line 9, in <module>
    load_entry_point('beets==1.3.16', 'console_scripts', 'beet')()
  File "/home/asampson/beets/beets/ui/__init__.py", line 1207, in main
    _raw_main(args)
  File "/home/asampson/beets/beets/ui/__init__.py", line 1197, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/home/asampson/beets/beets/ui/commands.py", line 958, in list_func
    list_items(lib, decargs(args), opts.album)
  File "/home/asampson/beets/beets/ui/commands.py", line 953, in list_items
    for item in lib.items(query):
  File "/home/asampson/beets/beets/dbcore/db.py", line 545, in __iter__
    objects = self.sort.sort(list(self._get_objects()))
  File "/home/asampson/beets/beets/dbcore/query.py", line 775, in sort
    return sorted(objs, key=key, reverse=not self.ascending)
  File "/home/asampson/beets/beets/dbcore/query.py", line 770, in key
    field_val = getattr(item, self.field)
  File "/home/asampson/beets/beets/dbcore/db.py", line 326, in __getattr__
    raise AttributeError('no such field {0!r}'.format(key))
AttributeError: no such field '-'

The same occurs for any sequence of dashses longer than two: "-------" results in the same crash. Interestingly, "----foo" does not.

@sampsyo sampsyo added the bug label Nov 20, 2015

@sampsyo sampsyo added this to the 1.3.16 milestone Nov 20, 2015

@diego-plan9

This comment has been minimized.

Copy link
Member

diego-plan9 commented Nov 23, 2015

It seems to be actually a particular case of another bug: if a query term (that is not a field-query, and not a regular expression query) ends with - or +, the parser interprets it as a Sort, failing ungracefully due to the string (minus the last - or +) not being a valid Item field:

At the current revision (0a719a0):

$ beet ls -- foo-
...
AttributeError: no such field 'foo'

$ beet ls -- bar+
...
AttributeError: no such field 'bar'

$ beet ls -- ----a--
...
AttributeError: no such field '----a-'

There is also some interference/ambiguity with the syntax of NotQuery: if the term starts with - and ends in - or +, such as ls -- -foo-, it is interpreted as "try to sort by -foo descending" instead of "exclude the items that have foo-". Seems like a bordeline case anyway, but I'm wondering what would be the semantically right way to interpret ls -- ---?

One of the culprits seems to be the if on queryparse.parse_sorted_query: I'm wondering if it would be enough to harden that test by checking if the term starts with -, and providing a cleaner way to catch the AtributeError exception so it is handled gracefully and the user is presented with a more informative message (something along the lines of "Invalid sorting field specified: x").

@sampsyo sampsyo changed the title Crash on query "--" Crash when sorting by nonexistent field Nov 23, 2015

@sampsyo

This comment has been minimized.

Copy link
Member Author

sampsyo commented Nov 23, 2015

Thanks for digging deeper! You're absolutely right about the cause.

About the ambiguity: great point. From my perspective, it's not all that important who "wins" for a query like --- or -foo-; we should just pick one (sort by -foo or exclude foo-) and be consistent.

And about the fix: It should be legal to sort by fields that don't exist on all entries in the database. In the same way that foo:bar is a legal query term even when only some items in your database have a foo flexible attribute, a sort foo- or foo+ shouldn't be an error. (It should just sort the music that's missing foo to one end or the other.) So yes, the fix should probably handle the missing value in this getattr and just yield an empty string.

In fact, perhaps the fix is as simple as changing that line to:

field_val = item.get(self.field, '')

diego-plan9 added a commit to diego-plan9/beets that referenced this issue Nov 24, 2015

Fix sorting by nonexistent field (beetbox#1734), tests
* Fix crash when sorting by nonexistent field.
* Add tests for queries with nonexistient fields for sorting.

diego-plan9 added a commit to diego-plan9/beets that referenced this issue Nov 24, 2015

Fix sorting by nonexistent field (beetbox#1734), tests
* Fix crash when sorting by nonexistent field.
* Add tests for queries with nonexistient fields for sorting.

sampsyo added a commit that referenced this issue Nov 25, 2015

Merge pull request #1736 from diego-plan9/querysort
Fix for crash when sorting by nonexistent field (#1734)

@sampsyo sampsyo closed this Dec 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.