Fix/snapshot improvement #961
Conversation
|
@arkid15r I have created this PR, do let me know for any changes |
WalkthroughThis changeset enhances the Snapshot functionality across the project. In the Django admin, the Snapshot view now exposes additional fields for display and search. The Snapshot model is updated to include new attributes with a custom save behavior, accompanied by migration updates. New GraphQL types and queries have been added to enable snapshot retrieval and listing, and the API has been extended in the frontend. Finally, tests have been updated to validate the new attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Frontend
participant GraphQL_API
participant SnapshotQuery
participant SnapshotNode
participant Database
Client->>Frontend: Request snapshot details
Frontend->>GraphQL_API: Execute GET_SNAPSHOT_DETAILS query (with key)
GraphQL_API->>SnapshotQuery: Forward query for snapshot data
SnapshotQuery->>SnapshotNode: Invoke resolver (resolve_snapshot or resolve_recent_snapshots)
SnapshotNode->>Database: Query Snapshot model for data
Database-->>SnapshotNode: Return snapshot details
SnapshotNode-->>SnapshotQuery: Provide resolved data
SnapshotQuery-->>GraphQL_API: Send snapshot data
GraphQL_API-->>Frontend: Return GraphQL response
Frontend-->>Client: Display snapshot details
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/src/api/queries/snapshotQueries.ts (1)
3-40: Well-structured GraphQL query for snapshot details.The query is well-organized and includes all necessary fields including the new title and key attributes. The nested data for newReleases, newProjects, and newChapters is comprehensive.
Consider adding a comment describing the purpose of this query and potentially using GraphQL fragments if this structure will be reused elsewhere in the application.
import { gql } from '@apollo/client' +/** + * Query to fetch detailed information about a snapshot by its key. + * Returns the snapshot with its basic attributes and nested entities (releases, projects, chapters). + */ export const GET_SNAPSHOT_DETAILS = gql` query GetSnapshotDetails($key: String!) { snapshot(key: $key) {backend/apps/owasp/graphql/nodes/snapshot.py (1)
13-16: Consider defining constants in a central locationThese limits for recent items are well defined, but they might be better placed in a central configuration file or settings module if they need to be reused across multiple components.
backend/apps/owasp/models/snapshot.py (2)
38-42: Add validation for key formatWhile the save method automatically sets the key in YYYY-mm format, consider adding validation to ensure manual entries follow this format too.
def save(self, *args, **kwargs): """Automatically set the key in YYYY-mm format before saving.""" if not self.key: self.key = now().strftime("%Y-%m") + else: + # Validate key format (YYYY-mm) + import re + if not re.match(r"^\d{4}-\d{2}$", self.key): + raise ValueError("Key must be in YYYY-mm format") super().save(*args, **kwargs)
44-46: Update string representation to include new fieldsSince you've added title and key fields, consider updating the str method to include them for better identification.
def __str__(self): """Return a string representation of the snapshot.""" - return f"Snapshot {self.start_at} to {self.end_at} ({self.status})" + return f"Snapshot {self.key}: {self.title or 'Untitled'} ({self.status})"backend/apps/owasp/graphql/queries/snapshot.py (2)
23-29: Improve exception handlingConsider catching the specific DoesNotExist exception rather than a broad try-except block to avoid masking other potential exceptions.
def resolve_snapshot(root, info, key): """Resolve snapshot by key.""" try: return Snapshot.objects.get(key=key) - except Snapshot.DoesNotExist: + except Snapshot.DoesNotExist as e: + # Log the exception if needed + # logger.debug(f"Snapshot with key {key} not found: {e}") return None + # Let other exceptions propagate to be handled by the GraphQL error handler
30-32: Consider adding pagination for recent_snapshotsFor future scalability, consider implementing cursor-based pagination for the recent_snapshots query, especially if the number of snapshots grows significantly.
-recent_snapshots = graphene.List( - SnapshotNode, - limit=graphene.Int(default_value=8), -) +from graphene_django.filter import DjangoFilterConnectionField + +# Add connection to SnapshotNode class in nodes/snapshot.py +# class SnapshotNode(GenericEntityNode): +# class Meta: +# interfaces = (graphene.relay.Node,) + +recent_snapshots = DjangoFilterConnectionField( + SnapshotNode, + first=graphene.Int(default_value=8), + orderBy=graphene.String(default_value="-created_at") +) def resolve_recent_snapshots(root, info, limit): """Resolve recent snapshots.""" return Snapshot.objects.order_by("-created_at")[:limit]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/snapshot.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/snapshot.py(1 hunks)backend/apps/owasp/migrations/0015_snapshot.py(2 hunks)backend/apps/owasp/models/snapshot.py(3 hunks)backend/tests/owasp/models/snapshot_test.py(2 hunks)frontend/src/api/queries/snapshotQueries.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (15)
backend/tests/owasp/models/snapshot_test.py (2)
15-16: Good addition of test attributes for snapshot model.These new attributes align with the Snapshot model updates, providing necessary test data for the new fields.
33-36: Well-structured test for new attributes.This test method properly validates the new title and key attributes, with a clear docstring explaining its purpose. Good practice to explicitly test newly added model attributes.
backend/apps/owasp/admin.py (2)
122-122: Good UI enhancement for admin view.Adding the title field to list_display provides better visibility of snapshots in the admin interface.
136-137: Improved searchability for snapshot administration.Adding title and key to search_fields enhances admin usability by allowing administrators to quickly find snapshots by these new attributes.
backend/apps/owasp/graphql/queries/__init__.py (2)
6-6: Appropriate import of new SnapshotQuery.The import statement is correctly added to bring in the new query functionality.
10-10: Properly integrated SnapshotQuery into OwaspQuery.The inheritance hierarchy is correctly extended to include the new SnapshotQuery, making snapshot data available through the GraphQL API.
backend/apps/owasp/graphql/nodes/snapshot.py (3)
19-30: GraphQL node structure looks goodThe SnapshotNode class is well-structured with appropriate field definitions for exposing the Snapshot model data through GraphQL.
65-68:❓ Verification inconclusive
Ensure published_at field is indexed
Similar to the created_at index concern, verify that published_at is indexed for optimal performance when ordering releases.
🏁 Script executed:
#!/bin/bash # Check if indexes exist on the published_at field for releases echo "Checking for indexes on releases table..." python -c " from django.db import connection from apps.github.models.release import Release table_name = Release._meta.db_table # Check for existing indexes with connection.cursor() as cursor: cursor.execute(f\"\"\" SELECT indexname, indexdef FROM pg_indexes WHERE tablename = '{table_name}' \"\"\") indexes = cursor.fetchall() has_published_at_index = any('published_at' in idx[1] for idx in indexes) if has_published_at_index: print(f'Index on published_at exists for table {table_name}') else: print(f'No index found on published_at for table {table_name}') print('Consider adding an index with:') print(f'CREATE INDEX idx_{table_name}_published_at ON {table_name} (published_at);') "Length of output: 1014
Action: Verify that the published_at field is indexed
The automated check failed because Django isn’t available in the current environment, so manual verification in your Django setup is required. Please ensure that the releases table has an index on the published_at column. If no such index exists, consider adding one—for example:
- Location:
backend/apps/owasp/graphql/nodes/snapshot.py(used in ordering new releases)- Recommendation: Confirm that the database table for releases includes an index on the published_at field. If it does not, add an index using a command similar to:
CREATE INDEX idx_<table_name>_published_at ON <table_name> (published_at);
57-60:❓ Verification inconclusive
Verify query performance for large datasets
The ordering by created_at with a limit could potentially cause performance issues with large datasets. Consider adding database indexes on the created_at field if they don't already exist.
🏁 Script executed:
#!/bin/bash # Check if indexes exist on the created_at field echo "Checking for indexes on new_issues related table..." python -c " from django.db import connection from apps.owasp.models.snapshot import Snapshot # Get the related model for new_issues related_model = Snapshot.new_issues.field.model table_name = related_model._meta.db_table # Check for existing indexes with connection.cursor() as cursor: cursor.execute(f\"\"\" SELECT indexname, indexdef FROM pg_indexes WHERE tablename = '{table_name}' \"\"\") indexes = cursor.fetchall() has_created_at_index = any('created_at' in idx[1] for idx in indexes) if has_created_at_index: print(f'Index on created_at exists for table {table_name}') else: print(f'No index found on created_at for table {table_name}') print('Consider adding an index with:') print(f'CREATE INDEX idx_{table_name}_created_at ON {table_name} (created_at);') "Length of output: 1114
Action Required: Manually Verify Index on the
created_atFieldThe current implementation orders
new_issuesbycreated_atwith a limit, which can lead to performance issues on large datasets if the underlying column isn’t indexed. The automated check failed due to a missing Django module, so please verify manually that:
- The related model for
new_issuesdefinescreated_atwith an index (e.g., viadb_index=Trueor through a customized Meta index).- If the index is missing, consider adding one to improve query performance:
CREATE INDEX idx_<table_name>_created_at ON <table_name> (created_at);backend/apps/owasp/models/snapshot.py (2)
20-21: The new fields enhance model identificationAdding title and key fields improves the model's usability and identification capabilities.
40-41:❓ Verification inconclusive
Consider collision handling for identical keys
Since keys are generated based on the month (YYYY-mm), multiple snapshots created in the same month would cause uniqueness conflicts.
🏁 Script executed:
#!/bin/bash # Verify if there's a possibility of multiple snapshots per month python -c " from apps.owasp.models.snapshot import Snapshot from django.db.models import Count from django.db.models.functions import TruncMonth from django.utils.timezone import now import datetime # Check if there are any snapshots in the system snapshot_count = Snapshot.objects.count() print(f'Total snapshots in system: {snapshot_count}') # Check frequency of snapshot creation if snapshot_count > 0: # Get first and last snapshot first = Snapshot.objects.order_by('created_at').first() last = Snapshot.objects.order_by('-created_at').first() # Calculate months between months_between = (last.created_at.year - first.created_at.year) * 12 + \ (last.created_at.month - first.created_at.month) + 1 if months_between > 0: avg_per_month = snapshot_count / months_between print(f'Average snapshots per month: {avg_per_month:.2f}') if avg_per_month > 1: print('POTENTIAL ISSUE: You are averaging more than one snapshot per month.') print('Consider extending the key format to include day or sequential number.') else: print('All snapshots were created in the same month') # Check for any existing key conflicts today = now().strftime('%Y-%m') month_snapshots = Snapshot.objects.filter(key=today).count() print(f'Snapshots with current month key ({today}): {month_snapshots}') "Length of output: 1575
Action Required: Review Snapshot Key Collision Handling
It appears that the current verification script failed (ModuleNotFoundError) due to a module path issue. Please adjust the environment by setting the PYTHONPATH (e.g., to "backend") before running the verification. In the meantime, note that generating keys solely based on the month (YYYY-mm) could cause collisions if more than one snapshot is created in the same month. Consider extending the key format (by including the day or a sequential number) or implementing collision resolution to avoid key conflicts.
- Verify that your environment is correctly configured (e.g., using
PYTHONPATH=backend) so that the modules can be imported.- Manually test or re-run a modified verification script with the correct PYTHONPATH to assess if multiple snapshots per month occur.
- If collisions are detected, update the key-generation logic to ensure uniqueness.
backend/apps/owasp/graphql/queries/snapshot.py (1)
13-21: Well-structured GraphQL query fieldsThe snapshot and recent_snapshots fields are well defined with appropriate parameters.
backend/apps/owasp/migrations/0015_snapshot.py (3)
8-8: Dependency update looks goodThe update to the GitHub dependency from version 0015 to 0016 is correctly reflected.
22-25: New fields are properly definedThe new title, key, created_at, and updated_at fields are properly defined in the migration with appropriate constraints.
23-23:❓ Verification inconclusive
Verify database field constraint for key format
Since the key field has a specific format requirement (YYYY-mm), consider adding a database-level constraint to enforce this format.
🏁 Script executed:
#!/bin/bash # Check if PostgreSQL is being used and if so, suggest a check constraint echo "Checking database backend and suggesting constraint improvements..." python -c " from django.db import connection from django.conf import settings # Check if PostgreSQL is being used db_engine = settings.DATABASES['default']['ENGINE'] is_postgresql = 'postgresql' in db_engine if is_postgresql: print('PostgreSQL detected. You could add a check constraint:') print('ALTER TABLE owasp_snapshots ADD CONSTRAINT key_format_check CHECK (key ~ \'^\d{4}-\d{2}$\');') else: print(f'Database engine: {db_engine}') print('Format validation should be handled in application code only.') "Length of output: 842
Action: Verify and Enforce Key Format Constraint
The key field is intended to follow the "YYYY-mm" format. If your database backend is PostgreSQL, consider adding a check constraint to enforce this directly at the database level. For example, you could use the following SQL statement:
ALTER TABLE owasp_snapshots ADD CONSTRAINT key_format_check CHECK (key ~ '^\d{4}-\d{2}$');Note: The verification script we attempted encountered a
ModuleNotFoundError(missing Django), likely because it was run in an environment without Django installed. Please ensure that you run these tests in an environment where Django is available or manually verify your database engine via your Django settings. Once you confirm that PostgreSQL is used, implement the check constraint in a migration to enforce the correct key format.
fixes: #915
Summary by CodeRabbit
New Features
Tests