Code duplication. db_base_plugin_v2.py and loadbalancer_db.py duplicate code

[Avishay Balderman]

quantum/db/db_base_plugin_v2.py and quantum/db/loadbalancer/loadbalancer_db.py duplicate code.
Here is the duplicated code:

  def _fields(self, resource, fields):
        if fields:
            return dict((key, item) for key, item in resource.iteritems()
                        if key in fields)
        return resource

    def _apply_filters_to_query(self, query, model, filters):
        if filters:
            for key, value in filters.iteritems():
                column = getattr(model, key, None)
                if column:
                    query = query.filter(column.in_(value))
        return query

    def _get_collection_query(self, context, model, filters=None):
        collection = self._model_query(context, model)
        collection = self._apply_filters_to_query(collection, model, filters)
        return collection

    def _get_collection(self, context, model, dict_func, filters=None,
                        fields=None):
        query = self._get_collection_query(context, model, filters)
        return [dict_func(c, fields) for c in query.all()]

    def _get_collection_count(self, context, model, filters=None):
        return self._get_collection_query(context, model, filters).count()

Needs to have common parent to support this code.

[Avishay Balderman (avishayb)]

Question: Will a 'mixin' class work for us here?

[Mark McClain (markmcclain)]

I think common base mixin could work. The classes also share _get_by_id() and _model_query() could be altered slightly to work in both cases.

[Avishay Balderman (avishayb)]

Question:
As far as I understand the main difference between the 2 classes regards the method '_model_query'
is '_model_query_hooks' and the method 'register_model_query_hook' inside quantum/db/db_base_plugin_v2.py that uses it.
So I think '_model_query_hooks' plus 'register_model_query_hook' should be pulled up to the mixin class.
Makes sense?

[Avishay Balderman (avishayb)]

Here is a first proposal for the mixin class. Most of the code is just Copy & Paste of the duplicated code. The only method that was modified is '_model_query' . I have added a check there for the existence of the attribute '_model_query_hooks'

class CommonDBUtils:
    """ Common DB Utils """
    # can be classmethod - no use of 'self'
    def _fields(self, resource, fields):
        if fields:
            return dict((key, item) for key, item in resource.iteritems()
                        if key in fields)
        return resource
    # can be classmethod - no use of 'self'
    def _apply_filters_to_query(self, query, model, filters):
        if filters:
            for key, value in filters.iteritems():
                column = getattr(model, key, None)
                if column:
                    query = query.filter(column.in_(value))
        return query

    def _get_collection_query(self, context, model, filters=None):
        collection = self._model_query(context, model)
        collection = self._apply_filters_to_query(collection, model, filters)
        return collection

    def _get_collection(self, context, model, dict_func, filters=None,
                        fields=None):
        query = self._get_collection_query(context, model, filters)
        return [dict_func(c, fields) for c in query.all()]

    def _get_collection_count(self, context, model, filters=None):
        return self._get_collection_query(context, model, filters).count()

    def _get_by_id(self, context, model, id):
        query = self._model_query(context, model)
        return query.filter(model.id == id).one()

    def _model_query(self, context, model):
        query = context.session.query(model)
        # define basic filter condition for model query
        # NOTE(jkoelker) non-admin queries are scoped to their tenant_id
        # NOTE(salvatore-orlando): unless the model allows for shared objects
        query_filter = None
        if not context.is_admin and hasattr(model, 'tenant_id'):
            if hasattr(model, 'shared'):
                query_filter = ((model.tenant_id == context.tenant_id) |
                                (model.shared))
            else:
                query_filter = (model.tenant_id == context.tenant_id)
        # Execute query hooks registered from mixins and plugins
        if hasattr(self, '_model_query_hooks'):
            for _name, hooks in self._model_query_hooks.get(model,
                                                            {}).iteritems():
                query_hook = hooks.get('query')
                filter_hook = hooks.get('filter')
                if query_hook:
                    query = query_hook(self, context, model, query)
                if filter_hook:
                    query_filter = filter_hook(self, context, model, query_filter)

        if query_filter is not None:
            query = query.filter(query_filter)
        return query

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/30757

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/30757
Committed: http://github.com/openstack/quantum/commit/e2e2c62133a0c56a8e5929aa56113451adcb17b5
Submitter: Jenkins
Branch: master

commit e2e2c62133a0c56a8e5929aa56113451adcb17b5
Author: Eugene Nikanorov <email address hidden>
Date: Tue May 28 11:45:25 2013 +0400

    Refactor db_base_plugin_v2 and to remove code duplication

    fixes bug 1104379

    Introduce CommonDbMixin which includes utility methods
    to manipulate model queries.

    Change-Id: Ib3602321328cbf945358e0581ecc649e3f69d196