feat: Add explicit ordering for segment rule conditions (#5671)

Co-authored-by: Evandro Myller <22429+emyller@users.noreply.github.com>

Author: khvn26

api/app/settings/common.py

@@ -1357,6 +1357,10 @@
 
 SEGMENT_RULES_CONDITIONS_LIMIT = env.int("SEGMENT_RULES_CONDITIONS_LIMIT", 100)
 
+SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED = env.bool(
+    "SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED", default=False
+)
+
 WEBHOOK_BACKOFF_BASE = env.int("WEBHOOK_BACKOFF_BASE", default=2)
 WEBHOOK_BACKOFF_RETRIES = env.int("WEBHOOK_BACKOFF_RETRIES", default=3)
 

api/core/models.py

@@ -66,7 +66,9 @@ class SoftDeleteExportableManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager)
 
 
 class SoftDeleteExportableModel(SoftDeleteObject, AbstractBaseExportableModel):  # type: ignore[misc]
-    objects = SoftDeleteExportableManager()  # type: ignore[misc]
+    objects: typing.ClassVar[SoftDeleteExportableManager] = (
+        SoftDeleteExportableManager()
+    )
 
     class Meta:
         abstract = True

api/segments/models.py

@@ -22,6 +22,7 @@
 )
 from audit.related_object_type import RelatedObjectType
 from core.models import (
+    SoftDeleteExportableManager,
     SoftDeleteExportableModel,
     abstract_base_auditable_model_factory,
 )
@@ -300,6 +301,21 @@ def deep_clone(self, cloned_segment: Segment) -> "SegmentRule":
         return cloned_rule
 
 
+class ConditionManager(SoftDeleteExportableManager):
+    def get_queryset(
+        self,
+    ) -> models.QuerySet["Condition"]:
+        # Effectively `Condition.Meta.ordering = ("id",) if ... else ()`,
+        # but avoid the weirdness of a setting-dependant migration
+        # and having to reload everything in tests
+        qs: models.QuerySet["Condition"]
+        if settings.SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED:
+            qs = super().get_queryset().order_by("id")
+        else:
+            qs = super().get_queryset()
+        return qs
+
+
 class Condition(
     SoftDeleteExportableModel,
     abstract_base_auditable_model_factory(["uuid"]),  # type: ignore[misc]
@@ -343,6 +359,8 @@ class Condition(
     created_at = models.DateTimeField(null=True, auto_now_add=True)
     updated_at = models.DateTimeField(null=True, auto_now=True)
 
+    objects: typing.ClassVar[ConditionManager] = ConditionManager()
+
     def __str__(self):  # type: ignore[no-untyped-def]
         return "Condition for %s: %s %s %s" % (
             str(self.rule),

api/tests/unit/segments/test_unit_segments_views.py

@@ -648,8 +648,11 @@ def test_update_segment_add_new_condition(
     admin_client_new: APIClient,
     segment: Segment,
     segment_rule: SegmentRule,
+    settings: SettingsWrapper,
 ) -> None:
     # Given
+    settings.SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED = True
+
     url = reverse(
         "api-v1:projects:project-segments-detail", args=[project.id, segment.id]
     )
@@ -705,11 +708,8 @@ def test_update_segment_add_new_condition(
     assert response.status_code == status.HTTP_200_OK
 
     assert nested_rule.conditions.count() == 2
-    assert (
-        nested_rule.conditions.order_by("-id").first().property
-        == new_condition_property
-    )
-    assert nested_rule.conditions.order_by("-id").first().value == new_condition_value
+    assert nested_rule.conditions.last().property == new_condition_property
+    assert nested_rule.conditions.last().value == new_condition_value
 
 
 def test_update_mismatched_rule_and_segment(

docs/docs/deployment/hosting/locally-api.md

@@ -222,6 +222,7 @@ the below variables will be ignored.
   and hence should not be modified for already running instances of flagsmith. It should only be used for new
   installations, and should not be modified. WARNING: setting this to a higher limit may prevent imports to our SaaS
   platform if required in the future.
+- `SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED`: Forces segment rule condition ordering by primary key, ascending. Default is `False` (no guaranteed order). **Warning**: changing this setting might affect evaluation for existing segments.
 - `ENABLE_API_USAGE_TRACKING`: Enable tracking of all API requests in Postgres / Influx. Default is True. Setting to
   False will mean that the Usage tab in the Organisation Settings will not show any data. Useful when using Postgres for
   analytics in high traffic environments to limit the size of database.
@@ -451,7 +452,7 @@ increasing the performance of your task processor.
 
 
 | Environment Variable                  | Description                                                                                                                                                                                       | Example value                                          | Default                                       |
-|---------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------|-----------------------------------------------|
+| ------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------ | --------------------------------------------- |
 | `CACHE_ENVIRONMENT_DOCUMENT_MODE`     | The caching mode. One of `PERSISTENT` or `EXPIRING`. Note that although the default is `EXPIRING` there is no caching by default due to the default value of `CACHE_ENVIRONMENT_DOCUMENT_SECONDS` | `PERSISTENT`                                           | `EXPIRING`                                    |
 | `CACHE_ENVIRONMENT_DOCUMENT_SECONDS`  | Number of seconds to cache the environment for (only relevant when `CACHE_ENVIRONMENT_DOCUMENT_MODE=EXPIRING`)                                                                                    | `60`                                                   | `0` ( = don't cache)                          |
 | `CACHE_ENVIRONMENT_DOCUMENT_BACKEND`  | Python path to the django cache backend chosen. See documentation [here](https://docs.djangoproject.com/en/4.2/topics/cache/).                                                                    | `django.core.cache.backends.memcached.PyMemcacheCache` | `django.core.cache.backends.db.DatabaseCache` |