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

sorting does not work when combined with other filters #339

Open
stefanprobst opened this issue Jul 10, 2022 · 15 comments
Open

sorting does not work when combined with other filters #339

stefanprobst opened this issue Jul 10, 2022 · 15 comments
Assignees

Comments

@stefanprobst
Copy link

(i) the sort query parameter is not documented in openapi, but i correctly guessed that the following will sort persons by their name:

curl "https://ica.acdh-dev.oeaw.ac.at/apis/api/entities/person/?sort=name" | jq '.results[].name'

"Aaron"
"Adamic"
"Anderson"
"Apel"
"Armstrong"
"Berger"
"Bishop"
"Borden"
"Brand"
"Brown"
"Byrnes"
"Caldwell"

(ii) however, when adding an additional query parameter, search results are no longer sorted:

curl "https://ica.acdh-dev.oeaw.ac.at/apis/api/entities/person/?sort=name&collection=13" | jq '.results[].name'

"Beer-Hofmann"
"Aaron"
"Barsis"
"Aaron"
"Sachs-Barsis"
"Beer-Hofmann Lens"
"Benatzky"
"Benedikt"
"Boyle"
"Burlingham Tiffany"
"Burnett"
"Doolittle"

(iii) confusingly, the second request seems to include names which are not returned in the first request, e.g. "Beer-Hofmann" - not sure what that means?

@stefanprobst
Copy link
Author

@sennierer any insights on this?

@richardhadden
Copy link

richardhadden commented Sep 19, 2022

Looking into this now: it seems at first glance that sort doesn't work at all.

Using MPR data, the first 55 results are sorted, then the rest are in some random order:

"Accerboni, Joseph Peter"
...
"Arnold, Johann"
"Laurentini"

I guess this is just the order they are stored in the database — with the result that any filters end up showing results that go beyond the top few which happen to be in the right order.

@stefanprobst Can you confirm this in some way, using your data?

@stefanprobst
Copy link
Author

I guess this is just the order they are stored in the database

yep, i guess that is what's happening. ICA backerd also seems to go unsorted at index 53, see:

curl "https://ica.acdh-dev.oeaw.ac.at/apis/api/entities/person/?sort=name&limit=200" | jq '.results[].name ' | cat -n

richardhadden pushed a commit that referenced this issue Sep 19, 2022
…backends

#339 Sorting not implemented; added the rest_framework OrderingFilters, which allows filtering on fields using `ordering` param
@richardhadden
Copy link

I think this fix should resolve the problem. It seems the drf OrderingFilter had not been added to filter_backends (apis_core/api_routers.py line 453). I have added this now and it seems to work (along with other query filters).

The drf param for sorting is ordering=. Change this in project settings using REST_FRAMEWORK["ORDERING_PARAM"]

@SteffRhes
Copy link
Member

Richard's PR is merged into main and so ordering= can be used now.

@stefanprobst please check and close this issue if verified.

@stefanprobst
Copy link
Author

the above query:

curl "https://ica.acdh-dev.oeaw.ac.at/apis/api/entities/person/?sort=name&limit=200" | jq '.results[].name ' | cat -n

is now sorted, thx.

how to sort in descending order?

@stefanprobst
Copy link
Author

also, looks like it does not show up in the openapi docs here: https://ica.acdh-dev.oeaw.ac.at/apis/swagger/schema/swagger-ui/#/apis/apis_api_entities_person_list

@richardhadden
Copy link

how to sort in descending order?

put a - before the parameter

(see https://www.django-rest-framework.org/api-guide/filtering/)

@stefanprobst
Copy link
Author

actually no - there is still something wrong. please try:

curl "https://ica.acdh-dev.oeaw.ac.at/apis/api/entities/person/?ordering=-name&limit=500" | jq '.results[].name ' | cat -n

@stefanprobst
Copy link
Author

are apis instances updated with this fix?

@stefanprobst
Copy link
Author

can you post an example request where this is working?

@richardhadden
Copy link

can you post an example request where this is working?

Not sure. I think @steffres merged this change, but maybe only into dev, so it may not be deployed?

@SteffRhes
Copy link
Member

I merged it, but am not aware of the current state of CI/CD and everything autodeployment-wise and github migration.

The PR is both in dev and main, so probably just not deployed?

@stefanprobst
Copy link
Author

if auto-deployment is not working - are you fixing it? should i open a new issue? where would i check if something failed?

@SteffRhes
Copy link
Member

@sennierer what is the status on CI/CD on github please?

In this issue here, a PR provides fix but apparentely it's not yet deployed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants