Skip to content

Improve channels API performance, remove all channels CRUD code - #3388

Merged
mbertrand merged 1 commit into
mainfrom
mb/channel_perf
Jun 10, 2026
Merged

Improve channels API performance, remove all channels CRUD code#3388
mbertrand merged 1 commit into
mainfrom
mb/channel_perf

Conversation

@mbertrand

@mbertrand mbertrand commented May 29, 2026

Copy link
Copy Markdown
Member

What are the relevant tickets?

Related to https://github.com/mitodl/hq/issues/9157

Description (What does it do?)

Improves the performance of the channels API in two ways:

  • Removes most N+1 queries when serializing channels. A new ChannelQuerySet.with_detail_relations() applies the full prefetch_related/select_related/annotate chain the ChannelSerializer needs, so the list and detail endpoints render with a constant number of queries instead of scaling with the number of lists/sub-channels per channel.
  • Caches channel responses for all users
  • Removes all backend CRUD code for channels and some leftover frontend CRUD code (no longer needed).

Also adds a k6 load test for the channel endpoints used to measure the before/after improvement.

How can this be tested?

Automated tests

docker compose run --rm web uv run pytest channels/ main/utils_test.py

Covers the prefetch query counts, the moderator/staff cache-bypass behavior, and the key-prefixed cache invalidation.

Load test (k6) — measuring the perf improvement

  1. Seed channels with enough lists/sub-channels to make the N+1 difference measurable (idempotent — re-running replaces the existing loadtest-* channels):
    docker compose exec -T web python manage.py shell <<'PY'
    from channels.factories import ChannelFactory, ChannelListFactory, SubChannelFactory
    from channels.models import Channel
    from learning_resources.factories import LearningPathFactory
    
    NUM_CHANNELS = 10
    LISTS_PER_CHANNEL = 12
    SUBCHANNELS_PER_CHANNEL = 8
    
    existing = Channel.objects.filter(name__startswith="loadtest-")
    print(f"deleting {existing.count()} existing loadtest channels")
    existing.delete()
    
    for i in range(NUM_CHANNELS):
        name = f"loadtest-{i}"
        channel = ChannelFactory.create(
            name=name,
            title=f"Load Test Channel {i}",
            is_topic=True,
            published=True,
            featured_list=LearningPathFactory.create().learning_resource,
        )
        ChannelListFactory.create_batch(LISTS_PER_CHANNEL, channel=channel)
        SubChannelFactory.create_batch(SUBCHANNELS_PER_CHANNEL, parent_channel=channel)
        print(f"  {name}: {channel.lists.count()} lists, "
              f"{channel.sub_channels.count()} sub_channels, "
              f"featured_list={channel.featured_list_id}")
    
    print(f"seeded {NUM_CHANNELS} rich loadtest channels")
    PY
  2. Run the channels load test against your backend. BACKEND_BASE_URL=http://learn.odl.local:8061 hits Django directly (no APISIX gateway required). The Docker helper mounts load_testing/ at /app (note: scripts/k6.sh already supplies the run subcommand, so don't pass it yourself):
    ./scripts/k6.sh /app/channels.compare.ts \
      -e BACKEND_BASE_URL=http://learn.odl.local:8061 \
      -e RUN_LABEL=after -e VUS=15 -e DURATION=3m
      -e CHANNEL_NAME_PREFIX=loadtest-  
    
    The summary prints p50/p95/p99 per endpoint, and full results are written to load_testing/results/channels..json.

Copilot AI review requested due to automatic review settings May 29, 2026 20:55
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

OpenAPI Changes

66 changes: 18 error, 0 warning, 48 info

View full changelog

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR improves channel API performance by adding shared caching for cache-safe channel responses, centralizing channel detail prefetching, and adding load tests to compare channel endpoint behavior.

Changes:

  • Adds request-time cache timeout resolution and channel-specific cache bypassing for moderators/staff.
  • Moves detailed channel queryset prefetch/select logic into ChannelQuerySet.with_detail_relations().
  • Adds tests and k6 load-testing scripts for channel list/detail/count endpoints.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
