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

contrib/alchemy: Use sort_attribute as default sort for instances, support DESC sorting #152

Merged
merged 2 commits into from
Nov 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions docs/resources.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ for the resource.

:class:`ModelResource` is written for create, read, update, delete actions on collections of items matching the resource schema.

A data store connection is maintained by a :class:`manager.Manager` instance.
The manager class can be specified in ``Meta.manager``; if no manager is specified, ``Api.default_manager`` is used.
Managers are configured through attributes in ``Meta``. Most managers expect a *model* to be defined under ``Meta.model``.

.. autoclass:: ModelResource
:members:

18 changes: 16 additions & 2 deletions flask_potion/contrib/alchemy/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def _init_model(self, resource, model, meta):
self.id_attribute = mapper.primary_key[0].name

self.id_field = self._get_field_from_column_type(self.id_column, self.id_attribute, io="r")
self.default_sort_expression = self.id_column.asc()
self.default_sort_expression = self._get_sort_expression(
model, meta, self.id_column)

fs = resource.schema
if meta.include_id:
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

reference:
https://github.com/django/django/blob/ed4bfacb3c942d0d32795e1a733b1b9367afd2e9/django/core/paginator.py#L110


attr_name, reverse = meta.sort_attribute
attr = getattr(model, attr_name)
return attr.desc() if reverse else attr.asc()

def _get_field_from_column_type(self, column, attribute, io="rw"):
args = ()
kwargs = {}
Expand Down Expand Up @@ -195,7 +204,12 @@ def _query_order_by(self, query, sort=None):
if isinstance(field, fields.ToOne):
target_alias = aliased(field.target.meta.model)
query = query.outerjoin(target_alias, column).reset_joinpoint()
column = getattr(target_alias, field.target.meta.sort_attribute or field.target.manager.id_attribute)
sort_attribute = None
if field.target.meta.sort_attribute:
sort_attribute, _ = field.target.meta.sort_attribute
column = getattr(
target_alias,
sort_attribute or field.target.manager.id_attribute)

order_clauses.append(column.desc() if reverse else column.asc())

Expand Down
19 changes: 18 additions & 1 deletion flask_potion/resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,28 @@ def __new__(mcs, name, bases, members):
if 'model' in changes or 'model' in meta and 'manager' in changes:
if meta.manager is not None:
class_.manager = meta.manager(class_, meta.model)

sort_attribute = meta.get('sort_attribute')
if sort_attribute is not None and isinstance(sort_attribute, str):
meta.sort_attribute = sort_attribute, False

return class_


class ModelResource(six.with_metaclass(ModelResourceMeta, Resource)):
"""
:class:`Meta` class attributes:

===================== ============================== ==============================================================================
Attribute name Default Description
===================== ============================== ==============================================================================
manager ``Api.default_manager`` A data store connection is maintained by a :class:`manager.Manager` instance.
Managers are configured through attributes in ``Meta``. Most managers expect
a *model* to be defined under ``Meta.model``.
sort_attribute None The field used to sort the list in the `instances` endpoint. Can be the
field name as ``string`` or a ``tuple`` with the field name and a boolean
for ``reverse`` (defaults to ``False``).
===================== ============================== ==============================================================================

.. method:: create

Expand Down Expand Up @@ -322,4 +339,4 @@ class Meta:
RefKey(),
IDKey()
)
natural_key = None
natural_key = None
75 changes: 75 additions & 0 deletions tests/contrib/alchemy/test_manager_sqlalchemy.py
Original file line number Diff line number Diff line change
Expand Up @@ -448,3 +448,78 @@ class Meta:
"$id": 1,
"username": "foo"
}, response.json)


class SQLAlchemySortTestCase(BaseTestCase):

def setUp(self):
super(SQLAlchemySortTestCase, self).setUp()
self.app.config['SQLALCHEMY_ENGINE'] = 'sqlite://'
self.api = Api(self.app)
self.sa = sa = SQLAlchemy(
self.app, session_options={"autoflush": False})

class Type(sa.Model):
id = sa.Column(sa.Integer, primary_key=True)
name = sa.Column(sa.String(60), nullable=False)

class Machine(sa.Model):
id = sa.Column(sa.Integer, primary_key=True)
name = sa.Column(sa.String(60), nullable=False)

type_id = sa.Column(sa.Integer, sa.ForeignKey(Type.id))
type = sa.relationship(Type, foreign_keys=[type_id])

sa.create_all()

class MachineResource(ModelResource):
class Meta:
model = Machine

class Schema:
type = fields.ToOne('type')

class TypeResource(ModelResource):
class Meta:
model = Type
sort_attribute = ('name', True)

self.MachineResource = MachineResource
self.TypeResource = TypeResource

self.api.add_resource(MachineResource)
self.api.add_resource(TypeResource)

def test_default_sorting_with_desc(self):
self.client.post('/type', data={"name": "aaa"})
self.client.post('/type', data={"name": "ccc"})
self.client.post('/type', data={"name": "bbb"})
response = self.client.get('/type')
self.assert200(response)
self.assertJSONEqual(
[{'$uri': '/type/2', 'name': 'ccc'},
{'$uri': '/type/3', 'name': 'bbb'},
{'$uri': '/type/1', 'name': 'aaa'}],
response.json)

def test_sort_by_related_field(self):
response = self.client.post('/type', data={"name": "aaa"})
self.assert200(response)
aaa_uri = response.json["$uri"]
response = self.client.post('/type', data={"name": "bbb"})
self.assert200(response)
bbb_uri = response.json["$uri"]
self.client.post(
'/machine', data={"name": "foo", "type": {"$ref": aaa_uri}})
self.assert200(response)
self.client.post(
'/machine', data={"name": "bar", "type": {"$ref": bbb_uri}})
self.assert200(response)
response = self.client.get('/machine?sort={"type": true}')
self.assert200(response)
type_uris = [entry['type']['$ref'] for entry in response.json]
self.assertTrue(type_uris, [bbb_uri, aaa_uri])
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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we pass true of false the test check the same thing