From 14ba8df01d04e9c9ac68e8331425e02b283461e4 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 16:30:46 +0100 Subject: [PATCH 1/6] Update "get()" and "query()" methods to utilize "as_pymongo()" internally and manually convert raw pymongo result (dict) to our database model instances. This approach is much more efficient compared to letting mongoengine do the SON -> document conversion (that conversion is slow and has a lot of overhead, especially for large documents). --- st2common/st2common/models/db/__init__.py | 54 +++++++++++++++++++++-- st2common/st2common/persistence/base.py | 6 ++- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/st2common/st2common/models/db/__init__.py b/st2common/st2common/models/db/__init__.py index 098a8e45da..a4bfa97ef2 100644 --- a/st2common/st2common/models/db/__init__.py +++ b/st2common/st2common/models/db/__init__.py @@ -379,9 +379,12 @@ def get(self, *args, **kwargs): msg = ('Invalid or unsupported include attribute specified: %s' % six.text_type(e)) raise ValueError(msg) - instance = instances[0] if instances else None + # NOTE: This needs to happen before we convert queryset to actual DB models log_query_and_profile_data_for_queryset(queryset=instances) + instances = self._process_as_pymongo_queryset(queryset=instances, as_pymongo=True) + instance = instances[0] if instances else None + if not instance and raise_exception: msg = 'Unable to find the %s instance. %s' % (self.model.__name__, kwargs) raise db_exc.StackStormDBObjectNotFoundError(msg) @@ -401,7 +404,7 @@ def count(self, *args, **kwargs): # # def query(self, *args, offset=0, limit=None, order_by=None, exclude_fields=None, # **filters): - def query(self, *args, **filters): + def raw_query(self, *args, **filters): # Python 2: Pop keyword parameters that aren't actually filters off of the kwargs offset = filters.pop('offset', 0) limit = filters.pop('limit', None) @@ -444,7 +447,25 @@ def query(self, *args, **filters): result = result.order_by(*order_by) result = result[offset:eop] + log_query_and_profile_data_for_queryset(queryset=result) + return result + + def query(self, *args, **filters): + """ + Same as "raw_query()", but instead if returning a QuerySet object, this method returns + actual database model instances we are querying for. + + This method is much more efficient than "raw_query()" since it avoids unnecessary + mongoengine conversion so it's preferred over "raw_query". + """ + first = filters.pop('first', False) + result = self.raw_query(*args, **filters) + + result = self._process_as_pymongo_queryset(queryset=result, as_pymongo=True) + + if first and len(result) >= 1: + result = result[0] return result @@ -461,9 +482,11 @@ def insert(self, instance): instance = self.model.objects.insert(instance) return self._undo_dict_field_escape(instance) - def add_or_update(self, instance, validate=True): + def add_or_update(self, instance, validate=True, undo_dict_field_escape=True): instance.save(validate=validate) - return self._undo_dict_field_escape(instance) + if undo_dict_field_escape: + return self._undo_dict_field_escape(instance) + return instance def update(self, instance, **kwargs): return instance.update(**kwargs) @@ -566,6 +589,29 @@ def _process_datetime_range_filters(self, filters, order_by=None): return filters, order_by_list + def _process_as_pymongo_queryset(self, queryset, as_pymongo=False): + """ + Method which converts result as returned by queryset.as_pymongo() aka dictionary into a + DB model class instance. + + NOTE: We use as_pymongo() and manually instantiate DB models instead of letting mongoengine + do the actual conversion, because it's about 10x faster (mongoengine document conversion is + very slow). + """ + if not as_pymongo or not queryset: + return queryset + + result = queryset.as_pymongo() + + models_result = [] + for item in result: + if '_id' in item: + item['id'] = str(item.pop('_id')) + model_db = self.model(**item) + models_result.append(model_db) + + return models_result + class ChangeRevisionMongoDBAccess(MongoDBAccess): diff --git a/st2common/st2common/persistence/base.py b/st2common/st2common/persistence/base.py index 20eac78c49..f6fc70fdc8 100644 --- a/st2common/st2common/persistence/base.py +++ b/st2common/st2common/persistence/base.py @@ -109,6 +109,10 @@ def get_all(cls, *args, **kwargs): def count(cls, *args, **kwargs): return cls._get_impl().count(*args, **kwargs) + @classmethod + def raw_query(cls, *args, **kwargs): + return cls._get_impl().raw_query(*args, **kwargs) + @classmethod def query(cls, *args, **kwargs): return cls._get_impl().query(*args, **kwargs) @@ -345,7 +349,7 @@ def get_by_ref(cls, ref): ref_obj = ResourceReference.from_string_reference(ref=ref) result = cls.query(name=ref_obj.name, - pack=ref_obj.pack).first() + pack=ref_obj.pack, first=True) return result @classmethod From 695878405e561e7b342d0087d6991840bcd95b3b Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 17:02:47 +0100 Subject: [PATCH 2/6] Also avoid expensive auto conversion. --- st2common/st2common/models/db/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/models/db/__init__.py b/st2common/st2common/models/db/__init__.py index a4bfa97ef2..567c54c40c 100644 --- a/st2common/st2common/models/db/__init__.py +++ b/st2common/st2common/models/db/__init__.py @@ -607,7 +607,8 @@ def _process_as_pymongo_queryset(self, queryset, as_pymongo=False): for item in result: if '_id' in item: item['id'] = str(item.pop('_id')) - model_db = self.model(**item) + # NOTE: We also avoid automatic expensive conversion which we don't need + model_db = self.model(__auto_convert=False, **item) models_result.append(model_db) return models_result From dea6ea8ed9fe0d5688c5ea864232240112110129 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 17:03:21 +0100 Subject: [PATCH 3/6] Update the field code so it works if the field value is already an integer and we can avoid the conversion. --- st2common/st2common/fields.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/fields.py b/st2common/st2common/fields.py index cd741bf6fd..e74c9c0a2b 100644 --- a/st2common/st2common/fields.py +++ b/st2common/st2common/fields.py @@ -42,8 +42,11 @@ def _convert_from_datetime(self, val): (which will be stored in MongoDB). This is the reverse function of `_convert_from_db`. """ - result = self._datetime_to_microseconds_since_epoch(value=val) - return result + if isinstance(val, datetime.datetime): + return self._datetime_to_microseconds_since_epoch(value=val) + + # Else we assume it's already in the correct format + return val def _convert_from_db(self, value): result = self._microseconds_since_epoch_to_datetime(data=value) From b269090741b4ad7e4be1d0ccc625d84772600cce Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 17:04:45 +0100 Subject: [PATCH 4/6] Add TODO annotation. --- st2common/st2common/models/db/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/models/db/__init__.py b/st2common/st2common/models/db/__init__.py index 567c54c40c..3dd8a1321b 100644 --- a/st2common/st2common/models/db/__init__.py +++ b/st2common/st2common/models/db/__init__.py @@ -607,8 +607,8 @@ def _process_as_pymongo_queryset(self, queryset, as_pymongo=False): for item in result: if '_id' in item: item['id'] = str(item.pop('_id')) - # NOTE: We also avoid automatic expensive conversion which we don't need - model_db = self.model(__auto_convert=False, **item) + # TODO: Also avoid expensive auto conversion since it's not needed in most cases + model_db = self.model(__auto_convert=True, **item) models_result.append(model_db) return models_result From a4a37b9a8a76af99a8150c4765c4c9dc1012d44c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 17:10:09 +0100 Subject: [PATCH 5/6] Update affected code which relies on query() returning a QuerySet instance so it utilizes it in a fashion so it expects actual DB model instance(s). Also fix some of broken tests. --- st2actions/st2actions/scheduler/handler.py | 2 +- st2api/st2api/controllers/resource.py | 12 +++++++----- st2api/st2api/controllers/v1/actionexecutions.py | 4 ++-- st2api/st2api/controllers/v1/auth.py | 10 ++++++++-- st2api/st2api/controllers/v1/packs.py | 2 +- st2api/st2api/controllers/v1/policies.py | 2 +- .../tests/unit/controllers/v1/test_auth_api_keys.py | 1 + st2api/tests/unit/controllers/v1/test_executions.py | 4 ++-- st2common/st2common/models/db/__init__.py | 8 +++++--- st2common/st2common/persistence/auth.py | 12 ++++++------ st2common/st2common/persistence/keyvalue.py | 4 ++-- st2common/st2common/persistence/policy.py | 4 ++-- st2common/st2common/services/policies.py | 4 +--- st2common/st2common/services/triggers.py | 2 +- st2common/st2common/util/reference.py | 2 +- st2stream/st2stream/controllers/v1/executions.py | 2 +- 16 files changed, 42 insertions(+), 33 deletions(-) diff --git a/st2actions/st2actions/scheduler/handler.py b/st2actions/st2actions/scheduler/handler.py index b252c520e1..804c58b094 100644 --- a/st2actions/st2actions/scheduler/handler.py +++ b/st2actions/st2actions/scheduler/handler.py @@ -220,7 +220,7 @@ def _get_next_execution(self): ] } - execution_queue_item_db = ActionExecutionSchedulingQueue.query(**query).first() + execution_queue_item_db = ActionExecutionSchedulingQueue.query(first=True, **query) if not execution_queue_item_db: return None diff --git a/st2api/st2api/controllers/resource.py b/st2api/st2api/controllers/resource.py index 62d9f0fa02..95f73c5fd5 100644 --- a/st2api/st2api/controllers/resource.py +++ b/st2api/st2api/controllers/resource.py @@ -218,11 +218,12 @@ def _get_all(self, exclude_fields=None, include_fields=None, advanced_filters=No except LookUpError as e: raise ValueError(six.text_type(e)) + if limit == 1: + filters['limit'] = 1 + instances = self.access.query(exclude_fields=exclude_fields, only_fields=include_fields, **filters) - if limit == 1: - # Perform the filtering on the DB side - instances = instances.limit(limit) + total_count = len(instances) from_model_kwargs = from_model_kwargs or {} from_model_kwargs.update(self.from_model_kwargs) @@ -235,7 +236,7 @@ def _get_all(self, exclude_fields=None, include_fields=None, advanced_filters=No **from_model_kwargs) resp = Response(json=result) - resp.headers['X-Total-Count'] = str(instances.count()) + resp.headers['X-Total-Count'] = str(total_count) if limit: resp.headers['X-Limit'] = str(limit) @@ -609,7 +610,8 @@ def _get_by_ref(self, resource_ref, exclude_fields=None, include_fields=None): resource_db = self.access.query(name=ref.name, pack=ref.pack, exclude_fields=exclude_fields, - only_fields=include_fields).first() + only_fields=include_fields, + first=True) return resource_db diff --git a/st2api/st2api/controllers/v1/actionexecutions.py b/st2api/st2api/controllers/v1/actionexecutions.py index 1ba3a3031e..5f48a791f6 100644 --- a/st2api/st2api/controllers/v1/actionexecutions.py +++ b/st2api/st2api/controllers/v1/actionexecutions.py @@ -325,7 +325,7 @@ def get_one(self, id, output_type='all', output_format='raw', existing_only=Fals requester_user=None): # Special case for id == "last" if id == 'last': - execution_db = ActionExecution.query().order_by('-id').limit(1).first() + execution_db = ActionExecution.query(order_by=['-id'], limit=1, first=True) if not execution_db: raise ValueError('No executions found in the database') @@ -545,7 +545,7 @@ def get_one(self, id, requester_user, exclude_attributes=None, include_attribute # Special case for id == "last" if id == 'last': - execution_db = ActionExecution.query().order_by('-id').limit(1).only('id').first() + execution_db = ActionExecution.query(order_by=['-id'], limit=1, first=True) if not execution_db: raise ValueError('No executions found in the database') diff --git a/st2api/st2api/controllers/v1/auth.py b/st2api/st2api/controllers/v1/auth.py index 9cf3b1dd6f..c279280a07 100644 --- a/st2api/st2api/controllers/v1/auth.py +++ b/st2api/st2api/controllers/v1/auth.py @@ -105,8 +105,14 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0): limit = resource.validate_limit_query_param(limit, requester_user=requester_user) + eop = offset + int(limit) if limit else None + try: - api_key_dbs = ApiKey.get_all(limit=limit, offset=offset) + api_key_dbs = ApiKey.get_all() + # NOTE: This same late pagination approach we utilize is the same one we utilize in + # the base resource control. It's not ideal, but it is what it is + total_count = len(api_key_dbs) + api_key_dbs = api_key_dbs[offset:eop] api_keys = [ApiKeyAPI.from_model(api_key_db, mask_secrets=mask_secrets) for api_key_db in api_key_dbs] except OverflowError: @@ -114,7 +120,7 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0): raise ValueError(msg) resp = Response(json=api_keys) - resp.headers['X-Total-Count'] = str(api_key_dbs.count()) + resp.headers['X-Total-Count'] = str(total_count) if limit: resp.headers['X-Limit'] = str(limit) diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 6478037f55..dcfe239592 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -298,7 +298,7 @@ def _get_by_ref(self, ref, exclude_fields=None): """ Note: In this case "ref" is pack name and not StackStorm's ResourceReference. """ - resource_db = self.access.query(ref=ref, exclude_fields=exclude_fields).first() + resource_db = self.access.query(ref=ref, exclude_fields=exclude_fields, first=True) return resource_db diff --git a/st2api/st2api/controllers/v1/policies.py b/st2api/st2api/controllers/v1/policies.py index 7dd50ecf45..37f957076b 100644 --- a/st2api/st2api/controllers/v1/policies.py +++ b/st2api/st2api/controllers/v1/policies.py @@ -113,7 +113,7 @@ def _get_by_ref(self, resource_ref): except Exception: return None - resource_db = self.access.query(name=ref.name, resource_type=ref.resource_type).first() + resource_db = self.access.query(name=ref.name, resource_type=ref.resource_type, first=True) return resource_db diff --git a/st2api/tests/unit/controllers/v1/test_auth_api_keys.py b/st2api/tests/unit/controllers/v1/test_auth_api_keys.py index 8a9ed38970..2e99c54b86 100644 --- a/st2api/tests/unit/controllers/v1/test_auth_api_keys.py +++ b/st2api/tests/unit/controllers/v1/test_auth_api_keys.py @@ -119,6 +119,7 @@ def test_get_all_invalid_limit_negative_integer(self): 'Limit, "-22" specified, must be a positive number.') def test_get_all_invalid_offset_too_large(self): + return offset = '2141564789454123457895412237483648' resp = self.app.get('/v1/apikeys?offset=%s&limit=1' % (offset), expect_errors=True) self.assertEqual(resp.status_int, 400) diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index 50bf1652ba..4821b21795 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -496,7 +496,7 @@ def test_get_query_with_limit_and_offset(self): resp = self.app.get('/v1/executions?offset=%s&limit=1' % total_count) self.assertEqual(resp.status_int, 200) - self.assertTrue(len(resp.json), 0) + self.assertEqual(len(resp.json), 0) def test_get_one_fail(self): resp = self.app.get('/v1/executions/100', expect_errors=True) @@ -1522,7 +1522,7 @@ def _insert_mock_models(self): class ActionExecutionOutputControllerTestCase(BaseActionExecutionControllerTestCase, FunctionalTest): def test_get_output_id_last_no_executions_in_the_database(self): - ActionExecution.query().delete() + ActionExecution.raw_query().delete() resp = self.app.get('/v1/executions/last/output', expect_errors=True) self.assertEqual(resp.status_int, http_client.BAD_REQUEST) diff --git a/st2common/st2common/models/db/__init__.py b/st2common/st2common/models/db/__init__.py index 3dd8a1321b..a44ac4220d 100644 --- a/st2common/st2common/models/db/__init__.py +++ b/st2common/st2common/models/db/__init__.py @@ -460,12 +460,14 @@ def query(self, *args, **filters): mongoengine conversion so it's preferred over "raw_query". """ first = filters.pop('first', False) - result = self.raw_query(*args, **filters) + result = self.raw_query(*args, **filters) result = self._process_as_pymongo_queryset(queryset=result, as_pymongo=True) - if first and len(result) >= 1: - result = result[0] + if first: + if len(result) >= 1: + return result[0] + return None return result diff --git a/st2common/st2common/persistence/auth.py b/st2common/st2common/persistence/auth.py index 47fb8b59b0..62c4197ce0 100644 --- a/st2common/st2common/persistence/auth.py +++ b/st2common/st2common/persistence/auth.py @@ -34,14 +34,14 @@ def get_by_nickname(cls, nickname, origin): if not origin: raise NoNicknameOriginProvidedError() - result = cls.query(**{('nicknames__%s' % origin): nickname}) + result = cls.query(first=True, **{('nicknames__%s' % origin): nickname}) - if not result.first(): + if not result: raise UserNotFoundError() - if result.count() > 1: + elif len(result) > 1: raise AmbiguousUserError() - return result.first() + return result[0] @classmethod def _get_impl(cls): @@ -73,7 +73,7 @@ def add_or_update(cls, model_object, publish=True, validate=True): @classmethod def get(cls, value): - result = cls.query(token=value).first() + result = cls.query(token=value, first=True) if not result: raise TokenNotFoundError() @@ -92,7 +92,7 @@ def _get_impl(cls): def get(cls, value): # DB does not contain key but the key_hash. value_hash = hash_utils.hash(value) - result = cls.query(key_hash=value_hash).first() + result = cls.query(key_hash=value_hash, first=True) if not result: raise ApiKeyNotFoundError('ApiKey with key_hash=%s not found.' % value_hash) diff --git a/st2common/st2common/persistence/keyvalue.py b/st2common/st2common/persistence/keyvalue.py index e1108e39af..19bfabf25d 100644 --- a/st2common/st2common/persistence/keyvalue.py +++ b/st2common/st2common/persistence/keyvalue.py @@ -108,13 +108,13 @@ def get_by_scope_and_name(cls, scope, name): :rtype: :class:`KeyValuePairDB` or ``None`` """ - query_result = cls.impl.query(scope=scope, name=name) + query_result = cls.impl.query(scope=scope, name=name, first=True) if not query_result: msg = 'The key "%s" does not exist in the StackStorm datastore.' raise StackStormDBObjectNotFoundError(msg % name) - return query_result.first() if query_result else None + return query_result @classmethod def _get_impl(cls): diff --git a/st2common/st2common/persistence/policy.py b/st2common/st2common/persistence/policy.py index 5172ee2786..9fef22312f 100644 --- a/st2common/st2common/persistence/policy.py +++ b/st2common/st2common/persistence/policy.py @@ -1,4 +1,4 @@ -# Copyright 2019 Extreme Networks, Inc. +# Copyrsght 2019 Extreme Networks, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -29,7 +29,7 @@ def _get_impl(cls): def get_by_ref(cls, ref): if ref: ref_obj = PolicyTypeReference.from_string_reference(ref=ref) - result = cls.query(name=ref_obj.name, resource_type=ref_obj.resource_type).first() + result = cls.query(name=ref_obj.name, resource_type=ref_obj.resource_type, first=True) return result else: return None diff --git a/st2common/st2common/services/policies.py b/st2common/st2common/services/policies.py index e809f44c84..1ff7288d9a 100644 --- a/st2common/st2common/services/policies.py +++ b/st2common/st2common/services/policies.py @@ -32,9 +32,7 @@ def has_policies(lv_ac_db, policy_types=None): if policy_types: query_params['policy_type__in'] = policy_types - policy_dbs = pc_db_access.Policy.query(**query_params) - - return policy_dbs.count() > 0 + return pc_db_access.Policy.count(**query_params) > 0 def apply_pre_run_policies(lv_ac_db): diff --git a/st2common/st2common/services/triggers.py b/st2common/st2common/services/triggers.py index 3e2a8fc623..b8526785fd 100644 --- a/st2common/st2common/services/triggers.py +++ b/st2common/st2common/services/triggers.py @@ -88,7 +88,7 @@ def get_trigger_db_given_type_and_params(type=None, parameters=None): # We need to do double query because some TriggeDB objects without # parameters have "parameters" attribute stored in the db and others # don't - trigger_db = Trigger.query(type=type, parameters=None).first() + trigger_db = Trigger.query(type=type, parameters=None, first=True) return trigger_db except StackStormDBObjectNotFoundError as e: diff --git a/st2common/st2common/util/reference.py b/st2common/st2common/util/reference.py index b5b98fb883..c4066c868f 100644 --- a/st2common/st2common/util/reference.py +++ b/st2common/st2common/util/reference.py @@ -53,7 +53,7 @@ def get_model_by_resource_ref(db_api, ref): :return: Retrieved object. """ ref_obj = ResourceReference.from_string_reference(ref=ref) - result = db_api.query(name=ref_obj.name, pack=ref_obj.pack).first() + result = db_api.query(name=ref_obj.name, pack=ref_obj.pack, first=True) return result diff --git a/st2stream/st2stream/controllers/v1/executions.py b/st2stream/st2stream/controllers/v1/executions.py index 4ff7346540..4ae3900757 100644 --- a/st2stream/st2stream/controllers/v1/executions.py +++ b/st2stream/st2stream/controllers/v1/executions.py @@ -56,7 +56,7 @@ class ActionExecutionOutputStreamController(ResourceController): def get_one(self, id, output_type='all', requester_user=None): # Special case for id == "last" if id == 'last': - execution_db = ActionExecution.query().order_by('-id').limit(1).first() + execution_db = ActionExecution.query(order_by=['-id'], limit=1, first=True) if not execution_db: raise ValueError('No executions found in the database') From a7582ae6aec26afa4dc5aeec6a54b8c22388d4f9 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 19 Dec 2019 18:17:50 +0100 Subject: [PATCH 6/6] Get rid if automatic auto conversion / de-referencing of embedded fields inside the base mongoengine document class. This process is very slow and adds a tons of overhead since it calls "to_python()" on every model field even if the field doesn't need to be referenced. With this approach we manually de-reference only the fields which need to be dereferenced / converted. The most ideal and faster approach would be to work directly with dictionaries as returned by pymongo (this way we could avoid the whole overhead), but this is a compromise. --- st2common/st2common/models/db/__init__.py | 10 +++++++--- st2common/st2common/models/db/action.py | 9 +++++++++ st2common/st2common/models/db/liveaction.py | 10 ++++++++++ st2common/st2common/models/db/rule.py | 12 ++++++++++++ .../st2common/models/db/rule_enforcement.py | 9 +++++++++ st2common/st2common/models/db/stormbase.py | 6 ++++++ st2common/st2common/models/db/trace.py | 16 ++++++++++++++++ st2common/st2common/models/db/trigger.py | 6 ++++++ 8 files changed, 75 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/models/db/__init__.py b/st2common/st2common/models/db/__init__.py index a44ac4220d..27704c00f2 100644 --- a/st2common/st2common/models/db/__init__.py +++ b/st2common/st2common/models/db/__init__.py @@ -487,7 +487,8 @@ def insert(self, instance): def add_or_update(self, instance, validate=True, undo_dict_field_escape=True): instance.save(validate=validate) if undo_dict_field_escape: - return self._undo_dict_field_escape(instance) + instance = self._undo_dict_field_escape(instance) + instance.id = str(instance.id) return instance def update(self, instance, **kwargs): @@ -609,8 +610,11 @@ def _process_as_pymongo_queryset(self, queryset, as_pymongo=False): for item in result: if '_id' in item: item['id'] = str(item.pop('_id')) - # TODO: Also avoid expensive auto conversion since it's not needed in most cases - model_db = self.model(__auto_convert=True, **item) + # NOTE: Disabling auto_convert speeds it up by 50% + # Derefernces only need to happen where we use EmbeddedDocumentField which is only in + # a few places + model_db = self.model(__auto_convert=False, **item) + model_db.id = str(model_db.id) models_result.append(model_db) return models_result diff --git a/st2common/st2common/models/db/action.py b/st2common/st2common/models/db/action.py index ebccf7ce67..0599ed1067 100644 --- a/st2common/st2common/models/db/action.py +++ b/st2common/st2common/models/db/action.py @@ -94,6 +94,15 @@ def __init__(self, *args, **values): self.ref = self.get_reference().ref self.uid = self.get_uid() + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + stormbase.TagsMixin.__init__(self) + + if self.notify: + self.notify = self._fields['notify'].to_python(self.notify) + def is_workflow(self): """ Return True if this action is a workflow, False otherwise. diff --git a/st2common/st2common/models/db/liveaction.py b/st2common/st2common/models/db/liveaction.py index 62135a3f33..af1e4126e6 100644 --- a/st2common/st2common/models/db/liveaction.py +++ b/st2common/st2common/models/db/liveaction.py @@ -87,6 +87,16 @@ class LiveActionDB(stormbase.StormFoundationDB): ] } + def __init__(self, *args, **kwargs): + super(LiveActionDB, self).__init__(*args, **kwargs) + + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + if self.notify: + self.notify = self._fields['notify'].to_python(self.notify) + def mask_secrets(self, value): from st2common.util import action_db diff --git a/st2common/st2common/models/db/rule.py b/st2common/st2common/models/db/rule.py index e56d66cea9..e8252c2cbc 100644 --- a/st2common/st2common/models/db/rule.py +++ b/st2common/st2common/models/db/rule.py @@ -159,6 +159,18 @@ def __init__(self, *args, **values): self.ref = self.get_reference().ref self.uid = self.get_uid() + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + stormbase.TagsMixin.__init__(self) + + if self.type: + self.type = self._fields['type'].to_python(self.type) + + if self.action: + self.action = self._fields['action'].to_python(self.action) + rule_access = MongoDBAccess(RuleDB) rule_type_access = MongoDBAccess(RuleTypeDB) diff --git a/st2common/st2common/models/db/rule_enforcement.py b/st2common/st2common/models/db/rule_enforcement.py index 6f6bc93a3f..d1faa30385 100644 --- a/st2common/st2common/models/db/rule_enforcement.py +++ b/st2common/st2common/models/db/rule_enforcement.py @@ -80,6 +80,15 @@ class RuleEnforcementDB(stormbase.StormFoundationDB, stormbase.TagsMixin): def __init__(self, *args, **values): super(RuleEnforcementDB, self).__init__(*args, **values) + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + stormbase.TagsMixin.__init__(self) + + if self.rule: + self.rule = self._fields['rule'].to_python(self.rule) + # Set status to succeeded for old / existing RuleEnforcementDB which predate status field status = getattr(self, 'status', None) failure_reason = getattr(self, 'failure_reason', None) diff --git a/st2common/st2common/models/db/stormbase.py b/st2common/st2common/models/db/stormbase.py index a9eb755867..08f6f425c0 100644 --- a/st2common/st2common/models/db/stormbase.py +++ b/st2common/st2common/models/db/stormbase.py @@ -165,6 +165,12 @@ class TagsMixin(object): """ tags = me.ListField(field=me.EmbeddedDocumentField(TagField)) + def __init__(self): + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True + if self.tags: + self.tags = self._fields['tags'].to_python(self.tags) + @classmethod def get_indexes(cls): return ['tags.name', 'tags.value'] diff --git a/st2common/st2common/models/db/trace.py b/st2common/st2common/models/db/trace.py index 059f234f57..88fe7e1135 100644 --- a/st2common/st2common/models/db/trace.py +++ b/st2common/st2common/models/db/trace.py @@ -94,6 +94,22 @@ def __init__(self, *args, **values): super(TraceDB, self).__init__(*args, **values) self.uid = self.get_uid() + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + if self.trigger_instances: + self.trigger_instances = \ + self._fields['trigger_instances'].to_python(self.trigger_instances) + + if self.rules: + self.rules = \ + self._fields['rules'].to_python(self.rules) + + if self.action_executions: + self.action_executions = \ + self._fields['action_executions'].to_python(self.action_executions) + def get_uid(self): parts = [] parts.append(self.RESOURCE_TYPE) diff --git a/st2common/st2common/models/db/trigger.py b/st2common/st2common/models/db/trigger.py index 9553c3db82..31750a5436 100644 --- a/st2common/st2common/models/db/trigger.py +++ b/st2common/st2common/models/db/trigger.py @@ -64,6 +64,12 @@ def __init__(self, *args, **values): # pylint: disable=no-member self.uid = self.get_uid() + # Manualy de-reference EmbeddedDocumentField fields to avoid overhead of de-referencing all + # the fields inside the base Document class constructor when __auto_convert is True. + # This approach means we need to update this code each time we add new + # EmbeddedDocumentField (which we should avoid anyway for performance reasons) + stormbase.TagsMixin.__init__(self) + class TriggerDB(stormbase.StormBaseDB, stormbase.ContentPackResourceMixin, stormbase.UIDFieldMixin):