channels/models.py Adds reusable queryset helper for channel detail serialization relations.
channels/views.py Applies optimized querysets and cache decorators to channel endpoints.
channels/views_test.py Adds query-count and channel cache behavior coverage.
main/utils.py Allows cache decorators to default to REDIS_VIEW_CACHE_DURATION at request time.
main/utils_test.py Tests deferred default timeout behavior.
load_testing/backend/client/client.ts Allows extra headers in generated k6 API clients.
load_testing/backend/channels.ts Adds k6 channel endpoint discovery and request scenarios.
load_testing/channels.compare.ts Adds a k6 entrypoint for channel load comparison runs.
load_testing/userinfo.ts Adds helpers for building load-test user identity headers.
openapi/specs/v0.yaml Updates counts endpoint description text.

Comment thread load_testing/channels.compare.ts
Comment thread openapi/specs/v0.yaml Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Comment thread channels/views.py Outdated
Comment thread openapi/specs/v0.yaml Outdated
Comment thread channels/views.py Outdated
Comment thread channels/views.py Outdated
@mbertrand mbertrand added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Jun 1, 2026
@abeglova abeglova self-assigned this Jun 2, 2026
Comment thread channels/models.py
(see ``ChannelQuerySet.with_detail_relations``) so serializing a
channel does not trigger an extra query per list.
"""
return [channel_list.channel_list for channel_list in self.lists.all()]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be ordered by position?

@mbertrand mbertrand Jun 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ChannelList.Meta1 sets ordering = ["position"], so self.lists.all() is position-ordered even on the non-prefetched path.

with_detail_relations prefetches lists with an explicit .order_by("position") now, so the prefetched path is too.

I'll add a unit test to make sure though

Comment thread channels/models.py Outdated
Comment thread channels/models.py
"topic_detail",
"department_detail",
"unit_detail",
"pathway_detail",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice to pull just the relevant one for the channel type when making an api call for one channel but probably it's not a big deal to pull them since the queries are small when the relevant detail is null

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The list endpoint for all channels would need all of them I think, inclined to think it's not worth creating different queries here for a channel detail view vs a list view. On the other hand not sure if the list view is actually used for anything anyway.

Comment thread channels/serializers.py Outdated
Comment thread channels/views.py Outdated
@abeglova abeglova assigned mbertrand and unassigned abeglova Jun 3, 2026
@mbertrand mbertrand changed the title Improve channels API performance Improve channels API performance, remove all channels CRUD code Jun 3, 2026
Comment thread channels/serializers.py

@abeglova abeglova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread channels/views.py
Comment on lines +148 to 153
return queryset

@method_decorator(
cache_page_for_all_users(
settings.REDIS_VIEW_CACHE_DURATION, cache="redis", key_prefix="channels"
)
)
def list(self, request, *args, **kwargs):
@method_decorator(cache_channel_response())
def list(self, request: Request, *args, **kwargs) -> Response:
"""List channel counts by resource type."""
return super().list(request, *args, **kwargs)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The overridden get_object method in ChannelByTypeNameDetailView bypasses object-level permission checks by not calling self.check_object_permissions(), creating a potential future vulnerability.
Severity: LOW

Suggested Fix

After retrieving the obj in the get_object method, add a call to self.check_object_permissions(self.request, obj) before returning the object. This will ensure that any future object-level permission classes added to the view are correctly enforced, adhering to DRF best practices.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: channels/views.py#L148-L153

Potential issue: The `get_object` method in `ChannelByTypeNameDetailView` is overridden
to fetch a `Channel` object but omits the standard call to
`self.check_object_permissions(self.request, obj)`. While access is currently controlled
by a `published=True` filter in the queryset and no object-level permissions are defined
for `Channel` models, this omission creates a latent bug. If object-level permissions
are added to this view in the future (e.g., to restrict access based on ownership), they
will not be enforced, potentially leading to unauthorized data access. This violates
Django REST Framework's documented best practices and creates a maintenance trap.

@mbertrand
mbertrand merged commit fec0f28 into main Jun 10, 2026
13 of 14 checks passed
@mbertrand
mbertrand deleted the mb/channel_perf branch June 10, 2026 18:03
@odlbot odlbot mentioned this pull request Jun 11, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants