Extend v0 API #2361
Extend v0 API #2361
Conversation
Summary by CodeRabbit
WalkthroughSwitches v0 API routers to RouterPaginated with a custom pagination class, refactors many endpoint schemas into Base/List/Detail/Error/Filter, adds detail endpoints and 404 mappings, renames model imports to *Model, adds DB indexes and migrations, bumps API version to 0.2.4, and updates tests to new schemas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (8)backend/tests/apps/api/rest/v0/event_test.py (1)
backend/tests/apps/api/rest/v0/organization_test.py (1)
backend/tests/apps/api/rest/v0/chapter_test.py (2)
backend/tests/apps/api/rest/v0/project_test.py (2)
backend/tests/apps/api/rest/v0/sponsor_test.py (1)
backend/tests/apps/api/rest/v0/issue_test.py (1)
backend/tests/apps/api/rest/v0/release_test.py (1)
backend/tests/apps/api/rest/v0/label_test.py (1)
⏰ 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). (3)
🔇 Additional comments (8)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/api/rest/v0/organization.py (1)
34-101: Mark nullable organization fields as optional.
OrganizationDetail.companyandOrganizationDetail.locationare typed asstr, yet the backingOrganizationmodel allows these fields to be blank/null (GitHub frequently returns them empty). When a stored row hasNULLfor either column the endpoint will now raise a 500 during response serialization. Please annotate them as optional (str | None) before release.
🧹 Nitpick comments (5)
backend/apps/api/rest/v0/member.py (2)
75-78: Ordering field should validate against model fields.The
orderingparameter accepts string literals but doesn't validate whether these fields exist on theUserModel. If the model structure changes, this could silently break ordering without errors.Consider adding validation or using a more type-safe approach:
from typing import Literal OrderingField = Literal["created_at", "-created_at", "updated_at", "-updated_at"] # Then use: ordering: OrderingField | None = Query( None, description="Ordering field", )Additionally, generate a verification script to confirm these fields exist on the User model:
#!/bin/bash # Verify that ordering fields exist on the User model ast-grep --pattern 'class User($$$): $$$ created_at = $$$ $$$ updated_at = $$$ $$$'
99-102: Consider extracting repeated error response pattern.The error response pattern
Response({"message": "..."}, status=HTTPStatus.NOT_FOUND)appears to be a common pattern that could be reused across endpoints.Consider creating a helper function:
def not_found_response(resource: str) -> Response: """Return a standardized 404 response.""" return Response( {"message": f"{resource} not found"}, status=HTTPStatus.NOT_FOUND ) # Then use: return not_found_response("Member")backend/tests/apps/api/rest/v0/event_test.py (1)
30-36: MockEvent implementation could be more explicit.The
MockEventclass dynamically sets attributes from the data dict, then explicitly setsnest_key = data["key"]. This pattern works but could be clearer.Consider making the mock more explicit about which attributes it provides:
class MockEvent: """Mock event object for testing ORM serialization.""" def __init__(self, data): self.key = data["key"] self.name = data["name"] self.description = data["description"] self.url = data["url"] self.end_date = data["end_date"] self.start_date = data["start_date"] self.nest_key = data["key"]Alternatively, if this pattern is used across multiple tests, extract it to a shared test utility.
#!/bin/bash # Check if similar MockEvent patterns exist in other test files rg -n "class Mock\w+:" backend/tests/apps/api/rest/v0/ -A 5backend/tests/apps/api/rest/v0/chapter_test.py (1)
30-36: MockChapter duplicates pattern from MockEvent.This
MockChapterclass follows the same pattern asMockEventin event_test.py. Consider extracting this to a shared test utility to reduce code duplication.Create a generic mock factory in a shared test utilities module:
# backend/tests/apps/api/rest/v0/conftest.py or test_utils.py def create_mock_orm_object(data: dict, nest_key_field: str = "key"): """Create a mock ORM object for testing schema serialization. Args: data: Dictionary of field values nest_key_field: Field name to use for nest_key property Returns: Mock object with attributes set from data and nest_key property """ class MockORM: def __init__(self, data, nest_key_field): for key, value in data.items(): setattr(self, key, value) self.nest_key = data[nest_key_field] return MockORM(data, nest_key_field) # Then use in tests: chapter = ChapterDetail.from_orm(create_mock_orm_object(chapter_data))Verify if other tests use this pattern:
#!/bin/bash # Find all Mock classes in test files rg -n "class Mock\w+:" backend/tests/apps/api/rest/v0/ -B 1 -A 5backend/apps/api/rest/v0/committee.py (1)
79-84: Rename handler to match committee endpoint.The detail handler is still called
get_chapterand the docstring says “Get chapter”, which is confusing now that this file serves committees. Please rename the function (and docstring) toget_committeeto align with the route, operation_id, and summary for maintainability.Apply this diff:
-@router.get( +@router.get( @@ -def get_chapter( +def get_committee( request: HttpRequest, committee_id: str = Path(example="project"), ) -> CommitteeDetail | CommitteeError: - """Get chapter.""" + """Get committee."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
backend/apps/api/rest/v0/__init__.py(2 hunks)backend/apps/api/rest/v0/chapter.py(1 hunks)backend/apps/api/rest/v0/committee.py(1 hunks)backend/apps/api/rest/v0/event.py(1 hunks)backend/apps/api/rest/v0/issue.py(1 hunks)backend/apps/api/rest/v0/label.py(2 hunks)backend/apps/api/rest/v0/member.py(1 hunks)backend/apps/api/rest/v0/organization.py(3 hunks)backend/apps/api/rest/v0/pagination.py(1 hunks)backend/apps/api/rest/v0/project.py(1 hunks)backend/apps/api/rest/v0/release.py(1 hunks)backend/apps/api/rest/v0/repository.py(1 hunks)backend/apps/api/rest/v0/sponsor.py(2 hunks)backend/apps/github/migrations/0037_rename_github_rele_created_d51966_idx_release_created_at_desc_idx_and_more.py(1 hunks)backend/apps/github/models/issue.py(1 hunks)backend/apps/github/models/release.py(1 hunks)backend/apps/github/models/repository.py(1 hunks)backend/apps/owasp/migrations/0054_event_event_end_date_desc_idx.py(1 hunks)backend/apps/owasp/models/event.py(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(2 hunks)backend/tests/apps/api/rest/v0/committee_test.py(2 hunks)backend/tests/apps/api/rest/v0/event_test.py(2 hunks)backend/tests/apps/api/rest/v0/issue_test.py(2 hunks)backend/tests/apps/api/rest/v0/label_test.py(2 hunks)backend/tests/apps/api/rest/v0/member_test.py(2 hunks)backend/tests/apps/api/rest/v0/organization_test.py(2 hunks)backend/tests/apps/api/rest/v0/project_test.py(2 hunks)backend/tests/apps/api/rest/v0/release_test.py(2 hunks)backend/tests/apps/api/rest/v0/repository_test.py(2 hunks)backend/tests/apps/api/rest/v0/sponsor_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
backend/tests/apps/api/rest/v0/issue_test.py (1)
backend/apps/api/rest/v0/issue.py (1)
IssueDetail(35-38)
backend/apps/owasp/migrations/0054_event_event_end_date_desc_idx.py (1)
backend/apps/github/migrations/0037_rename_github_rele_created_d51966_idx_release_created_at_desc_idx_and_more.py (1)
Migration(6-29)
backend/tests/apps/api/rest/v0/committee_test.py (3)
backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/api/rest/v0/committee.py (1)
CommitteeDetail(38-41)backend/apps/github/models/repository.py (1)
nest_key(152-154)
backend/apps/api/rest/v0/__init__.py (1)
backend/apps/api/rest/auth/api_key.py (1)
ApiKey(11-35)
backend/tests/apps/api/rest/v0/label_test.py (1)
backend/apps/api/rest/v0/label.py (1)
LabelDetail(28-31)
backend/tests/apps/api/rest/v0/event_test.py (2)
backend/apps/api/rest/v0/event.py (1)
EventDetail(34-37)backend/apps/github/models/repository.py (1)
nest_key(152-154)
backend/tests/apps/api/rest/v0/member_test.py (1)
backend/apps/api/rest/v0/member.py (1)
MemberDetail(35-45)
backend/tests/apps/api/rest/v0/organization_test.py (1)
backend/apps/api/rest/v0/organization.py (1)
OrganizationDetail(33-37)
backend/tests/apps/api/rest/v0/repository_test.py (2)
backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)backend/apps/api/rest/v0/repository.py (1)
RepositoryDetail(32-35)
backend/tests/apps/api/rest/v0/project_test.py (2)
backend/apps/github/models/repository.py (2)
project(162-164)nest_key(152-154)backend/apps/api/rest/v0/project.py (1)
ProjectDetail(40-43)
backend/apps/api/rest/v0/event.py (1)
backend/apps/owasp/models/event.py (1)
Event(25-310)
backend/apps/api/rest/v0/project.py (4)
backend/apps/github/models/repository.py (2)
project(162-164)nest_key(152-154)backend/apps/owasp/models/enums/project.py (1)
ProjectLevel(37-44)backend/apps/api/rest/v0/chapter.py (1)
resolve_key(29-31)backend/apps/api/rest/v0/committee.py (1)
resolve_key(29-31)
backend/tests/apps/api/rest/v0/chapter_test.py (2)
backend/apps/api/rest/v0/chapter.py (1)
ChapterDetail(38-42)backend/apps/github/models/repository.py (1)
nest_key(152-154)
backend/apps/api/rest/v0/issue.py (2)
backend/apps/github/models/generic_issue_model.py (3)
GenericIssueModel(10-77)State(18-20)repository_id(70-77)backend/apps/github/models/issue.py (1)
Issue(18-213)
backend/apps/api/rest/v0/release.py (1)
backend/apps/github/models/release.py (2)
Release(10-135)summary(59-61)
backend/apps/api/rest/v0/chapter.py (4)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/api/rest/v0/committee.py (2)
resolve_key(29-31)get_chapter(79-94)backend/apps/api/rest/v0/project.py (1)
resolve_key(31-33)backend/apps/github/models/repository.py (1)
nest_key(152-154)
backend/apps/api/rest/v0/committee.py (3)
backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/api/rest/v0/chapter.py (2)
resolve_key(29-31)get_chapter(87-99)backend/apps/github/models/repository.py (1)
nest_key(152-154)
backend/apps/github/migrations/0037_rename_github_rele_created_d51966_idx_release_created_at_desc_idx_and_more.py (1)
backend/apps/owasp/migrations/0054_event_event_end_date_desc_idx.py (1)
Migration(6-16)
backend/tests/apps/api/rest/v0/release_test.py (1)
backend/apps/api/rest/v0/release.py (1)
ReleaseDetail(33-36)
backend/apps/api/rest/v0/repository.py (2)
backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)backend/apps/github/models/repository.py (1)
Repository(25-352)
backend/tests/apps/api/rest/v0/sponsor_test.py (1)
backend/apps/api/rest/v0/sponsor.py (1)
SponsorDetail(33-39)
backend/apps/api/rest/v0/sponsor.py (1)
backend/apps/owasp/models/sponsor.py (1)
MemberType(29-32)
backend/apps/api/rest/v0/member.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (3)
avatar_url(13-15)login(23-25)name(18-20)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (13)
backend/apps/owasp/migrations/0054_event_event_end_date_desc_idx.py (1)
1-16: LGTM!The migration correctly implements the descending index on
end_datethat was added to the Event model. The dependency chain and index definition are accurate.backend/apps/owasp/models/event.py (1)
32-32: Confirm end_date index usage or remove unused index. We didn’t detect anyorder_byorfilteronend_datein the v0 API; verify that these endpoints will leverage this index or remove it if unused.backend/apps/api/rest/v0/member.py (3)
54-61: Filter schema is well-structured.The
MemberFilterschema withcompanyandlocationfields provides useful filtering capabilities with clear descriptions.
81-81: Verify presence and behavior of FilterSchema.filter
I couldn’t locate aFilterSchemadefinition in the codebase; confirm yourfilters.filter(UserModel.objects.order_by(...))call uses a valid schema whose.filter()method correctly applies filters to the queryset.
31-32: Empty Member schema is intentional
TheMemberclass extendsMemberBasesolely to represent the list‐view schema, mirroring other modules (e.g., Sponsor, Repository) and its docstring already documents the minimal‐fields design.Likely an incorrect or invalid review comment.
backend/tests/apps/api/rest/v0/member_test.py (3)
5-5: LGTM! Import updated to match new schema.The test correctly imports
MemberDetailto align with the API refactoring.
32-32: LGTM! Schema construction updated correctly.The test now uses
MemberDetailwhich matches the updated API surface.
49-49: Good security practice: verifying sensitive field exclusion.The test explicitly verifies that
backend/tests/apps/api/rest/v0/issue_test.py (1)
5-5: LGTM! Test updated to use new schema.The test correctly uses
IssueDetailto align with the API refactoring. All assertions remain valid.Also applies to: 31-31
backend/tests/apps/api/rest/v0/label_test.py (1)
3-3: LGTM! Test updated to use new schema.The test correctly uses
LabelDetailto align with the API refactoring. All assertions remain valid.Also applies to: 23-23
backend/apps/github/models/repository.py (1)
33-35: Confirm index addition and migrationIndex on the
namefield and its migration exist.backend/settings/base.py (1)
130-131: Confirm django-ninja pagination settings
CustomPagination exists and is correctly implemented. EnsureNINJA_PAGINATION_CLASSandNINJA_PAGINATION_PER_PAGEmatch the global setting names defined by django-ninja.backend/tests/apps/api/rest/v0/chapter_test.py (1)
37-37: Verify ORM mode configuration on ChapterDetail
EnsureSchema.Config.orm_mode = True(inherited byChapterDetail) so thatChapterDetail.from_orm(MockChapter(...))functions correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/apps/api/rest/v0/event.py (1)
65-83: Endpoint implementation depends on ORM mode configuration.The endpoint correctly uses response mapping for different status codes. However, line 81 returns a model instance directly, which requires ORM mode (flagged earlier).
Optional: Consider using tuple return for cleaner status code handling:
- return Response({"message": "Event not found"}, status=HTTPStatus.NOT_FOUND) + return HTTPStatus.NOT_FOUND, EventError(message="Event not found")This leverages django-ninja's automatic status code handling without the explicit
Responsewrapper.backend/apps/api/rest/v0/repository.py (2)
32-36: Consider enriching the detail schema.
RepositoryDetailonly addsdescriptionbeyond the base fields. The underlyingRepositorymodel exposes 30+ fields includingkey,languages,topics,license,size,stars_count,forks_count, and various boolean flags (is_archived,is_fork, etc.). For a detail endpoint, users typically expect richer information.If minimal fields are intentional for v0 API simplicity, document this design choice. Otherwise, consider adding commonly requested fields:
class RepositoryDetail(RepositoryBase): """Detail schema for Repository (used in single item endpoints).""" description: str | None = None + key: str + languages: dict = {} + topics: list[str] = [] + license: str = "" + stars_count: int = 0 + forks_count: int = 0 + is_archived: bool = False
96-99: Usekey__iexactinstead ofname__iexactin repository lookup (backend/apps/api/rest/v0/repository.py:96–99).
keyis alwaysname.lower(), so this is functionally equivalent and aligns with the internal API pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/api/rest/v0/event.py(1 hunks)backend/apps/api/rest/v0/pagination.py(1 hunks)backend/apps/api/rest/v0/release.py(1 hunks)backend/apps/api/rest/v0/repository.py(1 hunks)backend/tests/apps/api/rest/v0/event_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/api/rest/v0/pagination.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/apps/api/rest/v0/event.py (1)
backend/apps/owasp/models/event.py (1)
Event(25-310)
backend/apps/api/rest/v0/repository.py (2)
backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)backend/apps/github/models/repository.py (1)
Repository(25-352)
backend/tests/apps/api/rest/v0/event_test.py (1)
backend/apps/api/rest/v0/event.py (1)
EventDetail(34-37)
backend/apps/api/rest/v0/release.py (1)
backend/apps/github/models/release.py (2)
Release(10-135)summary(59-61)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (10)
backend/apps/api/rest/v0/release.py (3)
20-27: Past review comment addressed correctly.The
published_atfield is now properly typed asdatetime | None = None, which aligns with the model reality where this field can be null for draft releases or failed syncs. This resolves the previous 500 error risk during serialization.
78-94: Well-implemented list endpoint with good performance considerations.The queryset correctly:
- Excludes draft releases (null
published_at)- Uses
select_relatedto avoid N+1 queries- Applies case-insensitive filters for organization and repository
- Provides deterministic ordering with secondary sort
115-119: Verify intended behavior: detail endpoint can return draft releases.The
list_releaseendpoint (line 79) explicitly excludes releases with nullpublished_at, ensuring only published releases appear in lists. However,get_releasedoes not apply this filter, allowing draft releases to be fetched directly.This creates an API inconsistency where:
- List: only published releases
- Detail: includes draft releases
If this is intentional (e.g., for admin preview or direct-link access), consider documenting it. Otherwise, apply the same
published_at__isnull=Falsefilter here for consistency.If the filter should be applied, use this diff:
try: - return ReleaseModel.objects.get( + return ReleaseModel.objects.exclude(published_at__isnull=True).get( repository__organization__login__iexact=organization_id, repository__name__iexact=repository_id, tag_name=release_id, )backend/tests/apps/api/rest/v0/event_test.py (1)
29-43: LGTM! Clean test implementation.The test correctly validates the
EventDetailschema with proper datetime conversion and comprehensive field assertions. The direct instantiation approach is cleaner than the previous.from_orm()pattern.backend/apps/api/rest/v0/event.py (4)
30-32: LGTM!Clean minimal schema for list display, correctly inheriting from
EventBase.
34-37: LGTM!Correctly extends
EventBasewith the optionaldescriptionfield for detail endpoints.
40-43: LGTM!Standard error response schema following common patterns.
46-62: Verify ORM mode is configured before deployment.The endpoint returns a QuerySet directly (line 62), which django-ninja will serialize using the
Eventschema. This requires ORM mode to be configured onEventBase(flagged in the previous comment).backend/apps/api/rest/v0/repository.py (2)
35-35: Note on nullable description type hint.The type hint
str | Noneaddresses the past review concern about nullable descriptions. However, the underlying model field (Repository.description) is defined asCharField(blank=True)withoutnull=True, meaning the database stores empty strings ("") rather thanNULL. The type hint is more permissive than necessary but provides defensive handling if the model changes later.Based on past review comments.
54-76: LGTM! Well-optimized list endpoint.The endpoint correctly uses
select_related("organization")to avoid N+1 queries and applies a secondary sort byupdated_atfor deterministic ordering when the primary field has duplicates. The filter integration and response typing are appropriate.
|
Proposed change
Extend
v0API endpoints.Checklist
make check-testlocally; all checks and tests passed.