refactor: TT-245 refactor on find all function in cosmos db repository class (#312)

* refactor: TT-245 Refactor function find all inside cosmosdb Repository

* refactor: TT-245 remove unnecesary functions and add some testing

Author: jcalarcon98

commons/data_access_layer/cosmos_db.py

@@ -1,15 +1,15 @@
 import dataclasses
 import logging
-from typing import Callable, List
+from typing import Callable
 
 import azure.cosmos.cosmos_client as cosmos_client
 import azure.cosmos.exceptions as exceptions
-import flask
 from azure.cosmos import ContainerProxy, PartitionKey
 from flask import Flask
 from werkzeug.exceptions import HTTPException
 
 from commons.data_access_layer.database import CRUDDao, EventContext
+from utils.query_builder import CosmosDBQueryBuilder
 
 
 class CosmosDBFacade:
@@ -124,55 +124,6 @@ def from_definition(
             custom_cosmos_helper=custom_cosmos_helper,
         )
 
-    @staticmethod
-    def create_sql_condition_for_visibility(
-        visible_only: bool, container_name='c'
-    ) -> str:
-        if visible_only:
-            # We are considering that `deleted == null` is not a choice
-            return 'AND NOT IS_DEFINED(%s.deleted)' % container_name
-        return ''
-
-    @staticmethod
-    def create_sql_active_condition(
-        status_value: str, container_name='c'
-    ) -> str:
-        if status_value != None:
-            not_defined_condition = ''
-            condition_operand = ' AND '
-            if status_value == 'active':
-                not_defined_condition = (
-                    'AND NOT IS_DEFINED({container_name}.status)'.format(
-                        container_name=container_name
-                    )
-                )
-                condition_operand = ' OR '
-
-            defined_condition = '(IS_DEFINED({container_name}.status) \
-                AND {container_name}.status = \'{status_value}\')'.format(
-                container_name=container_name, status_value=status_value
-            )
-            return (
-                not_defined_condition + condition_operand + defined_condition
-            )
-
-        return ''
-
-    @staticmethod
-    def create_sql_where_conditions(
-        conditions: dict, container_name='c'
-    ) -> str:
-        where_conditions = []
-        for k in conditions.keys():
-            where_conditions.append(f'{container_name}.{k} = @{k}')
-
-        if len(where_conditions) > 0:
-            return "AND {where_conditions_clause}".format(
-                where_conditions_clause=" AND ".join(where_conditions)
-            )
-        else:
-            return ""
-
     @staticmethod
     def generate_params(conditions: dict) -> list:
         result = []
@@ -206,16 +157,6 @@ def attach_context(data: dict, event_context: EventContext):
             "session_id": event_context.session_id,
         }
 
-    @staticmethod
-    def create_sql_date_range_filter(date_range: dict) -> str:
-        if 'start_date' in date_range and 'end_date' in date_range:
-            return """
-                AND ((c.start_date BETWEEN @start_date AND @end_date) OR
-                 (c.end_date BETWEEN @start_date AND @end_date))
-                """
-        else:
-            return ''
-
     def create(
         self, data: dict, event_context: EventContext, mapper: Callable = None
     ):
@@ -257,53 +198,38 @@ def find_all(
         mapper: Callable = None,
     ):
         conditions = conditions if conditions else {}
-        partition_key_value = self.find_partition_key_value(event_context)
-        max_count = self.get_page_size_or(max_count)
-        params = [
-            {"name": "@partition_key_value", "value": partition_key_value},
-            {"name": "@offset", "value": offset},
-            {"name": "@max_count", "value": max_count},
-        ]
-
-        status_value = None
-        if conditions.get('status') != None:
-            status_value = conditions.get('status')
+        max_count: int = self.get_page_size_or(max_count)
+
+        status_value = conditions.get('status')
+        if status_value:
             conditions.pop('status')
 
         date_range = date_range if date_range else {}
