Feature/snapshot subscription graphql#5058
Conversation
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Summary by CodeRabbit
WalkthroughAdds snapshot subscription storage, Django admin registration, GraphQL query and mutation support, schema wiring, and unit tests for the model, admin, query, node, and mutation paths. ChangesSnapshot subscription feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/community-snapshots #5058 +/- ##
=============================================================
Coverage 98.82% 98.82%
=============================================================
Files 541 546 +5
Lines 17289 17435 +146
Branches 2496 2504 +8
=============================================================
+ Hits 17086 17231 +145
Misses 88 88
- Partials 115 116 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py`:
- Around line 167-173: The unsubscribe-token lookup in SnapshotSubscription
mutation only handles SnapshotSubscription.DoesNotExist and ValueError, but
invalid UUID input raises Django’s ValidationError before the query completes.
Update the exception handling in the mutation that calls
SnapshotSubscription.objects.get(unsubscribe_token=token) to also catch
ValidationError, and make sure the needed import from django.core.exceptions is
added so malformed tokens return the existing invalid-token result instead of an
unhandled error.
- Around line 63-86: The subscription creation logic in SnapshotSubscription
mutation is checking only
SnapshotSubscription.objects.filter(user=user).exists(), which blocks
re-subscription after cancel and still allows a race before create(). Update the
check to distinguish active vs inactive subscriptions (for example, based on
is_active in the SnapshotSubscription model) so canceled users can subscribe
again, and make the create path resilient to concurrent requests by handling the
one-to-one constraint failure around the subscription creation in the mutation
that builds the SnapshotSubscriptionResult.
In `@backend/apps/owasp/models/snapshot_subscription.py`:
- Around line 34-45: The subscription creation flow is blocking re-subscribes
because it treats inactive records as existing subscriptions. Update the
existence check in create_snapshot_subscription to only consider active
SnapshotSubscription rows by filtering on user and is_active=True, while keeping
the cancellation/unsubscribe behavior in cancel_snapshot_subscription and
unsubscribe_by_token unchanged so users can recreate a subscription after
canceling it.
In `@backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py`:
- Around line 12-45: The test only checks SnapshotSubscriptionAdmin directly and
misses whether apps.owasp.admin actually registers the model with admin.site.
Update the test in snapshot_subscription_test to import the admin package/module
that performs registration and assert SnapshotSubscription is present in
admin.site._registry, or that the registry entry is an instance of
SnapshotSubscriptionAdmin, so a broken import or missing admin.site.register
call is caught.
In
`@backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py`:
- Around line 187-195: Update test_malformed_token in
snapshot_subscription_test.py to match Django ORM behavior by mocking
django.core.exceptions.ValidationError instead of ValueError for
SnapshotSubscription.objects.get and keep the same unsubscribe_by_token
assertion. Also add a new test_create_after_cancel around
create_snapshot_subscription to verify a user can create a new subscription
after a previously cancelled/inactive one, so the test covers the intended
recreate flow rather than blocking on any existing record.
In
`@backend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.py`:
- Around line 34-59: The snapshot subscription tests are calling the Strawberry
Django field descriptor directly instead of the underlying resolver, so they
never execute the real my_subscription logic. Update the tests in
SnapshotSubscriptionQuery to fetch the my_subscription field from the class and
invoke its base_resolver.func with self.query and info, so the resolver
implementation is actually exercised for the authenticated, not found, and found
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a833131-728a-4590-803c-f98c71da2af2
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (18)
backend/apps/owasp/admin/__init__.pybackend/apps/owasp/admin/snapshot_subscription.pybackend/apps/owasp/api/internal/mutations/__init__.pybackend/apps/owasp/api/internal/mutations/snapshot_subscription.pybackend/apps/owasp/api/internal/nodes/snapshot_subscription.pybackend/apps/owasp/api/internal/queries/__init__.pybackend/apps/owasp/api/internal/queries/snapshot_subscription.pybackend/apps/owasp/migrations/0075_snapshotsubscription.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/snapshot_subscription.pybackend/data/nest.dumpbackend/settings/graphql.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/mutations/__init__.pybackend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/models/snapshot_subscription_test.py
| if SnapshotSubscription.objects.filter(user=user).exists(): | ||
| return SnapshotSubscriptionResult( | ||
| ok=False, | ||
| message="Subscription already exists.", | ||
| ) | ||
|
|
||
| if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices): | ||
| return SnapshotSubscriptionResult( | ||
| ok=False, | ||
| message="Invalid frequency. Must be 'weekly' or 'monthly'.", | ||
| ) | ||
|
|
||
| subscription = SnapshotSubscription.objects.create( | ||
| user=user, | ||
| frequency=input_data.frequency, | ||
| include_chapters=input_data.include_chapters, | ||
| include_events=input_data.include_events, | ||
| include_issues=input_data.include_issues, | ||
| include_posts=input_data.include_posts, | ||
| include_projects=input_data.include_projects, | ||
| include_pull_requests=input_data.include_pull_requests, | ||
| include_releases=input_data.include_releases, | ||
| include_users=input_data.include_users, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Canceled users cannot subscribe again, and concurrent creates can still 500.
Because cancel/unsubscribe only flips is_active to False, this exists() check permanently blocks re-subscription for that user. It also leaves a race window before create() against the model’s one-to-one constraint, so two overlapping requests can both pass Line 63 and one will fail with an unhandled integrity error.
Proposed fix
+from django.db import IntegrityError, transaction
+
`@strawberry.mutation`(permission_classes=[IsAuthenticated])
def create_snapshot_subscription(
self,
info: Info,
input_data: CreateSnapshotSubscriptionInput,
) -> SnapshotSubscriptionResult:
"""Create a new snapshot subscription for the logged-in user."""
user = info.context.request.user
-
- if SnapshotSubscription.objects.filter(user=user).exists():
- return SnapshotSubscriptionResult(
- ok=False,
- message="Subscription already exists.",
- )
if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices):
return SnapshotSubscriptionResult(
ok=False,
message="Invalid frequency. Must be 'weekly' or 'monthly'.",
)
- subscription = SnapshotSubscription.objects.create(
- user=user,
- frequency=input_data.frequency,
- include_chapters=input_data.include_chapters,
- include_events=input_data.include_events,
- include_issues=input_data.include_issues,
- include_posts=input_data.include_posts,
- include_projects=input_data.include_projects,
- include_pull_requests=input_data.include_pull_requests,
- include_releases=input_data.include_releases,
- include_users=input_data.include_users,
- )
+ defaults = {
+ "frequency": input_data.frequency,
+ "include_chapters": input_data.include_chapters,
+ "include_events": input_data.include_events,
+ "include_issues": input_data.include_issues,
+ "include_posts": input_data.include_posts,
+ "include_projects": input_data.include_projects,
+ "include_pull_requests": input_data.include_pull_requests,
+ "include_releases": input_data.include_releases,
+ "include_users": input_data.include_users,
+ "is_active": True,
+ }
+
+ try:
+ with transaction.atomic():
+ subscription, created = SnapshotSubscription.objects.get_or_create(
+ user=user,
+ defaults=defaults,
+ )
+ except IntegrityError:
+ return SnapshotSubscriptionResult(
+ ok=False,
+ message="Subscription already exists.",
+ )
+
+ if not created:
+ for field_name, value in defaults.items():
+ setattr(subscription, field_name, value)
+ subscription.save()
return SnapshotSubscriptionResult(
ok=True,
message="Subscription created successfully.",
subscription=subscription,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if SnapshotSubscription.objects.filter(user=user).exists(): | |
| return SnapshotSubscriptionResult( | |
| ok=False, | |
| message="Subscription already exists.", | |
| ) | |
| if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices): | |
| return SnapshotSubscriptionResult( | |
| ok=False, | |
| message="Invalid frequency. Must be 'weekly' or 'monthly'.", | |
| ) | |
| subscription = SnapshotSubscription.objects.create( | |
| user=user, | |
| frequency=input_data.frequency, | |
| include_chapters=input_data.include_chapters, | |
| include_events=input_data.include_events, | |
| include_issues=input_data.include_issues, | |
| include_posts=input_data.include_posts, | |
| include_projects=input_data.include_projects, | |
| include_pull_requests=input_data.include_pull_requests, | |
| include_releases=input_data.include_releases, | |
| include_users=input_data.include_users, | |
| ) | |
| user = info.context.request.user | |
| if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices): | |
| return SnapshotSubscriptionResult( | |
| ok=False, | |
| message="Invalid frequency. Must be 'weekly' or 'monthly'.", | |
| ) | |
| defaults = { | |
| "frequency": input_data.frequency, | |
| "include_chapters": input_data.include_chapters, | |
| "include_events": input_data.include_events, | |
| "include_issues": input_data.include_issues, | |
| "include_posts": input_data.include_posts, | |
| "include_projects": input_data.include_projects, | |
| "include_pull_requests": input_data.include_pull_requests, | |
| "include_releases": input_data.include_releases, | |
| "include_users": input_data.include_users, | |
| "is_active": True, | |
| } | |
| try: | |
| with transaction.atomic(): | |
| subscription, created = SnapshotSubscription.objects.get_or_create( | |
| user=user, | |
| defaults=defaults, | |
| ) | |
| except IntegrityError: | |
| return SnapshotSubscriptionResult( | |
| ok=False, | |
| message="Subscription already exists.", | |
| ) | |
| if not created: | |
| for field_name, value in defaults.items(): | |
| setattr(subscription, field_name, value) | |
| subscription.save() |
🤖 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 `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py` around
lines 63 - 86, The subscription creation logic in SnapshotSubscription mutation
is checking only SnapshotSubscription.objects.filter(user=user).exists(), which
blocks re-subscription after cancel and still allows a race before create().
Update the check to distinguish active vs inactive subscriptions (for example,
based on is_active in the SnapshotSubscription model) so canceled users can
subscribe again, and make the create path resilient to concurrent requests by
handling the one-to-one constraint failure around the subscription creation in
the mutation that builds the SnapshotSubscriptionResult.
| try: | ||
| subscription = SnapshotSubscription.objects.get(unsubscribe_token=token) | ||
| except (SnapshotSubscription.DoesNotExist, ValueError): | ||
| return SnapshotSubscriptionResult( | ||
| ok=False, | ||
| message="Invalid unsubscribe token.", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import os
import sys
import django
from django.conf import settings
# Minimal Django setup
if not settings.configured:
settings.configure(
DEBUG=True,
DATABASES={
'default': {
'ENGINE': 'django.db.backends.sqlite3',
'NAME': ':memory:',
}
},
INSTALLED_APPS=[
'django.contrib.contenttypes',
'django.contrib.auth',
],
USE_TZ=True,
SECRET_KEY='test-secret',
)
django.setup()
from uuid import UUID
from django.core.exceptions import ValidationError
from django.db import models
class TestModel(models.Model):
uuid_field = models.UUIDField()
class Meta:
app_label = 'test'
# Test the to_python method of UUIDField
field = TestModel._meta.get_field('uuid_field')
# Valid UUID
try:
result = field.to_python('550e8400-e29b-41d4-a716-446655440000')
print(f"Valid UUID parsed: {result}")
except Exception as e:
print(f"Valid UUID raised: {type(e).__name__}: {e}")
# Invalid UUID (malformed)
try:
result = field.to_python('not-a-uuid')
print(f"Invalid UUID parsed: {result}")
except Exception as e:
print(f"Invalid UUID raised: {type(e).__name__}: {e}")
# Test with query simulation
print("\n--- Testing actual ORM lookup simulation ---")
from django.db.models.lookups import Lookup
from django.db.models.sql.query import Query
# Try to see what happens when we try to create a lookup with invalid UUID
try:
# This simulates what Django does internally when you do .get(uuid_field='bad-uuid')
# The field's to_python is called before the query
converted = field.to_python('bad-uuid')
print(f"Converted: {converted}")
except Exception as e:
print(f"Conversion raised: {type(e).__name__}: {e}")
print(f"Is ValidationError? {isinstance(e, ValidationError)}")
PYRepository: OWASP/Nest
Length of output: 267
🏁 Script executed:
cat -n backend/apps/owasp/api/internal/mutations/snapshot_subscription.py | head -20Repository: OWASP/Nest
Length of output: 927
🏁 Script executed:
cat -n backend/apps/owasp/api/internal/mutations/snapshot_subscription.py | sed -n '160,180p'Repository: OWASP/Nest
Length of output: 1078
🏁 Script executed:
cat backend/requirements.txt backend/requirements-dev.txt 2>/dev/null | grep -i django || cat requirements.txt requirements-dev.txt 2>/dev/null | grep -i django || find . -name "pyproject.toml" -o -name "requirements*.txt" | head -5 | xargs grep -i django 2>/dev/null | head -10Repository: OWASP/Nest
Length of output: 891
🏁 Script executed:
python3 - <<'PY'
# Simulate Django UUIDField behavior without installing Django
# Based on Django source code: django/db/models/fields/__init__.py - UUIDField.to_python()
from uuid import UUID
from dataclasses import dataclass
from typing import Any
# Simulate Django's ValidationError
class ValidationError(Exception):
pass
def simulate_uuid_field_to_python(value: Any) -> UUID:
"""
Simulates Django's UUIDField.to_python() method behavior.
Source: https://github.com/django/django/blob/main/django/db/models/fields/__init__.py
"""
if value is None:
return None
if isinstance(value, UUID):
return value
try:
return UUID(str(value))
except (ValueError, AttributeError):
# Django raises ValidationError here if the value is not a valid UUID string
raise ValidationError("Invalid UUID format.")
# Test cases
test_cases = [
("550e8400-e29b-41d4-a716-446655440000", True), # Valid UUID
("not-a-uuid", False), # Invalid UUID
("", False), # Empty string
("123", False), # Too short
(None, True), # None should return None
]
print("Testing UUID field validation behavior:\n")
for value, should_pass in test_cases:
try:
result = simulate_uuid_field_to_python(value)
status = "PASS" if should_pass else "FAIL (should have raised)"
print(f"Value: {repr(value):30} -> Parsed: {result} [{status}]")
except ValidationError as e:
status = "PASS" if not should_pass else "FAIL (should have parsed)"
print(f"Value: {repr(value):30} -> ValidationError [{status}]")
except ValueError as e:
status = "FAIL (raises ValueError instead of ValidationError)"
print(f"Value: {repr(value):30} -> ValueError [{status}]")
except Exception as e:
print(f"Value: {repr(value):30} -> {type(e).__name__}: {e}")
print("\n" + "="*60)
print("Conclusion:")
print("Django's UUIDField.to_python() raises ValidationError for invalid UUIDs.")
print("The ORM's get() call will trigger this during field conversion.")
print("Therefore, catching ValidationError is necessary to prevent 500 errors.")
PYRepository: OWASP/Nest
Length of output: 826
🏁 Script executed:
find . -name "snapshot_subscription.py" -path "*/models/*" | head -1 | xargs cat -n | grep -A 5 -B 5 "unsubscribe_token"Repository: OWASP/Nest
Length of output: 495
Catch the ValidationError Django raises for invalid UUIDs.
An invalid UUID string passed to a UUIDField lookup is coerced by Django's ORM before the query runs. This coercion fails by raising ValidationError, not ValueError. Since the current exception handler only catches ValueError, a malformed unsubscribe token will propagate as an unhandled ValidationError, resulting in a 500 error.
Update the exception block to catch ValidationError:
from django.core.exceptions import ValidationError
try:
subscription = SnapshotSubscription.objects.get(unsubscribe_token=token)
except (SnapshotSubscription.DoesNotExist, ValidationError):
return SnapshotSubscriptionResult(
ok=False,
message="Invalid unsubscribe token.",
)🤖 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 `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py` around
lines 167 - 173, The unsubscribe-token lookup in SnapshotSubscription mutation
only handles SnapshotSubscription.DoesNotExist and ValueError, but invalid UUID
input raises Django’s ValidationError before the query completes. Update the
exception handling in the mutation that calls
SnapshotSubscription.objects.get(unsubscribe_token=token) to also catch
ValidationError, and make sure the needed import from django.core.exceptions is
added so malformed tokens return the existing invalid-token result instead of an
unhandled error.
| user = models.OneToOneField( | ||
| User, | ||
| on_delete=models.CASCADE, | ||
| related_name="snapshot_subscription", | ||
| ) | ||
| frequency = models.CharField( | ||
| max_length=10, | ||
| choices=Frequency.choices, | ||
| default=Frequency.WEEKLY, | ||
| ) | ||
| is_active = models.BooleanField(default=True) | ||
| unsubscribe_token = models.UUIDField( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether cancel/unsubscribe deletes SnapshotSubscription rows
# or only sets is_active=False without a reactivation path.
fd -i 'snapshot_subscription*.py' backend
echo
echo "=== Mutation / endpoint implementations touching SnapshotSubscription ==="
rg -n -C3 'SnapshotSubscription|is_active\s*=\s*False|delete\s*\(|unsubscribe_token|reactivat' \
backend/apps/owasp backend/tests/unit/apps/owasp
echo
echo "=== Focus on the known mutation file ==="
sed -n '1,240p' backend/apps/owasp/api/internal/mutations/snapshot_subscription.pyRepository: OWASP/Nest
Length of output: 50366
Canceled subscriptions cannot be recreated due to flawed existence check.
The current implementation permanently blocks users from re-subscribing after cancellation:
- Cancellation: Both
cancel_snapshot_subscriptionandunsubscribe_by_tokenonly setis_active = False. The record persists. - Creation:
create_snapshot_subscriptionchecksSnapshotSubscription.objects.filter(user=user).exists(). This returnsTrueeven if the existing record is inactive.
This design conflict prevents a user from toggling their subscription status after the initial creation.
Fix: Update the existence check in create_snapshot_subscription to only block active subscriptions:
if SnapshotSubscription.objects.filter(user=user, is_active=True).exists():🧰 Tools
🪛 ast-grep (0.44.0)
[info] 38-42: use help_text to document model columns
Context: models.CharField(
max_length=10,
choices=Frequency.choices,
default=Frequency.WEEKLY,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
🤖 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 `@backend/apps/owasp/models/snapshot_subscription.py` around lines 34 - 45, The
subscription creation flow is blocking re-subscribes because it treats inactive
records as existing subscriptions. Update the existence check in
create_snapshot_subscription to only consider active SnapshotSubscription rows
by filtering on user and is_active=True, while keeping the
cancellation/unsubscribe behavior in cancel_snapshot_subscription and
unsubscribe_by_token unchanged so users can recreate a subscription after
canceling it.
| def test_admin_configuration(self): | ||
| """Test admin configuration matches expected setup.""" | ||
| site = AdminSite() | ||
| admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site) | ||
|
|
||
| assert admin.list_display == ( | ||
| "user", | ||
| "frequency", | ||
| "is_active", | ||
| "created_at", | ||
| "updated_at", | ||
| ) | ||
| assert admin.list_filter == ("frequency", "is_active", "created_at") | ||
| assert admin.search_fields == ("user__email", "user__username") | ||
| assert admin.raw_id_fields == ("user",) | ||
| assert admin.readonly_fields == ("unsubscribe_token", "created_at", "updated_at") | ||
|
|
||
| # Check fieldsets structure | ||
| assert len(admin.fieldsets) == 3 | ||
|
|
||
| # Check Content Preferences fieldset | ||
| preferences_fieldset = admin.fieldsets[1] | ||
| assert preferences_fieldset[0] == "Content Preferences" | ||
|
|
||
| assert preferences_fieldset[1]["fields"] == ( | ||
| "include_chapters", | ||
| "include_events", | ||
| "include_issues", | ||
| "include_posts", | ||
| "include_projects", | ||
| "include_pull_requests", | ||
| "include_releases", | ||
| "include_users", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a test for actual admin-site registration.
This only validates SnapshotSubscriptionAdmin's static attributes after manual instantiation. It never checks that importing apps.owasp.admin registers SnapshotSubscription on admin.site, so a broken Line 19 import in backend/apps/owasp/admin/__init__.py or a removed Line 44 admin.site.register(...) would still pass.
Suggested coverage
+from django.contrib import admin
+
class TestSnapshotSubscriptionAdmin:
"""Test SnapshotSubscriptionAdmin configuration."""
+ def test_model_is_registered_on_default_admin_site(self):
+ """Test admin package wiring registers SnapshotSubscription."""
+ import apps.owasp.admin # noqa: F401
+
+ assert SnapshotSubscription in admin.site._registry
+ assert isinstance(
+ admin.site._registry[SnapshotSubscription],
+ SnapshotSubscriptionAdmin,
+ )
+
def test_admin_configuration(self):
"""Test admin configuration matches expected setup."""
site = AdminSite()
admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_admin_configuration(self): | |
| """Test admin configuration matches expected setup.""" | |
| site = AdminSite() | |
| admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site) | |
| assert admin.list_display == ( | |
| "user", | |
| "frequency", | |
| "is_active", | |
| "created_at", | |
| "updated_at", | |
| ) | |
| assert admin.list_filter == ("frequency", "is_active", "created_at") | |
| assert admin.search_fields == ("user__email", "user__username") | |
| assert admin.raw_id_fields == ("user",) | |
| assert admin.readonly_fields == ("unsubscribe_token", "created_at", "updated_at") | |
| # Check fieldsets structure | |
| assert len(admin.fieldsets) == 3 | |
| # Check Content Preferences fieldset | |
| preferences_fieldset = admin.fieldsets[1] | |
| assert preferences_fieldset[0] == "Content Preferences" | |
| assert preferences_fieldset[1]["fields"] == ( | |
| "include_chapters", | |
| "include_events", | |
| "include_issues", | |
| "include_posts", | |
| "include_projects", | |
| "include_pull_requests", | |
| "include_releases", | |
| "include_users", | |
| ) | |
| from django.contrib import admin | |
| class TestSnapshotSubscriptionAdmin: | |
| """Test SnapshotSubscriptionAdmin configuration.""" | |
| def test_model_is_registered_on_default_admin_site(self): | |
| """Test admin package wiring registers SnapshotSubscription.""" | |
| import apps.owasp.admin # noqa: F401 | |
| assert SnapshotSubscription in admin.site._registry | |
| assert isinstance( | |
| admin.site._registry[SnapshotSubscription], | |
| SnapshotSubscriptionAdmin, | |
| ) | |
| def test_admin_configuration(self): | |
| """Test admin configuration matches expected setup.""" | |
| site = AdminSite() | |
| admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site) | |
| assert admin.list_display == ( | |
| "user", | |
| "frequency", | |
| "is_active", | |
| "created_at", | |
| "updated_at", | |
| ) | |
| assert admin.list_filter == ("frequency", "is_active", "created_at") | |
| assert admin.search_fields == ("user__email", "user__username") | |
| assert admin.raw_id_fields == ("user",) | |
| assert admin.readonly_fields == ("unsubscribe_token", "created_at", "updated_at") | |
| # Check fieldsets structure | |
| assert len(admin.fieldsets) == 3 | |
| # Check Content Preferences fieldset | |
| preferences_fieldset = admin.fieldsets[1] | |
| assert preferences_fieldset[0] == "Content Preferences" | |
| assert preferences_fieldset[1]["fields"] == ( | |
| "include_chapters", | |
| "include_events", | |
| "include_issues", | |
| "include_posts", | |
| "include_projects", | |
| "include_pull_requests", | |
| "include_releases", | |
| "include_users", | |
| ) |
🤖 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 `@backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py` around
lines 12 - 45, The test only checks SnapshotSubscriptionAdmin directly and
misses whether apps.owasp.admin actually registers the model with admin.site.
Update the test in snapshot_subscription_test to import the admin package/module
that performs registration and assert SnapshotSubscription is present in
admin.site._registry, or that the registry entry is an instance of
SnapshotSubscriptionAdmin, so a broken import or missing admin.site.register
call is caught.
| def test_malformed_token(self, mutations): | ||
| """Test unsubscribe fails with malformed token.""" | ||
| with patch( | ||
| "apps.owasp.api.internal.mutations.snapshot_subscription.SnapshotSubscription.objects" | ||
| ) as mock_objects: | ||
| mock_objects.get.side_effect = ValueError | ||
| result = mutations.unsubscribe_by_token(token=MOCK_TOKEN) | ||
| assert not result.ok | ||
| assert result.message == "Invalid unsubscribe token." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find the actual mutation implementation
find backend -name "snapshot_subscription.py" -path "*/mutations/*" | head -5
# 2. Check what exceptions Django raises for malformed UUIDs
python3 << 'PY'
import sys
try:
from django.db import models
from uuid import UUID
# Check what exception Django UUIDField raises when parsing malformed UUID
field = models.UUIDField()
# Test malformed UUID string
try:
# Django field get_prep_value or from_db_value might raise
val = field.to_python("not-a-uuid")
print(f"to_python returned: {val} (type: {type(val).__name__})")
except Exception as e:
print(f"to_python raised: {type(e).__name__}: {e}")
try:
val = field.to_python("00000000-0000-0000-0000-00000000000X")
print(f"to_python returned for partial: {val} (type: {type(val).__name__})")
except Exception as e:
print(f"to_python raised for partial: {type(e).__name__}: {e}")
# Check field validation
try:
field.clean("not-a-uuid", None)
except Exception as e:
print(f"clean raised: {type(e).__name__}: {e}")
except ImportError as e:
print(f"Django not available in sandbox: {e}")
print("Will check Django docs via web search instead")
PY
# 3. Let's also check if there are any imports or usage of ValidationError in the project
grep -r "ValidationError" backend/apps/owasp/api/internal/mutations/snapshot_subscription.py 2>/dev/null || echo "No ValidationError found in mutation file"
# 4. Check test file for existing tests
grep -n "inactive" backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py 2>/dev/null || echo "No 'inactive' test found"
grep -n "resubscribe\|create.*after\|recreate" backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py 2>/dev/null || echo "No resubscribe tests found"Repository: OWASP/Nest
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Read the actual mutation implementation
cat -n backend/apps/owasp/api/internal/mutations/snapshot_subscription.py
# Check the test file context around line 187-195 and 197-207
sed -n '180,215p' backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py | cat -nRepository: OWASP/Nest
Length of output: 10089
🌐 Web query:
Django UUIDField malformed UUID exception type ValueError or ValidationError
💡 Result:
In Django, when a malformed UUID is provided to a UUIDField, the expected exception is a django.core.exceptions.ValidationError, not a standard Python ValueError [1][2][3]. Django's form-based UUIDField explicitly handles UUID conversion in its to_python method [1][2][3]. It attempts to create a uuid.UUID object from the input and catches the ValueError raised by the underlying Python uuid module, re-raising it as a django.core.exceptions.ValidationError [1][2][3]. Historically, this behavior caused issues in older versions of Django where certain components (such as specific widgets or querysets) expected a ValueError or did not account for the switch to ValidationError, leading to crashes [4][5][6]. Modern Django implementations consistently normalize these validation errors to ValidationError to ensure they can be caught by standard form validation and error-handling logic [1][2][3]. If you are encountering a ValueError instead of a ValidationError, it is often because you are interacting directly with lower-level database ORM fields or external libraries that are calling uuid.UUID directly without wrapping it in Django's validation layer [5][7]. For form-level inputs, you should always expect and catch django.core.exceptions.ValidationError [1][2].
Citations:
- 1: https://github.com/django/django/blob/fa7ffc6cb3b143fb8566cbf6a387d0f032377dc7/django/forms/fields.py
- 2: https://github.com/django/django/blob/stable/6.0.x/django/forms/fields.py
- 3: https://github.com/django/django/blob/main/django/forms/fields.py
- 4: https://code.djangoproject.com/ticket/29094
- 5: https://code.djangoproject.com/ticket/29206
- 6: https://code.djangoproject.com/ticket/27148
- 7: https://code.djangoproject.com/ticket/24319
🌐 Web query:
Django ORM UUIDField lookup malformed string raises exception
💡 Result:
In Django, when performing an ORM lookup on a UUIDField with a malformed string, the behavior regarding exceptions depends on the specific Django version and the nature of the database backend, but generally, it is designed to raise a validation-related exception [1][2]. Historically, passing an invalid string to a UUIDField lookup could result in different outcomes, such as a ValueError from the Python uuid module, a DataError from the database, or an unhandled exception depending on the context of the query execution [1][3]. Over several releases, Django has improved this behavior to ensure more consistent error handling [1][4][3]. Key points regarding this behavior: 1. Exception Type: When an invalid UUID string is provided to a filter or get call, Django's UUIDField often attempts to normalize or validate the input using its internal to_python method [5][2]. If this validation fails, it typically raises a django.core.exceptions.ValidationError [4][2]. In some internal contexts or specific lookup paths, this may still surface as a ValueError or, if it reaches the database layer without validation, a database-level error like DataError [1][3]. 2. Validation during Lookups: Django's ORM is not always required to pre-validate all input for every lookup type. However, for UUIDField, substantial efforts have been made to ensure that queries fail gracefully or predictably when provided with data that cannot be converted into a valid UUID [1][6]. 3. Development Context: If you are encountering an unexpected exception, it is often because the input string is not a valid hexadecimal UUID (with or without hyphens) [7][2]. If you need to perform fuzzy searching or partial matching (e.g., starts-with), keep in mind that standard lookups like exact and icontains have specific handling for UUID fields, and complex lookups sometimes require custom implementations or explicit string casting to avoid validation issues [8][9][10]. If you are writing code that accepts user-provided strings for UUID filtering, the recommended practice is to validate the string using the Python uuid.UUID class manually before passing it to the ORM query, or to wrap the query in a try-except block that catches ValidationError [2].
Citations:
- 1: https://code.djangoproject.com/ticket/24319
- 2: https://stackoverflow.com/questions/70032949/validationerror-django-q-query-is-not-a-valid-uuid
- 3: https://code.djangoproject.com/ticket/27148
- 4: https://code.djangoproject.com/ticket/29094
- 5: https://github.com/django/django/blob/da02cbd1effc951b14c981925d0e26a39566649e/django/forms/fields.py
- 6: https://code.djangoproject.com/ticket/24859
- 7: https://docs.djangoproject.com/en/6.0/ref/forms/fields/
- 8: http://code.djangoproject.com/ticket/29915
- 9: https://stackoverflow.com/questions/72101751/filtering-by-custom-uuidfield-got-broken-with-django-3-2-upgrade
- 10: https://stackoverflow.com/questions/55506739/how-can-i-filter-uuid-as-text-with-django
🏁 Script executed:
python3 << 'PY'
# Verify the exception type we expect in the test
from django.core.exceptions import ValidationError
print(f"ValidationError available: {hasattr(ValidationError, '__module__')}")
# The production code catches ValueError, which might be defensive but Django raises ValidationError.
# The test should reflect the real ORM behavior.
PYRepository: OWASP/Nest
Length of output: 267
Align test exception with Django ORM behavior and add subscription recreation test.
The test_malformed_token mocks ValueError, but Django’s UUIDField raises django.core.exceptions.ValidationError for malformed UUIDs in standard ORM lookups. Update the test to mock and assert ValidationError to reflect actual production behavior. Additionally, the create_snapshot_subscription logic currently blocks creation if any subscription exists (active or inactive). Add a test_create_after_cancel to verify if users can recreate a subscription after cancelling their previous one.
🤖 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
`@backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py`
around lines 187 - 195, Update test_malformed_token in
snapshot_subscription_test.py to match Django ORM behavior by mocking
django.core.exceptions.ValidationError instead of ValueError for
SnapshotSubscription.objects.get and keep the same unsubscribe_by_token
assertion. Also add a new test_create_after_cancel around
create_snapshot_subscription to verify a user can create a new subscription
after a previously cancelled/inactive one, so the test covers the intended
recreate flow rather than blocking on any existing record.
| def test_my_subscription_unauthenticated(self): | ||
| """Test mySubscription returns None for unauthenticated user.""" | ||
| info = mock_info(authenticated=False) | ||
| result = self.query.__class__.__dict__["my_subscription"](self.query, info=info) | ||
| assert result is None | ||
|
|
||
| def test_my_subscription_not_found(self): | ||
| """Test mySubscription returns None when no subscription exists.""" | ||
| info = mock_info() | ||
| with patch( | ||
| "apps.owasp.api.internal.queries.snapshot_subscription.SnapshotSubscription.objects" | ||
| ) as mock_objects: | ||
| mock_objects.get.side_effect = SnapshotSubscription.DoesNotExist | ||
| result = self.query.__class__.__dict__["my_subscription"](self.query, info=info) | ||
| assert result is None | ||
|
|
||
| def test_my_subscription_found(self): | ||
| """Test mySubscription returns subscription when it exists.""" | ||
| info = mock_info() | ||
| mock_sub = MagicMock(spec=SnapshotSubscription) | ||
| with patch( | ||
| "apps.owasp.api.internal.queries.snapshot_subscription.SnapshotSubscription.objects" | ||
| ) as mock_objects: | ||
| mock_objects.get.return_value = mock_sub | ||
| result = self.query.__class__.__dict__["my_subscription"](self.query, info=info) | ||
| assert result == mock_sub |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how strawberry_django field objects expose their resolver and whether they are callable.
fd -t f 'field.py' --full-path 'strawberry_django' 2>/dev/null | head
rg -nP --type=py -C3 '\b(def __call__|base_resolver|wrapped_func)\b' $(python -c "import strawberry_django, os; print(os.path.dirname(strawberry_django.__file__))" 2>/dev/null) 2>/dev/null | head -50
# Also check existing query tests in the repo for the established invocation pattern.
fd -e py -p 'queries' -p 'tests' | xargs rg -nP -l '__dict__\[' 2>/dev/null | headRepository: OWASP/Nest
Length of output: 148
Update test invocation to access the underlying resolver via base_resolver.
The tests invoke the resolver using self.query.__class__.__dict__["my_subscription"](self.query, info=info). Since @strawberry_django.field transforms the method into a StrawberryDjangoField descriptor, direct invocation of this descriptor does not execute the resolver logic.
To correctly unit test the resolver logic, access the underlying function via the base_resolver attribute:
# In snapshot_subscription_test.py
field = SnapshotSubscriptionQuery.__dict__["my_subscription"]
# Access the original function (func) from the base_resolver
result = field.base_resolver.func(self.query, info=info)This ensures the test executes the actual my_subscription implementation.
🤖 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
`@backend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.py`
around lines 34 - 59, The snapshot subscription tests are calling the Strawberry
Django field descriptor directly instead of the underlying resolver, so they
never execute the real my_subscription logic. Update the tests in
SnapshotSubscriptionQuery to fetch the my_subscription field from the class and
invoke its base_resolver.func with self.query and info, so the resolver
implementation is actually exercised for the authenticated, not found, and found
cases.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 19 files
Confidence score: 2/5
- In
backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, the create subscription path appears to block previously canceled users by only checking record existence, so users can get stuck permanently inactive via GraphQL after canceling — update the logic to allow reactivation (or check active state) before merging. - In
backend/apps/owasp/api/internal/mutations/snapshot_subscription.py,unsubscribe_by_tokendoes not catchValidationErrorfor malformed UUID tokens, which can bubble up as an unhandled GraphQL error instead of a controlled response — add UUID validation/error handling and return a safe client-facing error before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py">
<violation number="1" location="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py:63">
P1: Canceled users cannot resubscribe because create checks existence instead of active state/reactivation logic. This leaves users permanently inactive through GraphQL once they cancel.</violation>
<violation number="2" location="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py:169">
P2: `unsubscribe_by_token` misses `ValidationError` handling for malformed UUID tokens. Invalid token format can raise an unhandled exception and return a GraphQL error.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| """Create a new snapshot subscription for the logged-in user.""" | ||
| user = info.context.request.user | ||
|
|
||
| if SnapshotSubscription.objects.filter(user=user).exists(): |
There was a problem hiding this comment.
P1: Canceled users cannot resubscribe because create checks existence instead of active state/reactivation logic. This leaves users permanently inactive through GraphQL once they cancel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, line 63:
<comment>Canceled users cannot resubscribe because create checks existence instead of active state/reactivation logic. This leaves users permanently inactive through GraphQL once they cancel.</comment>
<file context>
@@ -0,0 +1,188 @@
+ """Create a new snapshot subscription for the logged-in user."""
+ user = info.context.request.user
+
+ if SnapshotSubscription.objects.filter(user=user).exists():
+ return SnapshotSubscriptionResult(
+ ok=False,
</file context>
| """Unsubscribe using a token from an email link. No auth required.""" | ||
| try: | ||
| subscription = SnapshotSubscription.objects.get(unsubscribe_token=token) | ||
| except (SnapshotSubscription.DoesNotExist, ValueError): |
There was a problem hiding this comment.
P2: unsubscribe_by_token misses ValidationError handling for malformed UUID tokens. Invalid token format can raise an unhandled exception and return a GraphQL error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, line 169:
<comment>`unsubscribe_by_token` misses `ValidationError` handling for malformed UUID tokens. Invalid token format can raise an unhandled exception and return a GraphQL error.</comment>
<file context>
@@ -0,0 +1,188 @@
+ """Unsubscribe using a token from an email link. No auth required."""
+ try:
+ subscription = SnapshotSubscription.objects.get(unsubscribe_token=token)
+ except (SnapshotSubscription.DoesNotExist, ValueError):
+ return SnapshotSubscriptionResult(
+ ok=False,
</file context>
HarshitVerma109
added
the
gsoc2026:harshitverma109
1 participant
Proposed change
Resolves #5056
Adds GraphQL mutations and query for managing snapshot digest subscriptions. Users can create, update, and cancel their subscription, and unsubscribe directly from email links using a token-based endpoint that requires no login.
Checklist