Normalize sorting
The idea behind this patch is to disable default sorting for inner listing operation, because it's not requred there, and leave it only for api calls. With this patch we also remove two unnecessary auxiliary functions from db api: _get_collection_sorted_by_time and _get_collection_sorted_by_name. Currently they are used to specify default parameters for 'sort_keys', but unfortunately they do it wrong, which leads to confusion and incorrect behavior of the system, when _get_collection_sorted_by_name is used to sort by time [1], when mutable is passed as a default parameter value and then modified in the code[2][3] (this is one of the most famous python anti-patterns [4]). To cope with non-determinism in tests instead of assertEqual method we use _assert_single_item, which allows to check results regardless of their order. Therefore, the patch allows to increase the performance of Mistral and make the code cleaner. [1] https://github.com/openstack/mistral/blob/stable/pike/mistral/db/v2/sqlalchemy/api.py#L528-L539 [2] https://github.com/openstack/mistral/blob/stable/pike/mistral/db/v2/sqlalchemy/api.py#L235 [3] https://github.com/openstack/mistral/blob/stable/pike/mistral/db/v2/sqlalchemy/api.py#L183-L184 [4] https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html Change-Id: Ib4380f883969cab4ab851cd910c830e5ce30dfe5
This commit is contained in:
Mike Fedosin
parent
a741d03fad
commit
1b6856d5ef
|
|
@ -174,7 +174,7 @@ def load_action_definition(name):
|
|||
return IMPL.load_action_definition(name)
|
||||
|
||||
|
||||
def get_action_definitions(limit=None, marker=None, sort_keys=['name'],
|
||||
def get_action_definitions(limit=None, marker=None, sort_keys=None,
|
||||
sort_dirs=None, **kwargs):
|
||||
return IMPL.get_action_definitions(
|
||||
limit=limit,
|
||||
|
|
@ -255,7 +255,7 @@ def load_workflow_execution(name):
|
|||
return IMPL.load_workflow_execution(name)
|
||||
|
||||
|
||||
def get_workflow_executions(limit=None, marker=None, sort_keys=['created_at'],
|
||||
def get_workflow_executions(limit=None, marker=None, sort_keys=None,
|
||||
sort_dirs=None, **kwargs):
|
||||
return IMPL.get_workflow_executions(
|
||||
limit=limit,
|
||||
|
|
@ -301,7 +301,7 @@ def load_task_execution(id):
|
|||
return IMPL.load_task_execution(id)
|
||||
|
||||
|
||||
def get_task_executions(limit=None, marker=None, sort_keys=['created_at'],
|
||||
def get_task_executions(limit=None, marker=None, sort_keys=None,
|
||||
sort_dirs=None, **kwargs):
|
||||
return IMPL.get_task_executions(
|
||||
limit=limit,
|
||||
|
|
@ -447,7 +447,7 @@ def load_environment(name):
|
|||
return IMPL.load_environment(name)
|
||||
|
||||
|
||||
def get_environments(limit=None, marker=None, sort_keys=['name'],
|
||||
def get_environments(limit=None, marker=None, sort_keys=None,
|
||||
sort_dirs=None, **kwargs):
|
||||
|
||||
return IMPL.get_environments(
|
||||
|
|
|
|||
|
|
@ -178,7 +178,9 @@ def _paginate_query(model, limit=None, marker=None, sort_keys=None,
|
|||
|
||||
sort_keys = sort_keys if sort_keys else []
|
||||
|
||||
if 'id' not in sort_keys:
|
||||
# We should add sorting by id only if we use pagination. Otherwise
|
||||
# we can omit it to increase the performance.
|
||||
if (marker or limit) and 'id' not in sort_keys:
|
||||
sort_keys.append('id')
|
||||
sort_dirs.append('asc') if sort_dirs else None
|
||||
|
||||
|
|
@ -224,28 +226,6 @@ def _get_collection(model, insecure=False, limit=None, marker=None,
|
|||
return query.all()
|
||||
|
||||
|
||||
def _get_collection_sorted_by_name(model, insecure=False, fields=None,
|
||||
sort_keys=['name'], **kwargs):
|
||||
return _get_collection(
|
||||
model=model,
|
||||
insecure=insecure,
|
||||
sort_keys=sort_keys,
|
||||
fields=fields,
|
||||
**kwargs
|
||||
)
|
||||
|
||||
|
||||
def _get_collection_sorted_by_time(model, insecure=False, fields=None,
|
||||
sort_keys=['created_at'], **kwargs):
|
||||
return _get_collection(
|
||||
model=model,
|
||||
insecure=insecure,
|
||||
sort_keys=sort_keys,
|
||||
fields=fields,
|
||||
**kwargs
|
||||
)
|
||||
|
||||
|
||||
def _get_db_object_by_name(model, name, filter_=None, order_by=None):
|
||||
|
||||
query = _secure_query(model)
|
||||
|
|
@ -404,7 +384,7 @@ def load_workbook(name, session=None):
|
|||
|
||||
@b.session_aware()
|
||||
def get_workbooks(session=None, **kwargs):
|
||||
return _get_collection_sorted_by_name(models.Workbook, **kwargs)
|
||||
return _get_collection(models.Workbook, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -511,15 +491,13 @@ def load_workflow_definition(name, namespace='', session=None):
|
|||
|
||||
|
||||
@b.session_aware()
|
||||
def get_workflow_definitions(sort_keys=['created_at'], fields=None,
|
||||
session=None, **kwargs):
|
||||
def get_workflow_definitions(fields=None, session=None, **kwargs):
|
||||
if fields and 'input' in fields:
|
||||
fields.remove('input')
|
||||
fields.append('spec')
|
||||
|
||||
return _get_collection_sorted_by_name(
|
||||
return _get_collection(
|
||||
model=models.WorkflowDefinition,
|
||||
sort_keys=sort_keys,
|
||||
fields=fields,
|
||||
**kwargs
|
||||
)
|
||||
|
|
@ -655,10 +633,7 @@ def load_action_definition(name, session=None):
|
|||
|
||||
@b.session_aware()
|
||||
def get_action_definitions(session=None, **kwargs):
|
||||
return _get_collection_sorted_by_name(
|
||||
model=models.ActionDefinition,
|
||||
**kwargs
|
||||
)
|
||||
return _get_collection(model=models.ActionDefinition, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -781,7 +756,7 @@ def delete_action_executions(session=None, **kwargs):
|
|||
|
||||
|
||||
def _get_action_executions(**kwargs):
|
||||
return _get_collection_sorted_by_time(models.ActionExecution, **kwargs)
|
||||
return _get_collection(models.ActionExecution, **kwargs)
|
||||
|
||||
|
||||
# Workflow executions.
|
||||
|
|
@ -816,10 +791,7 @@ def ensure_workflow_execution_exists(id, session=None):
|
|||
|
||||
@b.session_aware()
|
||||
def get_workflow_executions(session=None, **kwargs):
|
||||
return _get_collection_sorted_by_time(
|
||||
models.WorkflowExecution,
|
||||
**kwargs
|
||||
)
|
||||
return _get_collection(models.WorkflowExecution, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -994,7 +966,7 @@ def delete_task_executions(session=None, **kwargs):
|
|||
|
||||
|
||||
def _get_task_executions(**kwargs):
|
||||
return _get_collection_sorted_by_time(models.TaskExecution, **kwargs)
|
||||
return _get_collection(models.TaskExecution, **kwargs)
|
||||
|
||||
|
||||
# Delayed calls.
|
||||
|
|
@ -1073,10 +1045,7 @@ def get_delayed_call(id, session=None):
|
|||
|
||||
@b.session_aware()
|
||||
def get_delayed_calls(session=None, **kwargs):
|
||||
return _get_collection(
|
||||
model=models.DelayedCall,
|
||||
**kwargs
|
||||
)
|
||||
return _get_collection(model=models.DelayedCall, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -1160,12 +1129,8 @@ def load_cron_trigger(identifier, session=None):
|
|||
|
||||
|
||||
@b.session_aware()
|
||||
def get_cron_triggers(insecure=False, session=None, **kwargs):
|
||||
return _get_collection_sorted_by_name(
|
||||
models.CronTrigger,
|
||||
insecure=insecure,
|
||||
**kwargs
|
||||
)
|
||||
def get_cron_triggers(session=None, **kwargs):
|
||||
return _get_collection(models.CronTrigger, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -1284,7 +1249,7 @@ def load_environment(name, session=None):
|
|||
|
||||
@b.session_aware()
|
||||
def get_environments(session=None, **kwargs):
|
||||
return _get_collection_sorted_by_name(models.Environment, **kwargs)
|
||||
return _get_collection(models.Environment, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
@ -1514,12 +1479,8 @@ def load_event_trigger(id, insecure=False, session=None):
|
|||
|
||||
|
||||
@b.session_aware()
|
||||
def get_event_triggers(insecure=False, session=None, **kwargs):
|
||||
return _get_collection_sorted_by_time(
|
||||
model=models.EventTrigger,
|
||||
insecure=insecure,
|
||||
**kwargs
|
||||
)
|
||||
def get_event_triggers(session=None, **kwargs):
|
||||
return _get_collection(model=models.EventTrigger, **kwargs)
|
||||
|
||||
|
||||
@b.session_aware()
|
||||
|
|
|
|||
|
|
@ -145,8 +145,8 @@ class WorkbookTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workbooks()
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_workbooks_by_equal_value(self):
|
||||
db_api.create_workbook(WORKBOOKS[0])
|
||||
|
|
@ -203,8 +203,8 @@ class WorkbookTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workbooks(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_workbooks_by_less_than_value(self):
|
||||
created0 = db_api.create_workbook(WORKBOOKS[0])
|
||||
|
|
@ -232,8 +232,8 @@ class WorkbookTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workbooks(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_workbooks_by_values_in_list(self):
|
||||
created0 = db_api.create_workbook(WORKBOOKS[0])
|
||||
|
|
@ -489,8 +489,8 @@ class WorkflowDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workflow_definitions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_workflow_definition_by_less_than_value(self):
|
||||
created0 = db_api.create_workflow_definition(WF_DEFINITIONS[0])
|
||||
|
|
@ -518,8 +518,8 @@ class WorkflowDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workflow_definitions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_workflow_definition_by_values_in_list(self):
|
||||
created0 = db_api.create_workflow_definition(WF_DEFINITIONS[0])
|
||||
|
|
@ -782,8 +782,8 @@ class WorkflowDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workflow_definitions()
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_delete_workflow_definition(self):
|
||||
created0 = db_api.create_workflow_definition(WF_DEFINITIONS[0])
|
||||
|
|
@ -996,8 +996,8 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_action_definitions_by_greater_than_value(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1028,9 +1028,9 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(3, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self.assertEqual(created2, fetched[2])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
self._assert_single_item(fetched, name=created2['name'])
|
||||
|
||||
def test_filter_action_definitions_by_less_than_value(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1045,8 +1045,8 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_action_definitions_by_less_than_equal_value(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1061,9 +1061,9 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(3, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self.assertEqual(created2, fetched[2])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
self._assert_single_item(fetched, name=created2['name'])
|
||||
|
||||
def test_filter_action_definitions_by_values_in_list(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1079,8 +1079,8 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_action_definitions_by_values_notin_list(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1095,7 +1095,7 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(**_filter)
|
||||
|
||||
self.assertEqual(1, len(fetched))
|
||||
self.assertEqual(created2, fetched[0])
|
||||
self._assert_single_item(fetched, name=created2['name'])
|
||||
|
||||
def test_filter_action_definitions_by_multiple_columns(self):
|
||||
created0 = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1204,8 +1204,8 @@ class ActionDefinitionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_action_definitions(is_system=True)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_delete_action_definition_with_name(self):
|
||||
created = db_api.create_action_definition(ACTION_DEFINITIONS[0])
|
||||
|
|
@ -1606,8 +1606,8 @@ class WorkflowExecutionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workflow_executions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, state=created0['state'])
|
||||
self._assert_single_item(fetched, state=created1['state'])
|
||||
|
||||
def test_filter_workflow_execution_by_less_than_value(self):
|
||||
with db_api.transaction():
|
||||
|
|
@ -1637,8 +1637,8 @@ class WorkflowExecutionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_workflow_executions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, state=created0['state'])
|
||||
self._assert_single_item(fetched, state=created1['state'])
|
||||
|
||||
def test_filter_workflow_execution_by_values_in_list(self):
|
||||
with db_api.transaction():
|
||||
|
|
@ -1966,8 +1966,8 @@ class TaskExecutionTest(SQLAlchemyTest):
|
|||
)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_task_execution_by_equal_value(self):
|
||||
created, _ = self._create_task_executions()
|
||||
|
|
@ -2020,8 +2020,8 @@ class TaskExecutionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_task_executions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_task_execution_by_less_than_value(self):
|
||||
created0, created1 = self._create_task_executions()
|
||||
|
|
@ -2047,8 +2047,8 @@ class TaskExecutionTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_task_executions(**_filter)
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_filter_task_execution_by_values_in_list(self):
|
||||
created, _ = self._create_task_executions()
|
||||
|
|
@ -2329,8 +2329,8 @@ class CronTriggerTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_cron_triggers(pattern='* * * * *')
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_get_cron_trigger(self):
|
||||
cron_trigger = db_api.create_cron_trigger(CRON_TRIGGERS[0])
|
||||
|
|
@ -2531,8 +2531,8 @@ class EnvironmentTest(SQLAlchemyTest):
|
|||
fetched = db_api.get_environments()
|
||||
|
||||
self.assertEqual(2, len(fetched))
|
||||
self.assertEqual(created0, fetched[0])
|
||||
self.assertEqual(created1, fetched[1])
|
||||
self._assert_single_item(fetched, name=created0['name'])
|
||||
self._assert_single_item(fetched, name=created1['name'])
|
||||
|
||||
def test_delete_environment(self):
|
||||
created = db_api.create_environment(ENVIRONMENTS[0])
|
||||
|
|
|
|||
|
|
@ -127,7 +127,7 @@ def filters_to_dict(**kwargs):
|
|||
|
||||
def get_all(list_cls, cls, get_all_function, get_function,
|
||||
resource_function=None, marker=None, limit=None,
|
||||
sort_keys='created_at', sort_dirs='asc', fields='',
|
||||
sort_keys=None, sort_dirs=None, fields=None,
|
||||
all_projects=False, **filters):
|
||||
"""Return a list of cls.
|
||||
|
||||
|
|
@ -143,11 +143,11 @@ def get_all(list_cls, cls, get_all_function, get_function,
|
|||
:param limit: Optional. Maximum number of resources to return in a
|
||||
single result. Default value is None for backward
|
||||
compatibility.
|
||||
:param sort_keys: Optional. Columns to sort results by.
|
||||
Default: created_at.
|
||||
:param sort_dirs: Optional. Directions to sort corresponding to
|
||||
:param sort_keys: Optional. List of columns to sort results by.
|
||||
Default: ['created_at'].
|
||||
:param sort_dirs: Optional. List of directions to sort corresponding to
|
||||
sort_keys, "asc" or "desc" can be chosen.
|
||||
Default: asc.
|
||||
Default: ['asc'].
|
||||
:param fields: Optional. A specified list of fields of the resource to
|
||||
be returned. 'id' will be included automatically in
|
||||
fields if it's provided, since it will be used when
|
||||
|
|
@ -155,6 +155,10 @@ def get_all(list_cls, cls, get_all_function, get_function,
|
|||
:param filters: Optional. A specified dictionary of filters to match.
|
||||
:param all_projects: Optional. Get resources of all projects.
|
||||
"""
|
||||
sort_keys = ['created_at'] if sort_keys is None else sort_keys
|
||||
sort_dirs = ['asc'] if sort_dirs is None else sort_dirs
|
||||
fields = [] if fields is None else fields
|
||||
|
||||
if fields and 'id' not in fields:
|
||||
fields.insert(0, 'id')
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue