inveniosoftware / invenio-records-rest

Invenio records REST API module.
https://invenio-records-rest.readthedocs.io
MIT License
4 stars 62 forks source link

sorter: default factoty not working for unicode queries #189

Closed egabancho closed 6 years ago

egabancho commented 6 years ago

This line https://github.com/inveniosoftware/invenio-records-rest/blob/master/invenio_records_rest/sorter.py#L134 will make unicode queries return the default noquery sorting mechanism.

# Default sort with query
with app.test_request_context("/?q=test"):
    query, urlargs = default_sorter_factory(Search(), 'myindex')
    assert query.to_dict()['sort'] == \
        [{'field1': {'order': 'desc'}}, {'field2': {'order': 'asc'}}]
    assert urlargs == dict(sort='-myfield')

with app.test_request_context("/?q=Oh là là"):
    query, urlargs = default_sorter_factory(Search(), 'myindex')
    assert query.to_dict()['sort'] == \
        [{'field1': {'order': 'desc'}}, {'field2': {'order': 'asc'}}]
    assert urlargs == dict(sort='-myfield')

Simply changing the line mentioned above by something like:

ipdb> request.values.get('q', type=str)
ipdb> request.values.get('q', type=six.text_type)
u'Oh l\xe0 l\xe0'

Related with https://github.com/zenodo/zenodo/issues/1227

Note 1: maybe it's worth taking a look at all the castings we are doing to str, specially if q is involved

Note 2: we only use six in a couple places, urllib.parse and string_types, maybe it's worth extending a bit the _compact.py in case we want to remove this dependency.

dinosk commented 6 years ago

@lnielsen I tested this and the sorting factory works ok after https://github.com/inveniosoftware/invenio-records-rest/pull/200, should I work on removing the six dependency?

Edit: Hmm, even if I remove the dependency from us, pydocstyle has six in its requirements, so run-tests.sh will fail. I'm unassigning for now as I'm not sure how to proceed. (I pushed the progress here if we choose to continue.)

lnielsen commented 6 years ago

Fixed in #200