Skip to content

Conversation

Roffenlund
Copy link
Contributor

Annotate the queryset with active_package_versions instead of calling PackageVersion.is_removed, which avoids triggering an N+1 query.

Refs. TS-2672

Copy link

coderabbitai bot commented Oct 3, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

  • CyberstormPackageDependencySerializer: is_removed changed from BooleanField to SerializerMethodField with get_is_removed(obj) computing removal based on package-wide active status and the specific version’s is_active.
  • PackageVersionDependenciesListAPIView: queryset now annotates package_has_active_versions via Exists with OuterRef to detect any active versions for the package.
  • Tests: added parametrized test to validate is_active and is_removed in dependency listings; removed unused imports.
  • Query count test: replaced hard-coded max with shared MAX_QUERIES constant.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the main change of optimizing the dependencies list queryset and includes the relevant ticket reference, making it clear and directly related to the changeset.
Description Check ✅ Passed The description accurately summarizes the change to annotate the queryset to avoid N+1 queries and references the associated ticket, making it directly related to the implementation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cyberstorm-api-improve-dependencies-endpoint-db-query

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8f1e3a and a51c2c1.

📒 Files selected for processing (4)
  • django/thunderstore/api/cyberstorm/serializers/package.py (1 hunks)
  • django/thunderstore/api/cyberstorm/tests/test_package_version_list.py (1 hunks)
  • django/thunderstore/api/cyberstorm/tests/test_query_count.py (1 hunks)
  • django/thunderstore/api/cyberstorm/views/package_version_list.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
django/thunderstore/api/cyberstorm/tests/test_package_version_list.py (3)
django/conftest.py (1)
  • api_client (442-443)
django/thunderstore/repository/factories.py (2)
  • PackageFactory (44-52)
  • PackageVersionFactory (63-72)
django/thunderstore/repository/models/package.py (1)
  • dependencies (220-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Test pytest (2)
  • GitHub Check: Test pytest (6)
  • GitHub Check: Test pytest (4)
  • GitHub Check: Test pytest (1)
  • GitHub Check: Test pytest (5)
  • GitHub Check: Test pytest (3)
  • GitHub Check: Test pytest (6)
  • GitHub Check: Test pytest (3)
  • GitHub Check: Test pytest (5)
  • GitHub Check: Test pytest (4)
  • GitHub Check: Test pytest (2)
  • GitHub Check: Test pytest (1)
🔇 Additional comments (4)
django/thunderstore/api/cyberstorm/tests/test_query_count.py (1)

236-236: LGTM!

The use of the MAX_QUERIES constant aligns with the other tests in this file and improves maintainability.

django/thunderstore/api/cyberstorm/views/package_version_list.py (1)

57-63: LGTM!

The Exists annotation correctly identifies packages with at least one active version, avoiding the N+1 query issue. The annotation field package_has_active_versions is properly consumed by the serializer's get_is_removed method.

django/thunderstore/api/cyberstorm/tests/test_package_version_list.py (1)

137-166: LGTM!

The test correctly validates the is_removed and is_active fields in the dependencies endpoint. The parametrization covers both active and inactive dependency versions, and the assertions align with the serializer's logic.

django/thunderstore/api/cyberstorm/serializers/package.py (1)

72-78: LGTM!

The get_is_removed method correctly computes the removal status by first checking package-level removal (inactive package or no active versions), then falling back to version-level status. This approach leverages the package_has_active_versions annotation from the view to avoid N+1 queries.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Annotate the queryset with active_package_versions instead of calling
PackageVersion.is_removed, which avoids triggering an N+1 query.

Refs. TS-2672
@Roffenlund Roffenlund force-pushed the cyberstorm-api-improve-dependencies-endpoint-db-query branch from d44114e to a51c2c1 Compare October 3, 2025 13:47
Copy link

codecov bot commented Oct 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.74%. Comparing base (e8f1e3a) to head (a51c2c1).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1187   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files         337      337           
  Lines       10319    10324    +5     
  Branches      939      940    +1     
=======================================
+ Hits         9570     9575    +5     
  Misses        621      621           
  Partials      128      128           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

LGTM

@Roffenlund Roffenlund merged commit 3777922 into master Oct 6, 2025
28 checks passed
@Roffenlund Roffenlund deleted the cyberstorm-api-improve-dependencies-endpoint-db-query branch October 6, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants