Merge pull request #36227 from dimagi/revert-36091-jt/lookup-table-smart-sync

Revert "Lookup Table Limit Restore to Recently Modified"

Author: Jtang-1

Diff for: corehq/apps/fixtures/fixturegenerators.py

@@ -1,24 +1,20 @@
 from collections import defaultdict
+from functools import partial
 from operator import attrgetter
-from io import BytesIO
 from xml.etree import cElementTree as ElementTree
 
 from casexml.apps.phone.fixtures import FixtureProvider
 from casexml.apps.phone.utils import (
     GLOBAL_USER_ID,
-    record_datadog_metric,
-    get_cached_fixture_items,
-    cache_fixture_items_data,
-    combine_io_streams
+    get_or_cache_global_fixture,
 )
 from corehq.apps.fixtures.exceptions import FixtureTypeCheckError
-from corehq.apps.fixtures.models import fixture_bucket, LookupTable, LookupTableRow
+from corehq.apps.fixtures.models import FIXTURE_BUCKET, LookupTable, LookupTableRow
 from corehq.apps.products.fixtures import product_fixture_generator_json
 from corehq.apps.programs.fixtures import program_fixture_generator_json
 from corehq.util.metrics import metrics_histogram
 from corehq.util.xml_utils import serialize
 from .utils import clean_fixture_field_name, get_index_schema_node
-from dimagi.utils.couch import CriticalSection
 
 LOOKUP_TABLE_FIXTURE = 'lookup_table_fixture'
 REPORT_FIXTURE = 'report_fixture'
@@ -97,34 +93,31 @@ def item_lists_by_app(app, module):
 
 
 def get_global_items_by_domain(domain, case_id):
-    global_types = LookupTable.objects.by_domain(domain).filter(is_global=True)
-    return ItemListsProvider().get_global_items(domain, global_types, case_id, False)
+    global_types = {}
+    for data_type in LookupTable.objects.by_domain(domain):
+        if data_type.is_global:
+            global_types[data_type.id] = data_type
+    if global_types:
+        data_fn = partial(ItemListsProvider()._get_global_items, global_types, domain)
+        return get_or_cache_global_fixture(domain, case_id, FIXTURE_BUCKET, '', data_fn)
 
 
 class ItemListsProvider(FixtureProvider):
     id = 'item-list'
 
     def __call__(self, restore_state):
         restore_user = restore_state.restore_user
-        global_types = []
+        global_types = {}
         user_types = {}
-        should_full_sync = not restore_state.last_sync_log or not restore_state.last_sync_log.date
-        if should_full_sync:
-            data_types = LookupTable.objects.by_domain(restore_user.domain)
-        else:
-            data_types = LookupTable.objects.get_tables_modified_since(restore_user.domain,
-                                                                       restore_state.last_sync_log.date)
-        for data_type in data_types:
+        for data_type in LookupTable.objects.by_domain(restore_user.domain):
             if data_type.is_global:
-                global_types.append(data_type)
+                global_types[data_type.id] = data_type
             else:
                 user_types[data_type.id] = data_type
         items = []
         user_items_count = 0
         if global_types:
-            global_items = self.get_global_items(restore_state.restore_user.domain, global_types,
-                                                 restore_state.restore_user.user_id,
-                                                 restore_state.overwrite_cache)
+            global_items = self.get_global_items(global_types, restore_state)
             items.extend(global_items)
         if user_types:
             user_items, user_items_count = self.get_user_items_and_count(user_types, restore_user)
@@ -142,57 +135,21 @@ def __call__(self, restore_state):
         )
         return items
 
