contrib/alchemy: Use sort_attribute as default sort for instances, support DESC sorting #152
Conversation
|
There's an existing PR in #138 for sort attribute. While your implementation is more proper in that it edits the Would you be willing to update your code to use @luord's syntax and documentation? If not, let me know and I will do this after merging. |
…pport DESC sorting
|
Correct me if I'm wrong, but wouldn't this do the sort only for SQLAlchemy? What I did in my PR was at the resource level, independent of the manager. It was a while ago and I don't remember everything about the code so I could be mistaken, though. |
|
@luord correct, this is for sqlalchemy. @lyschoening I've ported the relevant changes from #138 in this branch, FWIW |
| response = self.client.get('/machine?sort={"type": false}') | ||
| self.assert200(response) | ||
| type_uris = [entry['type']['$ref'] for entry in response.json] | ||
| self.assertTrue(type_uris, [bbb_uri, aaa_uri]) |
There was a problem hiding this comment.
if we pass true of false the test check the same thing
| @@ -86,6 +87,14 @@ def _init_model(self, resource, model, meta): | |||
| fs.required.add(name) | |||
| fs.set(name, self._get_field_from_column_type(column, name, io=io)) | |||
|
|
|||
| def _get_sort_expression(self, model, meta, id_column): | |||
| if meta.sort_attribute is None: | |||
| return id_column.asc() | |||
There was a problem hiding this comment.
Does it sort everything by default on primary key column ?
What if we do not want to have this behaviour ?
Sorting can have substantial impact on performance for instance.
There was a problem hiding this comment.
That has been the behavior for a while. Without an intrinsic sort order, it is not possible to do reliable pagination. Since the primary key is indexed, it is a good default. Do you see a practical scenario where no sort is preferred? We could support sort_attribute=False to mean no sorting.
There was a problem hiding this comment.
Thanks for the feedback, that's a good point.
I'd take inspiration from django. If Pagination is activated and if the query is not sorted, we could raise a warning. But having a default on primary key is not that interesting for our use case (we would prefer on last_updated for instance). Though, not a big deal either. Feel free to ignore.
Use the
sort_attributeif defined instead ofid_attributeto sort instances by default.Also, allow reversing default sorting by prefixing the attribute name with
-