Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create RepeaterStubs #29599

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions corehq/motech/repeaters/dbaccessors.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,17 @@ def _iter_repeat_records_by_repeater(domain, repeater_id, chunk_size,
yield doc['id']


def iter_repeaters():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does iter convey any useful information? Judging by how it's used, get_all_repeaters() makes its usage in create_repeaterstubs a bit easier for me to understand. It was not obvious to me that it was fetching all repeaters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developers have been frustrated in the past when they discover that get_foo() returns a generator, instead of a list or tuple that can be iterated more than once. So I use iter_foo() to make callers aware that they aren't getting a list.

Maybe a type hint would check both boxes?

def get_all_repeaters() -> Generator:
    ...

from .models import Repeater

for result in Repeater.get_db().view(
'repeaters/repeaters',
include_docs=True,
reduce=False,
).all():
yield Repeater.wrap(result['doc'])


def get_repeat_records_by_payload_id(domain, payload_id):
repeat_records = get_sql_repeat_records_by_payload_id(domain, payload_id)
if repeat_records:
Expand Down
27 changes: 27 additions & 0 deletions corehq/motech/repeaters/migration_functions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""
Functions used by RunPython operations in migrations.

.. note::
Placing them in a separate module makes it easier to import them in
unittests.

"""
from django.db import transaction, IntegrityError

from corehq.motech.repeaters.dbaccessors import iter_repeaters
from corehq.motech.repeaters.models import RepeaterStub


def create_repeaterstubs(apps, schema_editor):
for repeater in iter_repeaters():
with transaction.atomic():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a transaction? Unless something will be added later, this is only performing a single create, which is going to be auto-committed anyhow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thought given to doing this process via bulk_create? It should be substantially faster

try:
RepeaterStub.objects.create(
domain=repeater.domain,
repeater_id=repeater.get_id,
is_paused=repeater.paused,
)
except IntegrityError:
# It's faster to violate uniqueness constraint and ask
# forgiveness than to use `.get()` or `.exists()` first.
pass
Comment on lines +25 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't get_or_create be the canonical way to do this?

16 changes: 16 additions & 0 deletions corehq/motech/repeaters/migrations/0004_create_repeaterstubs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from django.db import migrations

from ..migration_functions import create_repeaterstubs


class Migration(migrations.Migration):

dependencies = [
('repeaters', '0003_migrate_connectionsettings'),
]

operations = [
migrations.RunPython(create_repeaterstubs,
reverse_code=migrations.RunPython.noop,
elidable=True),
]
16 changes: 16 additions & 0 deletions corehq/motech/repeaters/migrations/0005_repeater_uniq.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('repeaters', '0004_create_repeaterstubs'),
]

operations = [
migrations.AddConstraint(
model_name='repeaterstub',
constraint=models.UniqueConstraint(fields=('repeater_id',),
name='one_to_one_repeater'),
),
]
48 changes: 45 additions & 3 deletions corehq/motech/repeaters/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@
from django.utils.translation import ugettext_lazy as _

from couchdbkit.exceptions import ResourceConflict, ResourceNotFound
from memoized import memoized
from requests.exceptions import ConnectionError, Timeout, RequestException
from memoized import memoized, memoized_property
from requests.exceptions import ConnectionError, RequestException, Timeout

from casexml.apps.case.xml import LEGAL_VERSIONS, V2
from couchforms.const import DEVICE_LOG_XMLNS
Expand Down Expand Up @@ -120,6 +120,7 @@
from corehq.util.couch import stale_ok
from corehq.util.metrics import metrics_counter
from corehq.util.quickcache import quickcache
from corehq.util.urlsanitize.urlsanitize import PossibleSSRFAttempt

from .const import (
MAX_ATTEMPTS,
Expand Down Expand Up @@ -151,7 +152,6 @@
ShortFormRepeaterJsonPayloadGenerator,
UserPayloadGenerator,
)
from ...util.urlsanitize.urlsanitize import PossibleSSRFAttempt


def log_repeater_timeout_in_datadog(domain):
Expand All @@ -176,6 +176,12 @@ def log_repeater_success_in_datadog(domain, status_code, repeater_type):

class RepeaterStubManager(models.Manager):

def get_or_none(self, *args, **kwargs):
try:
return self.get(*args, **kwargs)
except self.model.DoesNotExist:
return None

def all_ready(self):
"""
Return all RepeaterStubs ready to be forwarded.
Expand Down Expand Up @@ -212,6 +218,10 @@ class Meta:
models.Index(fields=['domain']),
models.Index(fields=['repeater_id']),
]
constraints = [
models.UniqueConstraint(fields=['repeater_id'],
name='one_to_one_repeater')
]
Comment on lines +221 to +224
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isn't this done using unique?