-    def get_global_items(self, domain, global_types, user_id, overwrite_cache):
-        """
-        :param user_id: User's id, if this is for case restore, then pass in case id
-        """
-        return combine_io_streams(
-            [self._get_or_cache_global_fixture(
-                domain,
-                global_type,
-                overwrite_cache,
-            ) for global_type in sorted(global_types, key=lambda global_type: global_type.tag)],
-            user_id
-        )
-
-    def _get_or_cache_global_fixture(self, domain, global_type, overwrite_cache=False):
-        """
-        Get the fixture data for a global fixture (one that does not vary by user).
-
-        :param domain: The domain to get or cache the fixture
-        :param user_id: User's id, if this is for case restore, then pass in case id
-        :param overwrite_cache: a boolean property from RestoreState object, default is False
-        :return: a byte string representation of the fixture
-        """
-        io_stream = None
-        key = fixture_bucket(global_type.id, domain)
-
-        if not overwrite_cache:
-            io_stream = get_cached_fixture_items(key)
-            record_datadog_metric('cache_miss' if io_stream is None else 'cache_hit', key)
-            if io_stream is not None:
-                io_stream = BytesIO(io_stream)
-
-        if io_stream is None:
-            with CriticalSection([key]):
-                if not overwrite_cache:
-                    # re-check cache to avoid re-computing it
-                    io_stream = get_cached_fixture_items(key)
-                    if io_stream is not None:
-                        io_stream = BytesIO(io_stream)
-                if io_stream is None:
-                    record_datadog_metric('generate', key)
-                    item = self._get_global_item(global_type, domain)
-                    io_stream = BytesIO()
-                    io_stream.write(ElementTree.tostring(item, encoding='utf-8'))
-                    io_stream.seek(0)
-                    cache_fixture_items_data(io_stream, domain, '', key)
-        return io_stream
-
-    def _get_global_item(self, global_type, domain):
+    def get_global_items(self, global_types, restore_state):
+        domain = restore_state.restore_user.domain
+        data_fn = partial(self._get_global_items, global_types, domain)
+        return get_or_cache_global_fixture(restore_state.restore_user.domain,
+                                           restore_state.restore_user.user_id,
+                                           FIXTURE_BUCKET,
+                                           '',
+                                           data_fn,
+                                           restore_state.overwrite_cache)
+
+    def _get_global_items(self, global_types, domain):
         def get_items_by_type(data_type):
             return LookupTableRow.objects.iter_rows(domain, table_id=data_type.id)
-        return self._get_fixtures({global_type.id: global_type}, get_items_by_type, GLOBAL_USER_ID)[0]
+
+        return self._get_fixtures(global_types, get_items_by_type, GLOBAL_USER_ID)
 
     def get_user_items_and_count(self, user_types, restore_user):
         user_items_count = 0

Diff for: corehq/apps/fixtures/models.py

@@ -12,11 +12,7 @@
 from corehq.util.jsonattrs import AttrsDict, AttrsList, list_of
 from .exceptions import FixtureVersionError
 
-FIXTURE_BUCKET_PREFIX = 'domain-fixtures'
-
-
-def fixture_bucket(lookup_table_id, domain):
-    return f'{FIXTURE_BUCKET_PREFIX}-{lookup_table_id}' + '/' + domain
+FIXTURE_BUCKET = 'domain-fixtures'
 
 
 class LookupTableManager(models.Manager):
@@ -31,9 +27,6 @@ def by_domain_tag(self, domain_name, tag):
     def domain_tag_exists(self, domain_name, tag):
         return self.filter(domain=domain_name, tag=tag).exists()
 
-    def get_tables_modified_since(self, domain, since_datetime):
-        return self.filter(domain=domain, last_modified__gt=since_datetime)
-
 
 @define
 class Alias:

Diff for: corehq/apps/fixtures/tests/test_fixture_data.py

@@ -1,32 +1,28 @@
 from xml.etree import cElementTree as ElementTree
-from datetime import datetime
 
 from django.test import TestCase
 
-from casexml.apps.phone.models import SimplifiedSyncLog
 from casexml.apps.case.tests.util import check_xml_line_by_line
 from casexml.apps.phone.tests.utils import \
     call_fixture_generator as call_fixture_generator_raw
 
 from corehq.apps.fixtures import fixturegenerators
 from corehq.apps.fixtures.models import (
-    fixture_bucket,
+    FIXTURE_BUCKET,
     Field,
     LookupTable,
     LookupTableRow,
     LookupTableRowOwner,
     OwnerType,
     TypeField,
 )
-from corehq.apps.fixtures.utils import clear_fixture_cache
 from corehq.apps.users.models import CommCareUser
 from corehq.blobs import get_blob_db
 
 
-def call_fixture_generator(user, last_sync=None):
+def call_fixture_generator(user):
     return [ElementTree.fromstring(f) if isinstance(f, bytes) else f
-            for f in call_fixture_generator_raw(fixturegenerators.item_lists, user,
-                                                last_sync=last_sync)]
+            for f in call_fixture_generator_raw(fixturegenerators.item_lists, user)]
 
 
 class FixtureDataTest(TestCase):
@@ -48,7 +44,6 @@ def setUp(self):
             item_attributes=[],
         )
         self.data_type.save()
-        self.addCleanup(get_blob_db().delete, key=fixture_bucket(self.data_type.id, self.domain))
 
         self.data_item = LookupTableRow(
             domain=self.domain,
@@ -80,14 +75,7 @@ def setUp(self):
             row_id=self.data_item.id,
         )
         self.ownership.save()