-        date_range_params = (
-            self.generate_params(date_range) if date_range else []
-        )
-        params.extend(self.generate_params(conditions))
-        params.extend(date_range_params)
-
-        query_str = """
-            SELECT * FROM c
-            WHERE c.{partition_key_attribute}=@partition_key_value
-            {conditions_clause}
-            {active_condition}
-            {date_range_sql_condition}
-            {visibility_condition}
-            {order_clause}
-            OFFSET @offset LIMIT @max_count
-            """.format(
-            partition_key_attribute=self.partition_key_attribute,
-            visibility_condition=self.create_sql_condition_for_visibility(
-                visible_only
-            ),
-            active_condition=self.create_sql_active_condition(status_value),
-            conditions_clause=self.create_sql_where_conditions(conditions),
-            date_range_sql_condition=self.create_sql_date_range_filter(
-                date_range
-            ),
-            order_clause=self.create_sql_order_clause(),
+
+        query_builder = (
+            CosmosDBQueryBuilder()
+            .add_sql_where_equal_condition(conditions)
+            .add_sql_active_condition(status_value)
+            .add_sql_date_range_condition(date_range)
+            .add_sql_visibility_condition(visible_only)
+            .add_sql_limit_condition(max_count)
+            .add_sql_offset_condition(offset)
+            .build()
         )
 
+        if len(self.order_fields) > 1:
+            attribute = self.order_fields[0]
+            order = self.order_fields[1]
+            query_builder.add_sql_order_by_condition(attribute, order)
+
+        query_str = query_builder.get_query()
+        params = query_builder.get_parameters()
+        partition_key_value = self.find_partition_key_value(event_context)
+
         result = self.container.query_items(
             query=query_str,
             parameters=params,
             partition_key=partition_key_value,
-            max_item_count=max_count,
         )
 
         function_mapper = self.get_mapper_or_dict(mapper)

tests/commons/data_access_layer/cosmos_db_test.py

@@ -660,28 +660,6 @@ def test_delete_permanently_with_valid_id_should_succeed(
         assert e.status_code == 404
 
 
-def test_repository_create_sql_where_conditions_with_multiple_values(
-    cosmos_db_repository: CosmosDBRepository,
-):
-    result = cosmos_db_repository.create_sql_where_conditions(
-        {'owner_id': 'mark', 'customer_id': 'me'}, "c"
-    )
-
-    assert result is not None
-    assert (
-        result == "AND c.owner_id = @owner_id AND c.customer_id = @customer_id"
-    )
-
-
-def test_repository_create_sql_where_conditions_with_no_values(
-    cosmos_db_repository: CosmosDBRepository,
-):
-    result = cosmos_db_repository.create_sql_where_conditions({}, "c")
-
-    assert result is not None
-    assert result == ""
-
-
 def test_repository_append_conditions_values(
     cosmos_db_repository: CosmosDBRepository,
 ):

tests/time_tracker_api/activities/activities_model_test.py

@@ -1,5 +1,4 @@
 from unittest.mock import Mock, patch
-import pytest
 
 from commons.data_access_layer.database import EventContext
 from time_tracker_api.activities.activities_model import (
@@ -8,9 +7,6 @@
 )
 
 
-@patch(
-    'time_tracker_api.activities.activities_model.ActivityCosmosDBRepository.create_sql_condition_for_visibility'
-)
 @patch(
     'time_tracker_api.activities.activities_model.ActivityCosmosDBRepository.find_partition_key_value'
 )

tests/time_tracker_api/time_entries/time_entries_namespace_test.py

@@ -8,12 +8,8 @@
 from pytest_mock import MockFixture, pytest
 
 from utils.time import (
-    get_current_year,
-    get_current_month,
     current_datetime,
     current_datetime_str,
-    get_date_range_of_month,
-    datetime_str,
 )
 from utils import worked_time
 from time_tracker_api.time_entries.time_entries_model import (
@@ -204,10 +200,6 @@ def test_get_time_entry_should_succeed_with_valid_id(
     'time_tracker_api.time_entries.time_entries_dao.TimeEntriesCosmosDBDao.handle_date_filter_args',
     Mock(),
 )
-@patch(
-    'time_tracker_api.time_entries.time_entries_repository.TimeEntryCosmosDBRepository.create_sql_date_range_filter',
-    Mock(),
-)
 @patch(
     'commons.data_access_layer.cosmos_db.CosmosDBRepository.generate_params',
     Mock(),
@@ -232,7 +224,6 @@ def test_get_time_entries_by_type_of_user_when_is_user_tester(
     expected_user_ids,
 ):
     test_user_id = "id1"
-    non_test_user_id = "id2"
     te1 = TimeEntryCosmosDBModel(
         {
             "id": '1',
@@ -285,10 +276,6 @@ def test_get_time_entries_by_type_of_user_when_is_user_tester(
     'time_tracker_api.time_entries.time_entries_dao.TimeEntriesCosmosDBDao.handle_date_filter_args',
     Mock(),
 )
-@patch(
-    'time_tracker_api.time_entries.time_entries_repository.TimeEntryCosmosDBRepository.create_sql_date_range_filter',
-    Mock(),
-)
 @patch(
     'commons.data_access_layer.cosmos_db.CosmosDBRepository.generate_params',
     Mock(),
@@ -313,7 +300,6 @@ def test_get_time_entries_by_type_of_user_when_is_not_user_tester(
     expected_user_ids,
 ):
     test_user_id = "id1"
-    non_test_user_id = "id2"
     te1 = TimeEntryCosmosDBModel(
         {
             "id": '1',
@@ -386,7 +372,6 @@ def test_get_time_entry_should_succeed_with_valid_id(
 )
 def test_get_time_entry_raise_http_exception(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     http_exception: HTTPException,
@@ -407,7 +392,6 @@ def test_get_time_entry_raise_http_exception(
 
 def test_update_time_entry_calls_partial_update_with_incoming_payload(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     owner_id: str,
@@ -465,7 +449,6 @@ def test_update_time_entry_should_reject_bad_request(
 
 def test_update_time_entry_raise_not_found(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     owner_id: str,
@@ -499,7 +482,6 @@ def test_update_time_entry_raise_not_found(
 
 def test_delete_time_entry_calls_delete(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     time_entries_dao,
@@ -529,7 +511,6 @@ def test_delete_time_entry_calls_delete(
 )
 def test_delete_time_entry_raise_http_exception(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     http_exception: HTTPException,
@@ -554,7 +535,6 @@ def test_delete_time_entry_raise_http_exception(
 
 def test_stop_time_entry_calls_partial_update(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     time_entries_dao,
@@ -581,7 +561,6 @@ def test_stop_time_entry_calls_partial_update(
 
 def test_stop_time_entry_raise_unprocessable_entity(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     time_entries_dao,
@@ -611,7 +590,6 @@ def test_stop_time_entry_raise_unprocessable_entity(
 
 def test_restart_time_entry_calls_partial_update(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     time_entries_dao,
@@ -638,7 +616,6 @@ def test_restart_time_entry_calls_partial_update(
 
 def test_restart_time_entry_raise_unprocessable_entity(
     client: FlaskClient,
-    mocker: MockFixture,
     valid_header: dict,
     valid_id: str,
     time_entries_dao,

tests/time_tracker_api/time_entries/time_entries_query_builder_test.py

@@ -6,7 +6,7 @@
 from utils.repository import remove_white_spaces
 
 
-def test_TimeEntryQueryBuilder_is_subclass_CosmosDBQueryBuilder():
+def test_time_entry_query_builder_should_be_subclass_of_cosmos_query_builder():
     query_builder = CosmosDBQueryBuilder()
     time_entries_query_builder = TimeEntryQueryBuilder()
 
@@ -15,50 +15,6 @@ def test_TimeEntryQueryBuilder_is_subclass_CosmosDBQueryBuilder():
     )
 
 
-def test_add_sql_date_range_condition_should_update_where_list():
-    start_date = "2021-03-19T05:07:00.000Z"
-    end_date = "2021-03-25T10:00:00.000Z"
-    time_entry_query_builder = (
-        TimeEntryQueryBuilder().add_sql_date_range_condition(
-            {
-                "start_date": start_date,
-                "end_date": end_date,
-            }
-        )
-    )
-    expected_params = [
-        {"name": "@start_date", "value": start_date},
-        {"name": "@end_date", "value": end_date},
-    ]
-    assert len(time_entry_query_builder.where_conditions) == 1
-    assert len(time_entry_query_builder.parameters) == len(expected_params)
-    assert time_entry_query_builder.get_parameters() == expected_params
-
-
-def test_build_with_add_sql_date_range_condition():
-    time_entry_query_builder = (
-        TimeEntryQueryBuilder()
-        .add_sql_date_range_condition(
-            {
-                "start_date": "2021-04-19T05:00:00.000Z",
-                "end_date": "2021-04-20T10:00:00.000Z",
-            }
-        )
-        .build()
-    )
-
-    expected_query = """
-                    SELECT * FROM c
-                    WHERE ((c.start_date BETWEEN @start_date AND @end_date) OR
-                    (c.end_date BETWEEN @start_date AND @end_date))
-                    """
-    query = time_entry_query_builder.get_query()
-
-    assert remove_white_spaces(query) == remove_white_spaces(expected_query)
-    assert len(time_entry_query_builder.where_conditions) == 1
-    assert len(time_entry_query_builder.get_parameters()) == 2
-
-
 def test_add_sql_interception_with_date_range_condition():
     start_date = "2021-01-19T05:07:00.000Z"
     end_date = "2021-01-25T10:00:00.000Z"