@property
@memoized
Expand Down Expand Up @@ -299,6 +309,28 @@ def _get_connection_settings(self):
def name(self):
return self.connection_settings.name

def save(self, **kwargs):
is_new = not self._id
super().save(**kwargs)
if is_new:
RepeaterStub.objects.create(
domain=self.domain,
repeater_id=self._id,
is_paused=self.paused,
)

def delete(self):
if self.repeater_stub.id:
self.repeater_stub.delete()
super().delete()
Comment on lines +322 to +325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are two separate databases, it seems possible that deleting the stub could succeed, while deleting from the SQL database could fail. Is that a situation that needs to be avoided? If so, it seems like it would be slightly safer to wrap this in a transaction so we can rollback the stub's deletion in the event that the couch deletion fails.


@memoized_property
def repeater_stub(self):
return RepeaterStub.objects.get(
domain=self.domain,
repeater_id=self._id,
)

@classmethod
def available_for_domain(cls, domain):
"""Returns whether this repeater can be used by a particular domain
Expand Down Expand Up @@ -448,14 +480,24 @@ def retire(self):
self['base_doc'] += DELETED_SUFFIX
self.paused = False
self.save()
# Deleting RepeaterStub will cascade-delete SQLRepeatRecords
# and SQLRepeatRecordAttempts too. (RequestLog will still keep a
# record of all send attempts.)
self.repeater_stub.delete()
# NOTE: Undeleting a Repeater needs to include creating a
# RepeaterStub for it.
Comment on lines +483 to +488
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this, pause, and resume, are we concerned about what happens if couch or SQL are not both simultaneously available? Again, using SQL transactions might be useful to mitigate that here. It might also be useful to have at least one test that illustrates how we handle when one of the systems is not available


def pause(self):
self.paused = True
self.save()
self.repeater_stub.is_paused = True
self.repeater_stub.save()

def resume(self):
self.paused = False
self.save()
self.repeater_stub.is_paused = False
self.repeater_stub.save()

def get_url(self, repeat_record):
# to be overridden
Expand Down
127 changes: 122 additions & 5 deletions corehq/motech/repeaters/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from uuid import uuid4

from django.conf import settings
from django.db import IntegrityError, transaction
from django.test import SimpleTestCase, TestCase
from django.utils import timezone

Expand All @@ -21,6 +22,7 @@
RECORD_PENDING_STATE,
RECORD_SUCCESS_STATE,
)
from ..migration_functions import create_repeaterstubs
from ..models import (
FormRepeater,
RepeaterStub,
Expand Down Expand Up @@ -49,13 +51,9 @@ def setUp(self):
url='https://www.example.com/api/',
)
self.repeater.save()
self.repeater_stub = RepeaterStub.objects.create(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
)
self.repeater_stub = self.repeater.repeater_stub

def tearDown(self):
self.repeater_stub.delete()
self.repeater.delete()
super().tearDown()

Expand Down Expand Up @@ -402,3 +400,122 @@ def test_yes(self):
with make_repeat_record(self.repeater_stub, RECORD_PENDING_STATE):
is_migrated = are_repeat_records_migrated(DOMAIN)
self.assertTrue(is_migrated)


class RepeaterStubOneToOneRepeaterTests(RepeaterFixtureMixin, TestCase):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F821 undefined name 'RepeaterFixtureMixin'


def test_one_in_domain(self):
self.assertEqual(RepeaterStub.objects.filter(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
Comment on lines +405 to +410
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are difficult to follow. They are deceptively short, because all of the work is done in the setup methods. Ideally, tests should follow the AAA pattern (Arrange, Act, Assert). I want to be able to see what is being constructed, what you are doing on those constructed objects, and what ultimately is the expected result we're asserting against. In this case, all of our arrange blocks are hidden behind multiple layers -- they don't exist in the test body itself, they don't exist in the class, so they're rather cryptic to find in RepeaterFixtureMixin. Does this behavior warrant 4 tests? It seems like we're just testing Django's models.UniqueConstraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Test mix-ins are awful, and I'm not sure how practical these tests are. I'll rework this.

).count(), 1)

def test_one_globally(self):
self.assertEqual(RepeaterStub.objects.filter(
repeater_id=self.repeater.get_id,
).count(), 1)

def test_only_one_in_domain(self):
with transaction.atomic():
# (Run in its own transaction so as not to mess with tearDown)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error was this giving you? Failing to create a duplicate object should not affect tearDown

with self.assertRaises(IntegrityError):
RepeaterStub.objects.create(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
)

def test_only_one_globally(self):
with transaction.atomic():
with self.assertRaises(IntegrityError):
RepeaterStub.objects.create(
domain='another-domain',
repeater_id=self.repeater.get_id,
)


class SaveDeleteRepeaterTests(TestCase):

def setUp(self):
self.repeater = FormRepeater(
domain=DOMAIN,
url='https://www.example.com/api/',
)
Comment on lines +439 to +442
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FormRepeater is still a couch model, right? Do our tests get stuck if this repeater already exists?


def tearDown(self):
try:
self.repeater.delete()
except TypeError:
# test_delete() causes tearDown() to throw TypeError because
# self.repeater is already deleted.
pass

def test_save_new(self):
with self.assertNumQueries(1):
self.repeater.save()
self.assertEqual(RepeaterStub.objects.filter(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
).count(), 1)
Comment on lines +452 to +458
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have two tests here? One is testing that only one database query is made -- is that an important part of save's contract that needs to be enforced here? The other is that saving a repeater creates a repeater stub. The test might be more readable if that was made explicit, i.e. test_saving_creates_stub


def test_save_old(self):
self.repeater.save()
self.repeater.paused = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line have any effect for this test?

with self.assertNumQueries(0):
self.repeater.save()
self.assertEqual(RepeaterStub.objects.filter(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
).count(), 1)

def test_delete(self):
self.repeater.save()
with self.assertNumQueries(3):
# 1. SELECT ... FROM "repeaters_repeaterstub" ...
# 2. SELECT ... FROM "repeaters_repeatrecord" ...
# 3. DELETE FROM "repeaters_repeaterstub" ...
self.repeater.delete()
Comment on lines +472 to +476
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a better example for assertNumQueries being brittle -- why is it important that only 3 queries are executed here?

self.assertEqual(RepeaterStub.objects.filter(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
).count(), 0)


class TestMigrationCantDuplicate(RepeaterFixtureMixin, TestCase):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F821 undefined name 'RepeaterFixtureMixin'


def test_migration_cant_duplicate(self):
self.assertEqual(RepeaterStub.objects.filter(
repeater_id=self.repeater.get_id,
).count(), 1)
create_repeaterstubs(None, None)
self.assertEqual(RepeaterStub.objects.filter(
repeater_id=self.repeater.get_id,
).count(), 1)
Comment on lines +486 to +492
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might be more readable if the objects were created/attempted to be created within this body. Specifically, creating an assertion prior to executing the system under test is confusing. Isn't this test essentially just testing the uniqueness constraint again? A different test might be to ensure that, in the presence of an existing stub, that create_repeaterstubs doesn't throw an exception. But I'd like to see that kind of test in test_migration_functions.py so that it's clear its migration-specific, and that it is simple to remove once the migration is complete



class PauseResumeRetireRepeaterTests(RepeaterFixtureMixin, TestCase):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F821 undefined name 'RepeaterFixtureMixin'


def _get_repeater_stub(self):
return RepeaterStub.objects.get(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
)

def test_pause(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this could be named better. test_pausing_repeater_pauses_stub conveys what the test is doing more clearly.

self.assertFalse(self._get_repeater_stub().is_paused)
self.repeater.pause()
self.assertTrue(self._get_repeater_stub().is_paused)

def test_resume(self):
repeater_stub = self._get_repeater_stub()
repeater_stub.is_paused = True
repeater_stub.save()

self.repeater.resume()
self.assertFalse(self._get_repeater_stub().is_paused)

def test_retire(self):
self.repeater.retire()
self.assertFalse(RepeaterStub.objects.filter(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
).exists())
15 changes: 7 additions & 8 deletions corehq/motech/repeaters/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,14 @@ def setUpClass(cls):
name='Test API',
url="http://localhost/api/"
)
cls.repeater = FormRepeater(
domain=DOMAIN,
connection_settings_id=cls.connection_settings.id,
)
cls.repeater.save()

def setUp(self):
self.repeater_stub = RepeaterStub.objects.create(
self.repeater = FormRepeater(
domain=DOMAIN,
connection_settings_id=self.connection_settings.id,
)
self.repeater.save()
self.repeater_stub = RepeaterStub.objects.get(
domain=DOMAIN,
repeater_id=self.repeater.get_id,
)
Expand All @@ -104,11 +104,10 @@ def setUp(self):
just_now += timedelta(seconds=1)

def tearDown(self):
self.repeater_stub.delete()
self.repeater.delete()

@classmethod
def tearDownClass(cls):
cls.repeater.delete()
cls.connection_settings.delete()
cls.domain.delete()
super().tearDownClass()
Expand Down
Loading