-
-        self.sync_log = SimplifiedSyncLog(
-            date=datetime.utcnow(),
-            domain=self.domain,
-            user_id=self.user.user_id,
-            build_id='some-build-id'
-        )
-        self.sync_log.save()
+        self.addCleanup(get_blob_db().delete, key=FIXTURE_BUCKET + '/' + self.domain)
 
     def test_xml(self):
         check_xml_line_by_line(self, """
@@ -258,49 +246,11 @@ def test_cached_global_fixture_user_id(self):
 
         fixtures = call_fixture_generator(frank)
         self.assertEqual({item.attrib['user_id'] for item in fixtures}, {frank.user_id})
-        self.assertTrue(get_blob_db().exists(key=fixture_bucket(sandwich.id, self.domain)))
+        self.assertTrue(get_blob_db().exists(key=FIXTURE_BUCKET + '/' + self.domain))
 
         fixtures = call_fixture_generator(sammy)
         self.assertEqual({item.attrib['user_id'] for item in fixtures}, {sammy.user_id})
 
-    def test_include_fixture_when_data_type_modified(self):
-        fixture = call_fixture_generator(self.user.to_ota_restore_user(self.domain), self.sync_log)
-        self.assertEqual(len(fixture), 0)
-
-        self.data_type.save()
-        fixture, = call_fixture_generator(self.user.to_ota_restore_user(self.domain), self.sync_log)
-        check_xml_line_by_line(self, """
-        <fixture id="item-list:district" user_id="%s">
-            <district_list>
-                <district>
-                    <state_name>Delhi_state</state_name>
-                    <district_name lang="hin">Delhi_in_HIN</district_name>
-                    <district_name lang="eng">Delhi_in_ENG</district_name>
-                    <district_id>Delhi_id</district_id>
-                </district>
-            </district_list>
-        </fixture>
-        """ % self.user.user_id, ElementTree.tostring(fixture, encoding='utf-8'))
-
-    def test_include_fixture_when_cache_is_cleared(self):
-        fixture = call_fixture_generator(self.user.to_ota_restore_user(self.domain), self.sync_log)
-        self.assertEqual(len(fixture), 0)
-
-        clear_fixture_cache(self.domain, [self.data_type.id])
-        fixture, = call_fixture_generator(self.user.to_ota_restore_user(self.domain), self.sync_log)
-        check_xml_line_by_line(self, """
-        <fixture id="item-list:district" user_id="%s">
-            <district_list>
-                <district>
-                    <state_name>Delhi_state</state_name>
-                    <district_name lang="hin">Delhi_in_HIN</district_name>
-                    <district_name lang="eng">Delhi_in_ENG</district_name>
-                    <district_id>Delhi_id</district_id>
-                </district>
-            </district_list>
-        </fixture>
-        """ % self.user.user_id, ElementTree.tostring(fixture, encoding='utf-8'))
-
     def make_data_type(self, name, is_global):
         data_type = LookupTable(
             domain=self.domain,
@@ -313,7 +263,6 @@ def make_data_type(self, name, is_global):
             item_attributes=[],
         )
         data_type.save()
-        self.addCleanup(get_blob_db().delete, key=fixture_bucket(data_type.id, self.domain))
         return data_type
 
     def make_data_item(self, data_type, cost):

Diff for: corehq/apps/fixtures/tests/test_upload.py

@@ -684,24 +684,6 @@ def error_tx():
             self.upload([(None, 'N', 'orange')])
         clear_cache.assert_called_once()
 
-    def test_row_added_clears_its_table_cache(self):
-        self.upload([(None, 'N', 'apple')])
-        table = self.get_table()
-        apple_id = self.get_rows(None)[0].id.hex
-
-        with patch.object(mod, "clear_fixture_cache") as mock_clear:
-            self.upload([(apple_id, 'N', 'apple'), (None, 'N', 'banana')])
-            self.assertEqual(mock_clear.call_args[0][1], {table.id})
-
-    def test_row_deleted_clears_its_table_cache(self):
-        self.upload([(None, 'N', 'apple')])
-        table = self.get_table()
-        apple_id = self.get_rows(None)[0].id.hex
-
-        with patch.object(mod, "clear_fixture_cache") as mock_clear:
-            self.upload([(apple_id, 'Y', 'apple')])
-            self.assertEqual(mock_clear.call_args[0][1], {table.id})
-
 
 class TestLookupTableOwnershipUpload(TestCase):
     do_upload = _run_upload
@@ -825,26 +807,6 @@ def test_non_string_owner_names(self):
         self.assertEqual(self.get_rows(), [('apple', {'3'}, {'3'}, {'3'})])
         self.assertFalse(result.errors)
 
-    def test_owner_added_clears_cache(self):
-        """Test that adding an owner to an existing row clears that table's cache"""
-        self.upload([(None, 'N', 'apple')])
-        table = LookupTable.objects.by_domain_tag(self.domain, 'things')
-        apple_id = self.get_rows(None)[0].id.hex
-
-        with patch.object(mod, "clear_fixture_cache") as mock_clear:
-            self.upload([(apple_id, 'N', 'apple', 'user1', 'G1', 'loc1')])
-            self.assertEqual(mock_clear.call_args[0][1], {table.id})
-
-    def test_owner_deleted_clears_cache(self):
-        """Test that removing an owner from an existing row clears that table's cache"""
-        self.upload([(None, 'N', 'apple', 'user1', 'G1', 'loc1')])
-        table = LookupTable.objects.by_domain_tag(self.domain, 'things')
-        apple_id = self.get_rows(None)[0].id.hex
-
-        with patch.object(mod, "clear_fixture_cache") as mock_clear:
-            self.upload([(apple_id, 'N', 'apple', None, None, None)])
-            self.assertEqual(mock_clear.call_args[0][1], {table.id})
-
     def upload(self, rows, *, check_result=True, **kw):
         data = self.make_rows(rows)
         workbook = TestFixtureUpload.get_workbook_from_data(self.headers, data)

Diff for: corehq/apps/fixtures/utils.py

@@ -59,8 +59,8 @@ def get_index_schema_node(fixture_id, attrs_to_index):
 
 
 def clear_fixture_cache(domain, data_type_ids):
+    from corehq.apps.fixtures.models import FIXTURE_BUCKET
     assert not isinstance(data_type_ids, (str, UUID)), f'expected list or set, got {type(data_type_ids).__name__}'
     LookupTable.objects.filter(id__in=data_type_ids).update(last_modified=datetime.utcnow())
-    from corehq.apps.fixtures.models import fixture_bucket
-    for data_type_id in data_type_ids:
-        get_blob_db().delete(key=fixture_bucket(data_type_id, domain))
+
+    get_blob_db().delete(key=FIXTURE_BUCKET + '/' + domain)

Diff for: corehq/blobs/models.py

@@ -67,7 +67,8 @@ class Meta:
         unique_together = [
             # HACK work around unique=True implies db_index=True
             # https://code.djangoproject.com/ticket/24082
-            # Avoid extra varchar_pattern_ops index
+            # Avoid extra varchar_pattern_ops index while still maintaining
+            # an index for exact matches. We don't need varchar_pattern_ops
             # since we do not do LIKE queries on these
             # https://stackoverflow.com/a/50926644/10840
             ("key",),

Diff for: corehq/ex-submodules/casexml/apps/phone/tests/test_ota_fixtures.py

@@ -3,7 +3,7 @@
 from casexml.apps.phone.utils import MockDevice
 from corehq.apps.domain.models import Domain
 from corehq.apps.fixtures.models import (
-    fixture_bucket,
+    FIXTURE_BUCKET,
     Field,
     LookupTable,
     LookupTableRow,
@@ -37,13 +37,11 @@ def setUpClass(cls):
         cls.group2 = Group(domain=DOMAIN, name='group2', case_sharing=True, users=[])
         cls.group2.save()
 
-        sa_data_type, _ = make_item_lists(SA_PROVINCES, 'western cape')
-        fr_data_type, _ = make_item_lists(FR_PROVINCES, 'burgundy', cls.group1)
-        ca_data_type, _ = make_item_lists(CA_PROVINCES, 'alberta', cls.group2)
+        make_item_lists(SA_PROVINCES, 'western cape'),
+        make_item_lists(FR_PROVINCES, 'burgundy', cls.group1),
+        make_item_lists(CA_PROVINCES, 'alberta', cls.group2),
 
-        cls.addClassCleanup(get_blob_db().delete, key=fixture_bucket(sa_data_type.id, DOMAIN))
-        cls.addClassCleanup(get_blob_db().delete, key=fixture_bucket(fr_data_type.id, DOMAIN))
-        cls.addClassCleanup(get_blob_db().delete, key=fixture_bucket(ca_data_type.id, DOMAIN))
+        cls.addClassCleanup(get_blob_db().delete, key=FIXTURE_BUCKET + "/" + DOMAIN)
 
         cls.restore_user = cls.user.to_ota_restore_user(DOMAIN)