Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds explicit http_method_names = ["get", "post", "head", "delete"] to three transect viewsets, restricting accepted HTTP verbs to GET, POST, HEAD, and DELETE (omitting OPTIONS, PUT, and PATCH). ChangesTransect HTTP method allowlist
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
cdk-nag report✅ No unsuppressed errors. See |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/resources/benthic_transect.py (1)
38-43: 💤 Low valueUse
http_method_namesto disable PUT/PATCH on these transect viewsets.
BenthicTransectViewSet(and the same pattern inFishBeltTransectViewSet/InvertBeltTransectViewSet) currently overridesupdate/partial_updatejust to raiseMethodNotAllowed. This repo already useshttp_method_namesin other DRF resources (e.g.src/api/resources/sampleunitmethods/*andsrc/api/resources/notification.py), so switching would make method support/schema/OPTIONSmore declarative.♻️ Suggested alternative
-import django_filters -from rest_framework.exceptions import MethodNotAllowed +import django_filters from ..models import BenthicTransect from .base import BaseProjectApiViewSet from .sample_units_base import SampleUnitFilterSet, SampleUnitSerializer @@ class BenthicTransectViewSet(BaseProjectApiViewSet): serializer_class = BenthicTransectSerializer queryset = BenthicTransect.objects.all() filterset_class = BenthicTransectFilterSet - - def update(self, request, *args, **kwargs): - raise MethodNotAllowed("PUT") - - def partial_update(self, request, *args, **kwargs): - raise MethodNotAllowed("PATCH") + http_method_names = [m for m in BaseProjectApiViewSet.http_method_names if m not in ("put", "patch")]The same change would apply to
FishBeltTransectViewSetandInvertBeltTransectViewSet. Feel free to disregard if the explicit-override style is preferred for consistency with the rest of the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/resources/benthic_transect.py` around lines 38 - 43, The viewsets currently override update/partial_update to raise MethodNotAllowed; instead remove those overrides in BenthicTransectViewSet (and apply the same change to FishBeltTransectViewSet and InvertBeltTransectViewSet) and declare http_method_names on each class to explicitly list supported methods (e.g. omit 'put' and 'patch' while keeping 'get','post','delete','head','options' as appropriate) so DRF and schema/OPTIONS reflect the allowed methods declaratively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/resources/benthic_transect.py`:
- Around line 38-43: The viewsets currently override update/partial_update to
raise MethodNotAllowed; instead remove those overrides in BenthicTransectViewSet
(and apply the same change to FishBeltTransectViewSet and
InvertBeltTransectViewSet) and declare http_method_names on each class to
explicitly list supported methods (e.g. omit 'put' and 'patch' while keeping
'get','post','delete','head','options' as appropriate) so DRF and schema/OPTIONS
reflect the allowed methods declaratively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a3bcbe6-7d52-478f-a2ec-412c02dc4214
📒 Files selected for processing (3)
src/api/resources/benthic_transect.pysrc/api/resources/fish_belt_transect.pysrc/api/resources/invert_belt_transect.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
gridcell
left a comment
gridcell
left a comment
There was a problem hiding this comment.
AI Code Review
Assessment: Clean, well-scoped bug fix that correctly removes PUT and PATCH from three transect ViewSets using the standard DRF http_method_names mechanism. The main gap is the absence of regression tests asserting 405 on the newly restricted endpoints. Two minor style issues (redundant options entry, verb ordering) and a missing changelog entry round out the findings.
| Severe | Warning | Question | Info | Praise |
|---|---|---|---|---|
| 0 | 1 | 1 | 3 | 1 |
Files reviewed: 3 / 3
Cross-Cutting Concerns
- No regression tests assert 405 for PUT/PATCH on the three restricted transect endpoints. A parametrized test covering all three viewsets would provide CI-level protection against the restriction being accidentally undone.
- No CHANGELOG or API documentation update accompanies this breaking API contract change. Downstream consumers will discover the method restriction at runtime rather than through release notes or updated OpenAPI docs.
- The sibling
sampleunitmethodsViewSets retainputwhile the transect ViewSets now support only create/read/delete. This asymmetric model is architecturally intentional but currently undocumented; a brief comment on each ViewSet or a note innew_protocol_readme.mdwould prevent future drift.
CR-001 · warning · test_gap — No regression tests assert 405 for PUT/PATCH on the three restricted transect endpoints
The PR adds http_method_names to BenthicTransectViewSet, FishBeltTransectViewSet, and InvertBeltTransectViewSet to block PUT and PATCH, but the diff contains no test changes. DRF resolves http_method_names at dispatch time before any view logic runs, so the restriction is reliable today, but it is invisible to CI. A future refactor that removes or overrides http_method_names (e.g., a mixin override, base-class change, or override in a child class) would silently re-expose the blocked methods and pass all tests.
Suggestion: Add a small parametrized test asserting 405 Method Not Allowed for PUT and PATCH against each of the three endpoints (
benthictransect-detail,fishbelttransect-detail,invertbelttransect-detail). Mirror the structure of existing API tests insrc/api/tests/.
Why it matters: Without a test, the method restriction is invisible to CI. Any future change that widens
http_method_namesback to include PUT or PATCH would pass all tests while silently re-exposing an endpoint that was deliberately locked down.
Applies to: src/api/resources/benthic_transect.py, src/api/resources/fish_belt_transect.py, src/api/resources/invert_belt_transect.py
Found by: reviewer-tests
CR-002 · question · api_contract — Confirm no existing client code issues PUT or PATCH to these transect endpoints
Worth confirming: (a) is there any client code — including the mobile app or sync payloads — that currently issues PUT or PATCH to these bare transect endpoints and will now receive 405 Method Not Allowed? (b) are there any in-flight mobile sync payloads that rely on PUT semantics for upsert on these endpoints? If so, a coordinated client release would be needed alongside this server-side change.
Applies to: src/api/resources/benthic_transect.py, src/api/resources/fish_belt_transect.py, src/api/resources/invert_belt_transect.py
Found by: reviewer-backend-python
CR-005 · info · docs — Breaking API change has no accompanying changelog or doc update
Removing PUT and PATCH from these three endpoints is a breaking change to the public API contract. No changelog or release-notes entry accompanies the PR. The auto-generated OpenAPI documentation at https://mermaid-api.readthedocs.io/ will also need to be regenerated so downstream API consumers see the updated allowed methods.
Applies to: src/api/resources/benthic_transect.py, src/api/resources/fish_belt_transect.py, src/api/resources/invert_belt_transect.py
Found by: reviewer-docs
CR-006 · praise · maintainability — Correct, minimal mechanism for restricting HTTP methods with clear intent
Disabling PUT and PATCH via DRF's http_method_names is the right approach: disallowed verbs are rejected at dispatch time with 405 before any permission or serializer code is reachable, so there is no authorization regression. It avoids overriding update()/partial_update() in application code, keeps the restriction visible at the class level, and the three-file change is perfectly parallel, making intent obvious.
Applies to: src/api/resources/benthic_transect.py, src/api/resources/fish_belt_transect.py, src/api/resources/invert_belt_transect.py
Found by: reviewer-security, reviewer-backend-python
Agent Coverage
| Agent | Role | Files | Findings | Blocking |
|---|---|---|---|---|
| reviewer-security | security | 3 | 3 | 0 |
| reviewer-backend-python | backend | 3 | 3 | 0 |
| reviewer-python-quality | code_quality | 3 | 2 | 0 |
| reviewer-tests | testing | 3 | 2 | 0 |
| reviewer-docs | documentation | 3 | 1 | 0 |
Generated by AI Review Team
2 participants
Summary by CodeRabbit