diff --git a/analytics/tests/test_counts.py b/analytics/tests/test_counts.py index 34959b31c7..ae5e6219cc 100644 --- a/analytics/tests/test_counts.py +++ b/analytics/tests/test_counts.py @@ -74,6 +74,8 @@ from zerver.models import ( get_user, is_cross_realm_bot_email, ) +from zilencer.models import RemoteInstallationCount, RemoteZulipServer +from zilencer.views import get_last_id_from_server class AnalyticsTestCase(ZulipTestCase): @@ -1852,3 +1854,26 @@ class TestRealmActiveHumans(AnalyticsTestCase): 1, ) self.assertEqual(RealmCount.objects.filter(property="realm_active_humans::day").count(), 1) + + +class GetLastIdFromServerTest(ZulipTestCase): + def test_get_last_id_from_server_ignores_null(self) -> None: + """ + Verifies that get_last_id_from_server ignores null remote_ids, since this goes + against the default Postgres ordering behavior, which treats nulls as the largest value. + """ + self.server_uuid = "6cde5f7a-1f7e-4978-9716-49f69ebfc9fe" + self.server = RemoteZulipServer.objects.create( + uuid=self.server_uuid, + api_key="magic_secret_api_key", + hostname="demo.example.com", + last_updated=timezone_now(), + ) + first = RemoteInstallationCount.objects.create( + end_time=timezone_now(), server=self.server, property="test", value=1, remote_id=1 + ) + RemoteInstallationCount.objects.create( + end_time=timezone_now(), server=self.server, property="test2", value=1, remote_id=None + ) + result = get_last_id_from_server(self.server, RemoteInstallationCount) + self.assertEqual(result, first.remote_id) diff --git a/zilencer/migrations/0031_alter_remoteinstallationcount_remote_id_and_more.py b/zilencer/migrations/0031_alter_remoteinstallationcount_remote_id_and_more.py new file mode 100644 index 0000000000..9ebe6965b5 --- /dev/null +++ b/zilencer/migrations/0031_alter_remoteinstallationcount_remote_id_and_more.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.6 on 2023-10-23 20:22 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zilencer", "0030_alter_remoteinstallationcount_remote_id"), + ] + + operations = [ + migrations.AlterField( + model_name="remoteinstallationcount", + name="remote_id", + field=models.IntegerField(null=True), + ), + migrations.AlterField( + model_name="remoterealmcount", + name="remote_id", + field=models.IntegerField(null=True), + ), + ] diff --git a/zilencer/models.py b/zilencer/models.py index 9794674e0b..0e497df05c 100644 --- a/zilencer/models.py +++ b/zilencer/models.py @@ -135,7 +135,7 @@ class BaseRemoteCount(BaseCount): # The remote_id field is the id value of the corresponding *Count object # on the remote server. # It lets us deduplicate data from the remote server. - remote_id = models.IntegerField() + remote_id = models.IntegerField(null=True) class Meta: abstract = True diff --git a/zilencer/views.py b/zilencer/views.py index 4c4d644950..03842fc0e8 100644 --- a/zilencer/views.py +++ b/zilencer/views.py @@ -444,6 +444,10 @@ def validate_incoming_table_data( for row in rows: if is_count_stat and row["property"] not in COUNT_STATS: raise JsonableError(_("Invalid property {property}").format(property=row["property"])) + if row.get("id") is None: + # This shouldn't be possible, as submitting data like this should be + # prevented by our param validators. + raise AssertionError(f"Missing id field in row {row}") if row["id"] <= last_id: raise JsonableError(_("Data is out of order.")) last_id = row["id"] @@ -592,7 +596,15 @@ def remote_server_post_analytics( def get_last_id_from_server(server: RemoteZulipServer, model: Any) -> int: - last_count = model.objects.filter(server=server).order_by("remote_id").only("remote_id").last() + last_count = ( + model.objects.filter(server=server) + # Rows with remote_id=None are managed by the bouncer service itself, + # and thus aren't meant for syncing and should be ignored here. + .exclude(remote_id=None) + .order_by("remote_id") + .only("remote_id") + .last() + ) if last_count is not None: return last_count.remote_id return 0