Sorting by key plugin for browse results collection
ClosedPublic

Authored by yashn on Jun 25 2017, 5:41 AM.

Details

Summary

Results can be sorted in an ascending order or descending order by any key present in the results.

Test Plan

Added a couple of testcases.

Diff Detail

Repository
rRSDB resultsdb
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yashn created this revision.Jun 25 2017, 5:41 AM
jskladan requested changes to this revision.Jun 26 2017, 2:10 PM
jskladan added a subscriber: jskladan.

Some minor issues, mostly WRT keeping the new code in the same style as the already existing pieces. My only actual functional issue here is that I'd probably just plain allowed _sort to manipulate submit_time - other fields of the Result object are IMO not really that relevant for custom-sorting, and the "exciting" user data (which I suppose could be more interesting to sort by) is stored in ResultData (and this inaccessible by this patch) anyway.

The idea behind the code is pretty sound, and I love that you also added unit tests! I just believe that from the implementation/usability standpoint, it is better to just say "you can sort by the time asc/desc" to keep the expectations right. If it makes any sense.

resultsdb/controllers/api_v2.py
315–327

Rather than having the default value of _sort='desc:submit_time', make it None, and change the underlying code respectively (None equals default behaviour).

320

Instead of non-capturing group, you could just r'^(?P<order>asc|desc):(?P<column>.+)$' or even r'^(?P<order>asc|desc):(?P<column>%s)$' % ('|'.join(db_columns)) and just have the regexp sort the "is this one of the columns I deem allowed" for you.

Ideally, I'd just honestly allow sorting only by submit_time. Given how the data is structured, it is the (IMO) only field directly from the Result object, which makes sense to sort-by. And if I wanted to sort by the extra data (ResultData) then the whole code would need to be rehauled anyway.

323

While this is very clever, I'd rather see a bit more explicit sort_order = {'asc': db.asc, 'desc': db.desc}[sort_match.group('order')].

435

As in the comment above. In the spirit of keeping the code style, please have the default value be an empty string, and just ensure the default behavior in that case.

478

ditto

This revision now requires changes to proceed.Jun 26 2017, 2:10 PM
yashn updated this revision to Diff 3077.Jun 26 2017, 7:19 PM
jskladan accepted this revision.Jun 27 2017, 8:25 AM

Looks good. I fixed a non-issue lint error and merged the patch. Thanks!

This revision is now accepted and ready to land.Jun 27 2017, 8:25 AM
Closed by commit rRSDB6ccbbed6c109: Sorting by key plugin for browse results collection (authored by yashn, committed by Josef Skladanka <jskladan@redhat.com>). · Explain WhyJun 27 2017, 9:22 AM
This revision was automatically updated to reflect the committed changes.