Skip to content

fix(Segments): Fix N+1 on get_segment retrieve #7563

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.

Sign up for GitHub

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

Open
matthewelwell wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −1
Open

fix(Segments): Fix N+1 on get_segment retrieve #7563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion api/segments/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def get_queryset(self): # type: ignore[no-untyped-def]
project = self.get_project()
queryset = Segment.live_objects.filter(project=project, is_system_segment=False)

if self.action == "list":
if self.action in ("list", "retrieve"):
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

high

The N+1 optimization (prefetching rules and conditions) should also be applied to other actions that return the full segment representation, such as update, partial_update, and clone. This ensures that the response serialization and the cloning logic (which iterates over rules/conditions) do not trigger redundant database queries.

Suggested change
if self.action in ("list", "retrieve"):
if self.action in ("list", "retrieve", "update", "partial_update", "clone"):

# TODO: at the moment, the UI only shows the name and description of the segment in the list view.
# we shouldn't return all of the rules and conditions in the list view.
queryset = queryset.prefetch_related(
Expand All @@ -116,6 +116,9 @@ def get_queryset(self): # type: ignore[no-untyped-def]
"metadata",
)

if self.action == "retrieve":
queryset = queryset.select_related("project__organisation")
Comment on lines +119 to +120
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

high

The select_related("project__organisation") optimization should be extended to all detail actions that trigger object permission checks via get_object(). This includes update, partial_update, destroy, associated_features, and clone. This prevents redundant lazy-loading of the project and organisation during the has_object_permission check in SegmentPermissions.

Suggested change
if self.action == "retrieve":
queryset = queryset.select_related("project__organisation")
if self.action in ("retrieve", "update", "partial_update", "destroy", "associated_features", "clone"):
queryset = queryset.select_related("project__organisation")


query_serializer = SegmentListQuerySerializer(data=self.request.query_params)
query_serializer.is_valid(raise_exception=True)

Expand Down
45 changes: 45 additions & 0 deletions api/tests/unit/segments/test_unit_segments_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,51 @@ def test_list_segments__without_rbac__expected_num_queries(
assert response_json["count"] == num_segments


@pytest.mark.skipif(
settings.IS_RBAC_INSTALLED is True,
reason="Skip this test if RBAC is installed",
)
@pytest.mark.parametrize(
"client, expected_num_queries",
[
(lazy_fixture("admin_master_api_key_client"), 12),
(lazy_fixture("admin_client"), 14),
],
)
def test_retrieve_segment__without_rbac__expected_num_queries(
django_assert_num_queries: DjangoAssertNumQueries,
project: Project,
client: APIClient,
required_a_segment_metadata_field: MetadataModelField,
expected_num_queries: int,
) -> None:
# Given
# A segment with metadata and multiple rules and conditions to expose any N+1 issues.
segment = Segment.objects.create(project=project, name="test segment")
Metadata.objects.create(
object_id=segment.id,
content_type=ContentType.objects.get_for_model(segment),
model_field=required_a_segment_metadata_field,
field_value="test",
)
all_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE)
for _ in range(3):
any_rule = SegmentRule.objects.create(rule=all_rule, type=SegmentRule.ANY_RULE)
Condition.objects.create(property="foo", value="bar", rule=any_rule)

url = reverse(
"api-v1:projects:project-segments-detail",
args=[project.id, segment.id],
)

# When
with django_assert_num_queries(expected_num_queries):
response = client.get(url)

# Then
assert response.status_code == status.HTTP_200_OK


def test_list_segments__system_segment_exists__excludes_system_segment(
project: Project,
admin_client: APIClient,
Expand Down
Loading