Add pause feature to backend #886
Conversation
WalkthroughThis pull request introduces a new Alembic migration script that adds pause functionality to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 7
🧹 Outside diff range and nitpick comments (5)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py (2)
30-31: Consider adding a composite index for query optimization.Since the PR objectives mention querying paused items with their timestamps, consider adding a composite index on (is_paused, paused_at) to optimize these queries.
Add this after the existing index:
# Add composite index for efficient querying of paused items by time op.create_index( op.f('ix_mediaitem_is_paused_paused_at'), 'MediaItem', ['is_paused', 'paused_at'] )
34-39: Update downgrade function if composite index is added.If you implement the suggested composite index, ensure it's dropped in the downgrade function.
Add this before dropping the existing index:
op.drop_index(op.f('ix_mediaitem_is_paused_paused_at'), table_name='MediaItem')src/program/media/item.py (1)
416-433: Consider addingPausedstate to handle paused itemsThe pause functionality is implemented, but the state determination logic (
_determine_state) doesn't consider the paused state. This could lead to inconsistencies in state reporting.Consider:
- Adding a
Pausedstate to theStatesenum- Updating
_determine_stateto return thePausedstate whenis_pausedis True- Ensuring that the state transitions handle the paused state appropriately
This would provide better visibility of paused items in the system and ensure consistent state reporting.
src/program/state_transition.py (1)
52-54: Consider refactoring duplicated filtering logicThe filtering logic for seasons and episodes is similar. To adhere to the DRY principle and improve maintainability, consider extracting this logic into a helper function.
Here's an example of how you might refactor the code:
+def filter_items(items): + return [ + item for item in items + if not item.is_paused + and item.last_state != States.Completed + and Scraping.should_submit(item) + ] if existing_item.type == "show": - items_to_submit = [s for s in existing_item.seasons - if not s.is_paused and s.last_state != States.Completed and Scraping.should_submit(s)] + items_to_submit = filter_items(existing_item.seasons) elif existing_item.type == "season": - items_to_submit = [e for e in existing_item.episodes - if not e.is_paused and e.last_state != States.Completed and Scraping.should_submit(e)] + items_to_submit = filter_items(existing_item.episodes)Also applies to: 56-58
src/routers/secure/items.py (1)
15-15: Confirm necessity of importingMediaTypeEnsure that
MediaTypeis required in this file. If it's not used, you can remove the import to clean up the code.If
MediaTypeis used in the code (which it appears to be), then this import is appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(3 hunks)src/program/state_transition.py(1 hunks)src/routers/secure/items.py(2 hunks)
🧰 Additional context used
🪛 Ruff
src/routers/secure/items.py
449-449: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
470-470: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
511-511: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
547-547: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (8)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py (1)
21-31: Verify transaction safety for the migration.
Multiple DDL operations are being performed. Ensure that the target database supports transactional DDL to maintain data consistency in case of migration failure.
✅ Verification successful
SQLite is used - transaction safety is guaranteed
The migration is safe as the application uses SQLite (configured as sqlite:////riven/data/media.db). SQLite automatically wraps DDL operations in transactions and provides full ACID compliance for schema changes. The migration will either complete fully or roll back completely on failure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the database configuration or environment variables indicate the database type
rg -l "DATABASE_URL|SQLALCHEMY_DATABASE_URI" --type ini --type env
Length of output: 96
Script:
#!/bin/bash
# Let's try different approaches to find database configuration
# 1. Look for common config files
fd -e yml -e yaml -e conf -e cfg -e env -e ini -e toml -e json
# 2. Search for database URL patterns in all files
rg -l "postgresql://|mysql://|sqlite://|oracle://"
# 3. Look for SQLAlchemy configuration
rg -l "SQLALCHEMY_DATABASE_URI|engine\s*=\s*create_engine"
# 4. Check alembic configuration
cat src/alembic/alembic.ini 2>/dev/null || true
Length of output: 794
Script:
#!/bin/bash
# Let's check the database configuration in docker-compose-dev.yml
cat docker-compose-dev.yml
# Also check if there are any other alembic migrations to understand the database setup pattern
fd . src/alembic/versions/ -e py
Length of output: 660
src/program/media/item.py (1)
264-265: LGTM!
The changes to to_dict() correctly expose the pause-related fields and handle the datetime conversion consistently with other similar fields.
src/program/state_transition.py (3)
20-24: LGTM: Correctly handles paused items by early return
The code appropriately checks if the existing_item is paused and skips further processing, ensuring that paused items are not processed further.
35-36: LGTM: Skips processing of paused seasons
The condition correctly skips seasons that are paused or in States.Completed or States.Unreleased, preventing unnecessary processing.
41-42: Verify consistency in handling 'Unreleased' state for episodes
In the condition for episodes, only States.Completed is checked. For seasons, both States.Completed and States.Unreleased are considered. Should episodes in States.Unreleased also be skipped to maintain consistency?
To confirm whether episodes can be in the States.Unreleased state, consider reviewing the possible states for episodes and adjust the condition if necessary.
src/routers/secure/items.py (3)
449-449: Confirm usage of Depends in default arguments aligns with FastAPI practices
The static analysis tool flagged the use of Depends in default argument values. In FastAPI, using Depends(get_db) in function parameters is the recommended way to inject dependencies.
Please verify that this use of Depends is appropriate in the FastAPI context.
Also applies to: 470-470, 491-491, 511-511, 547-547
🧰 Tools
🪛 Ruff
449-449: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
494-495: Check for potential AttributeError when accessing item properties
In get_pause_state, if item is None, attempting to access item.is_paused will raise an AttributeError. However, since you already handle the case where item is None, this should be safe.
446-489: LGTM!
The pause_items and unpause_items endpoints are well-structured and correctly implement the pause functionality.
🧰 Tools
🪛 Ruff
449-449: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
470-470: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| @@ -0,0 +1,39 @@ | |||
| """add_pause_functionality | |||
|
|
|||
| Revision ID: [generate a new revision ID] | |||
There was a problem hiding this comment.
Replace placeholder revision ID with an actual unique identifier.
The revision ID needs to be replaced with a valid unique identifier. Alembic typically uses a UUID-like string for this purpose.
Apply this change:
-Revision ID: [generate a new revision ID]
+Revision ID: c99239e3445fAnd:
-revision: str = '[generate a new revision ID]'
+revision: str = 'c99239e3445f'Also applies to: 15-15
There was a problem hiding this comment.
code rabbit has a point...
There was a problem hiding this comment.
@Gaisberg, glad you agree. Let me know if you need any further assistance.
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py
Outdated
Show resolved
Hide resolved
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| logger.debug(f"Pausing {self.id}") | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
|
|
||
| # Get the session and flush changes | ||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
| logger.info(f"{self.log_string} unpaused") | ||
|
|
There was a problem hiding this comment.
Improve transaction handling and logging consistency
Several issues need to be addressed in the pause/unpause methods:
- Inconsistent transaction handling:
pause()flushes butunpause()doesn't - Missing transaction management: changes could be lost if an error occurs
- Inconsistent logging:
pause()has both debug and info logs, whileunpause()only has info - No validation of current state before pausing/unpausing
Consider this improved implementation:
def pause(self) -> None:
"""Pause processing of this media item"""
+ if self.is_paused:
+ logger.debug(f"{self.log_string} is already paused")
+ return
+
logger.debug(f"Pausing {self.id}")
- self.is_paused = True
- self.paused_at = datetime.now()
- logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
-
- # Get the session and flush changes
- session = object_session(self)
- if session:
- session.flush()
+ try:
+ self.is_paused = True
+ self.paused_at = datetime.now()
+
+ session = object_session(self)
+ if session:
+ session.flush()
+
+ logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
+ except Exception as e:
+ logger.error(f"Failed to pause {self.log_string}: {str(e)}")
+ raise
def unpause(self) -> None:
"""Resume processing of this media item"""
- self.is_paused = False
- self.paused_at = None
- logger.info(f"{self.log_string} unpaused")
+ if not self.is_paused:
+ logger.debug(f"{self.log_string} is not paused")
+ return
+
+ logger.debug(f"Unpausing {self.id}")
+ try:
+ self.is_paused = False
+ self.paused_at = None
+
+ session = object_session(self)
+ if session:
+ session.flush()
+
+ logger.info(f"{self.log_string} unpaused")
+ except Exception as e:
+ logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
+ raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| logger.debug(f"Pausing {self.id}") | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| # Get the session and flush changes | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| self.is_paused = False | |
| self.paused_at = None | |
| logger.info(f"{self.log_string} unpaused") | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise |
There was a problem hiding this comment.
@Gaisberg, glad you agree!
src/routers/secure/items.py
Outdated
| # Build query using SQLAlchemy | ||
| query = select(func.count(MediaItem.id)).where(MediaItem.is_paused.is_(True)) | ||
|
|
||
| if type: | ||
| valid_types = [t.value for t in MediaType] | ||
| if type not in valid_types: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | ||
| ) | ||
| query = query.where(MediaItem.type == type) | ||
|
|
||
| logger.debug(f"Executing count query: {str(query)}") | ||
| count = db.scalar(query) or 0 # Default to 0 if None | ||
| logger.debug(f"Found {count} paused items") | ||
|
|
||
| return { | ||
| "total": count, | ||
| "type": type if type else "all" | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Error in get_paused_count: {str(e)}") | ||
| # Return 0 count on error | ||
| return { | ||
| "total": 0, | ||
| "type": type if type else "all", | ||
| "error": "Failed to get count" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching broad exceptions and handle specific exceptions instead
Catching the base Exception class can mask other unexpected errors and make debugging more difficult. It's better to catch specific exceptions that you anticipate might occur.
Apply this diff to catch specific SQLAlchemy exceptions:
try:
# Build query using SQLAlchemy
query = select(func.count(MediaItem.id)).where(MediaItem.is_paused.is_(True))
# ... existing code ...
count = db.scalar(query) or 0 # Default to 0 if None
logger.debug(f"Found {count} paused items")
return {
"total": count,
"type": type if type else "all"
}
- except Exception as e:
+ except SQLAlchemyError as e:
logger.error(f"Error in get_paused_count: {str(e)}")
# Return 0 count on error
return {
"total": 0,
"type": type if type else "all",
"error": "Failed to get count"
}Note: You'll need to import SQLAlchemyError from sqlalchemy.exc:
from sqlalchemy.exc import SQLAlchemyError
src/routers/secure/items.py
Outdated
| for item in items: | ||
| item.pause() | ||
| db.commit() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling for database commit operations
The db.commit() calls lack exception handling. If a commit fails, it may leave the session in an inconsistent state. It's important to handle exceptions and rollback transactions if necessary.
Consider wrapping the database operations in a try-except block:
try:
for item in items:
item.pause() # or item.unpause()
db.commit()
except Exception as e:
db.rollback()
logger.error(f"Failed to update items: {str(e)}")
raise HTTPException(status_code=500, detail="Failed to update items.")Also applies to: 481-484
src/routers/secure/items.py
Outdated
| sql = "SELECT id, title, type, is_paused, paused_at FROM \"MediaItem\" WHERE is_paused = true" | ||
| if type: | ||
| valid_types = [t.value for t in MediaType] | ||
| if type not in valid_types: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | ||
| ) | ||
| sql += f" AND type = '{type}'" | ||
|
|
There was a problem hiding this comment.
Use SQLAlchemy queries instead of raw SQL to prevent SQL injection
Concatenating user input directly into SQL statements can lead to SQL injection vulnerabilities. Even though there's input validation, it's safer and more maintainable to use SQLAlchemy's query builder methods.
Apply this diff to refactor the code using SQLAlchemy ORM:
- sql = "SELECT id, title, type, is_paused, paused_at FROM \"MediaItem\" WHERE is_paused = true"
- if type:
- valid_types = [t.value for t in MediaType]
- if type not in valid_types:
- raise HTTPException(
- status_code=400,
- detail=f"Invalid type. Must be one of: {', '.join(valid_types)}"
- )
- sql += f" AND type = '{type}'"
-
- logger.debug(f"Executing SQL: {sql}")
- result = db.execute(sql)
- rows = result.fetchall()
+ query = select(
+ MediaItem.id,
+ MediaItem.title,
+ MediaItem.type,
+ MediaItem.is_paused,
+ MediaItem.paused_at
+ ).where(MediaItem.is_paused.is_(True))
+
+ if type:
+ valid_types = [t.value for t in MediaType]
+ if type not in valid_types:
+ raise HTTPException(
+ status_code=400,
+ detail=f"Invalid type. Must be one of: {', '.join(valid_types)}"
+ )
+ query = query.where(MediaItem.type == type)
+
+ logger.debug(f"Executing query: {str(query)}")
+ result = db.execute(query)
+ rows = result.fetchall()Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@Gaisberg, glad to hear that you agree with refactoring to use SQLAlchemy queries. Let me know if you need any assistance with the implementation.
| @@ -0,0 +1,39 @@ | |||
| """add_pause_functionality | |||
|
|
|||
| Revision ID: [generate a new revision ID] | |||
There was a problem hiding this comment.
code rabbit has a point...
src/program/media/item.py
Outdated
| logger.debug(f"Pausing {self.id}") | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") |
There was a problem hiding this comment.
Clean up the log string, the latter attribute logs can be debug level if needed
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| logger.debug(f"Pausing {self.id}") | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
|
|
||
| # Get the session and flush changes | ||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
| logger.info(f"{self.log_string} unpaused") | ||
|
|
| @@ -17,45 +17,62 @@ def process_event(emitted_by: Service, existing_item: MediaItem | None = None, c | |||
| no_further_processing: ProcessedEvent = (None, []) | |||
| items_to_submit = [] | |||
|
|
|||
| #TODO - Reindex non-released badly indexed items here | |||
| # Skip processing if item is paused | |||
There was a problem hiding this comment.
This (lines: 20-25) is the only state_transition modification needed, remove the rest.
There was a problem hiding this comment.
This modification is still missing
There was a problem hiding this comment.
I think you misunderstood. Restore the original state_transition.py and just add the 3 line addition.
src/routers/secure/items.py
Outdated
| } | ||
|
|
||
| @router.post("/{ids}/pause", response_model=PauseResponse, operation_id="pause_items") |
There was a problem hiding this comment.
use query parameters if you want to supply multiple ids, path parameters should only provide one.
src/routers/secure/items.py
Outdated
| for item in items: | ||
| item.pause() | ||
| db.commit() | ||
|
|
src/routers/secure/items.py
Outdated
| sql = "SELECT id, title, type, is_paused, paused_at FROM \"MediaItem\" WHERE is_paused = true" | ||
| if type: | ||
| valid_types = [t.value for t in MediaType] | ||
| if type not in valid_types: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | ||
| ) | ||
| sql += f" AND type = '{type}'" | ||
|
|
src/routers/secure/items.py
Outdated
| # Build query using SQLAlchemy | ||
| query = select(func.count(MediaItem.id)).where(MediaItem.is_paused.is_(True)) | ||
|
|
||
| if type: | ||
| valid_types = [t.value for t in MediaType] | ||
| if type not in valid_types: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | ||
| ) | ||
| query = query.where(MediaItem.type == type) | ||
|
|
||
| logger.debug(f"Executing count query: {str(query)}") | ||
| count = db.scalar(query) or 0 # Default to 0 if None | ||
| logger.debug(f"Found {count} paused items") | ||
|
|
||
| return { | ||
| "total": count, | ||
| "type": type if type else "all" | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Error in get_paused_count: {str(e)}") | ||
| # Return 0 count on error | ||
| return { | ||
| "total": 0, | ||
| "type": type if type else "all", | ||
| "error": "Failed to get count" | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/routers/secure/items.py (1)
512-553: Consider adding pagination for large result sets.While the implementation is good, returning all paused items without pagination could cause performance issues if there are many items.
Consider adding pagination parameters similar to the existing
get_itemsendpoint:@router.get("/paused", response_model=AllPausedResponse) async def get_all_paused( request: Request, type: Optional[str] = None, limit: Optional[int] = 50, page: Optional[int] = 1, db: Session = Depends(get_db) ): # ... existing type validation ... query = query.offset((page - 1) * limit).limit(limit) total_count = db.scalar(select(func.count(MediaItem.id)).where(MediaItem.is_paused.is_(True))) return AllPausedResponse( count=total_count, items=items, page=page, limit=limit, total_pages=(total_count + limit - 1) // limit )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(5 hunks)src/routers/secure/items.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py
🧰 Additional context used
🪛 Ruff
src/program/media/item.py
417-417: SyntaxError: unindent does not match any outer indentation level
419-419: SyntaxError: unindent does not match any outer indentation level
423-423: SyntaxError: unindent does not match any outer indentation level
424-424: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
439-439: SyntaxError: unindent does not match any outer indentation level
443-443: SyntaxError: unindent does not match any outer indentation level
444-444: SyntaxError: Unexpected indentation
src/routers/secure/items.py
461-461: SyntaxError: Expected an indented block after try statement
464-464: SyntaxError: Expected a statement
464-464: SyntaxError: Expected a statement
464-465: SyntaxError: Expected an expression
465-465: SyntaxError: Unexpected indentation
474-474: SyntaxError: Expected a statement
🔇 Additional comments (7)
src/routers/secure/items.py (3)
47-63: LGTM! Well-structured response models.
The response models are well-documented and use appropriate types for their fields.
495-510: LGTM! Well-implemented pause state endpoint.
The endpoint properly handles the not found case and formats the datetime response correctly.
555-590: LGTM! Well-implemented count endpoint.
The endpoint has proper error handling and type validation. Good practice returning 0 with an error message on failure instead of throwing an exception.
src/program/media/item.py (4)
80-82: LGTM: Column definitions follow best practices
The new columns are properly defined with appropriate types and constraints.
93-93: Index addition previously reviewed
265-266: LGTM: Dictionary representation properly handles new fields
The implementation correctly handles type conversion and null cases.
417-456: LGTM: Robust implementation of pause/unpause functionality
The implementation includes:
- State validation before operations
- Proper transaction handling with session.flush()
- Comprehensive error handling
- Appropriate logging at debug/info levels
🧰 Tools
🪛 Ruff
417-417: SyntaxError: unindent does not match any outer indentation level
419-419: SyntaxError: unindent does not match any outer indentation level
423-423: SyntaxError: unindent does not match any outer indentation level
424-424: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
439-439: SyntaxError: unindent does not match any outer indentation level
443-443: SyntaxError: unindent does not match any outer indentation level
444-444: SyntaxError: Unexpected indentation
src/routers/secure/items.py
Outdated
| for item in items: | ||
| item.pause() # or item.unpause() | ||
| db.commit() | ||
| except Exception as e: | ||
| db.rollback() | ||
| logger.error(f"Failed to update items: {str(e)}") | ||
| raise HTTPException(status_code=500, detail="Failed to update items.") |
There was a problem hiding this comment.
Fix indentation and improve error handling.
The try-except block has incorrect indentation and should be more specific about the exceptions it catches.
Apply this diff:
try:
- for item in items:
- item.pause() # or item.unpause()
- db.commit()
-except Exception as e:
- db.rollback()
- logger.error(f"Failed to update items: {str(e)}")
- raise HTTPException(status_code=500, detail="Failed to update items.")
+ for item in items:
+ item.pause()
+ db.commit()
+ except SQLAlchemyError as e:
+ db.rollback()
+ logger.error(f"Failed to pause items: {str(e)}")
+ raise HTTPException(status_code=500, detail="Failed to pause items")📝 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.
| try: | |
| for item in items: | |
| item.pause() # or item.unpause() | |
| db.commit() | |
| except Exception as e: | |
| db.rollback() | |
| logger.error(f"Failed to update items: {str(e)}") | |
| raise HTTPException(status_code=500, detail="Failed to update items.") | |
| try: | |
| for item in items: | |
| item.pause() | |
| db.commit() | |
| except SQLAlchemyError as e: | |
| db.rollback() | |
| logger.error(f"Failed to pause items: {str(e)}") | |
| raise HTTPException(status_code=500, detail="Failed to pause items") |
🧰 Tools
🪛 Ruff
461-461: SyntaxError: Expected an indented block after try statement
464-464: SyntaxError: Expected a statement
464-464: SyntaxError: Expected a statement
464-465: SyntaxError: Expected an expression
465-465: SyntaxError: Unexpected indentation
src/routers/secure/items.py
Outdated
| } | ||
|
|
||
| @router.post("/{ids}/pause", response_model=PauseResponse, operation_id="pause_items") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use query parameters for multiple IDs.
The path parameter should be used for single ID operations. For multiple IDs, use query parameters instead.
Apply this diff:
-@router.post("/{ids}/pause", response_model=PauseResponse, operation_id="pause_items")
+@router.post("/{id}/pause", response_model=PauseResponse, operation_id="pause_item")
+@router.post("/pause", response_model=PauseResponse, operation_id="pause_items")Committable suggestion skipped: line range outside the PR's diff.
src/routers/secure/items.py
Outdated
| async def unpause_items(request: Request, ids: str = None, db: Session = Depends(get_db)): | ||
| """Unpause media items to resume processing""" | ||
| item_ids = handle_ids(ids) | ||
|
|
||
| items = db.execute( | ||
| select(MediaItem).where(MediaItem.id.in_(item_ids)) | ||
| ).unique().scalars().all() | ||
|
|
||
| if not items: | ||
| raise HTTPException(status_code=404, detail="No items found") | ||
|
|
||
| for item in items: | ||
| item.unpause() | ||
| db.commit() | ||
|
|
||
| return { | ||
| "message": f"Successfully unpaused {len(items)} items", | ||
| "ids": item_ids | ||
| } |
There was a problem hiding this comment.
Add error handling and use query parameters.
Similar to the pause endpoint, this endpoint needs proper error handling and should use query parameters for multiple IDs.
Apply this diff:
-@router.post("/{ids}/unpause", response_model=PauseResponse, operation_id="unpause_items")
+@router.post("/{id}/unpause", response_model=PauseResponse, operation_id="unpause_item")
+@router.post("/unpause", response_model=PauseResponse, operation_id="unpause_items")
async def unpause_items(request: Request, ids: str = None, db: Session = Depends(get_db)):
"""Unpause media items to resume processing"""
item_ids = handle_ids(ids)
items = db.execute(
select(MediaItem).where(MediaItem.id.in_(item_ids))
).unique().scalars().all()
if not items:
raise HTTPException(status_code=404, detail="No items found")
+ try:
for item in items:
item.unpause()
- db.commit()
+ db.commit()
+ except SQLAlchemyError as e:
+ db.rollback()
+ logger.error(f"Failed to unpause items: {str(e)}")
+ raise HTTPException(status_code=500, detail="Failed to unpause items")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
474-474: SyntaxError: Expected a statement
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| if self.is_paused: | ||
| logger.debug(f"{self.log_string} is already paused") | ||
| return | ||
|
|
||
| logger.debug(f"Pausing {self.id}") | ||
| try: | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
||
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
| if not self.is_paused: | ||
| logger.debug(f"{self.log_string} is not paused") | ||
| return | ||
|
|
||
| logger.debug(f"Unpausing {self.id}") | ||
| try: | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} unpaused") | ||
| except Exception as e: | ||
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
There was a problem hiding this comment.
Fix indentation issues in pause/unpause methods
The methods have inconsistent indentation that needs to be fixed to match the class level.
Apply this diff to fix the indentation:
def pause(self) -> None:
"""Pause processing of this media item"""
- if self.is_paused:
- logger.debug(f"{self.log_string} is already paused")
- return
+ if self.is_paused:
+ logger.debug(f"{self.log_string} is already paused")
+ return
logger.debug(f"Pausing {self.id}")
- try:
- self.is_paused = True
- self.paused_at = datetime.now()
+ try:
+ self.is_paused = True
+ self.paused_at = datetime.now()
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
- except Exception as e:
- logger.error(f"Failed to pause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
+ except Exception as e:
+ logger.error(f"Failed to pause {self.log_string}: {str(e)}")
+ raise
def unpause(self) -> None:
"""Resume processing of this media item"""
- if not self.is_paused:
- logger.debug(f"{self.log_string} is not paused")
- return
+ if not self.is_paused:
+ logger.debug(f"{self.log_string} is not paused")
+ return
logger.debug(f"Unpausing {self.id}")
- try:
- self.is_paused = False
- self.paused_at = None
+ try:
+ self.is_paused = False
+ self.paused_at = None
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} unpaused")
- except Exception as e:
- logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} unpaused")
+ except Exception as e:
+ logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
+ raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise |
🧰 Tools
🪛 Ruff
417-417: SyntaxError: unindent does not match any outer indentation level
419-419: SyntaxError: unindent does not match any outer indentation level
423-423: SyntaxError: unindent does not match any outer indentation level
424-424: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
439-439: SyntaxError: unindent does not match any outer indentation level
443-443: SyntaxError: unindent does not match any outer indentation level
444-444: SyntaxError: Unexpected indentation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/routers/secure/items.py (2)
518-559: Consider adding pagination for large datasets.While the current implementation works well, consider adding pagination to handle large numbers of paused items efficiently. This would be consistent with the existing
get_itemsendpoint that already implements pagination.🧰 Tools
🪛 Ruff
522-522: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
561-596: Consider implementing caching for count queries.For better performance, especially with large datasets, consider implementing caching for the count results with a short TTL (e.g., 5 minutes). This would reduce database load for frequently accessed count data.
🧰 Tools
🪛 Ruff
565-565: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
589-589: Undefined name
SQLAlchemyError(F821)
src/program/media/item.py (1)
417-456: Consider refactoring pause/unpause methods to reduce code duplication.The error handling and session management logic is duplicated between the methods.
Consider extracting the common logic into a helper method:
def _update_pause_state(self, pause: bool) -> None: """Helper method to update pause state with proper error handling.""" try: self.is_paused = pause self.paused_at = datetime.now() if pause else None session = object_session(self) if session: session.flush() logger.info(f"{self.log_string} {'paused' if pause else 'unpaused'}") except Exception as e: logger.error(f"Failed to {'pause' if pause else 'unpause'} {self.log_string}: {str(e)}") raise def pause(self) -> None: """Pause processing of this media item. Raises: Exception: If pausing fails due to database or other errors. """ if self.is_paused: logger.debug(f"{self.log_string} is already paused") return logger.debug(f"Pausing {self.id}") self._update_pause_state(True) def unpause(self) -> None: """Resume processing of this media item. Raises: Exception: If unpausing fails due to database or other errors. """ if not self.is_paused: logger.debug(f"{self.log_string} is not paused") return logger.debug(f"Unpausing {self.id}") self._update_pause_state(False)🧰 Tools
🪛 Ruff
419-419: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
419-419: SyntaxError: Simple statements must be separated by newlines or semicolons
419-420: SyntaxError: Expected an expression
421-421: SyntaxError: Expected an identifier, but found a keyword 'return' that cannot be used here
423-423: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/program/media/item.py(5 hunks)src/routers/secure/items.py(2 hunks)
🧰 Additional context used
🪛 Ruff
src/program/media/item.py
419-419: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
419-419: SyntaxError: Simple statements must be separated by newlines or semicolons
419-420: SyntaxError: Expected an expression
421-421: SyntaxError: Expected an identifier, but found a keyword 'return' that cannot be used here
423-423: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
src/routers/secure/items.py
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
🔇 Additional comments (5)
src/routers/secure/items.py (3)
23-63: LGTM! Well-structured response models.
The response models are properly defined using Pydantic BaseModel with appropriate type hints and documentation.
501-516: LGTM! Well-implemented pause state check.
The endpoint properly handles item lookup and returns appropriate response with comprehensive details.
🧰 Tools
🪛 Ruff
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
448-499: Verify pause state handling in event processing.
Ensure that the event processing system properly respects the pause state of items.
✅ Verification successful
Let me gather more specific information about how the pause state is handled.
Pause state is properly handled in event processing system
The verification shows that pause functionality is well-integrated:
- State transition system explicitly checks
is_pausedand skips processing of paused items - Pause state is properly cascaded in show/season hierarchies, with paused items filtered out
- MediaItem class has proper database schema support with indexed columns and complete pause/unpause implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if event processing code handles paused items
# Search for pause state checks in event processing
rg -l "is_paused" --type py
ast-grep --pattern 'if $item.is_paused:
$$$
'
Length of output: 238
Script:
#!/bin/bash
# Check implementation details of pause handling
# Check state transition logic
rg -A 10 "is_paused" src/program/state_transition.py
# Check MediaItem class implementation
rg -A 5 "is_paused" src/program/media/item.py
# Look for event processing logic
ast-grep --pattern 'def process_$event($$$) {
$$$
}'
Length of output: 4316
🧰 Tools
🪛 Ruff
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/program/media/item.py (2)
80-82: LGTM! Well-structured pause-related fields and index.
The implementation includes:
- Appropriate column types with correct nullability
- Performance-optimizing index on
is_paused - Clear documentation
Also applies to: 93-93
265-266: LGTM! Consistent dictionary representation of pause fields.
The implementation follows the established pattern for datetime fields and includes proper null handling.
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| + if self.is_paused: | ||
| + logger.debug(f"{self.log_string} is already paused") | ||
| + return | ||
|
|
||
| logger.debug(f"Pausing {self.id}") | ||
| try: | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
||
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
|
|
||
| if not self.is_paused: | ||
| logger.debug(f"{self.log_string} is not paused") | ||
| return | ||
|
|
||
| logger.debug(f"Unpausing {self.id}") | ||
| try: | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} unpaused") | ||
| except Exception as e: | ||
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
Fix indentation issues in pause/unpause methods.
The methods have inconsistent indentation that needs to be fixed.
Apply this diff to fix the indentation:
def pause(self) -> None:
"""Pause processing of this media item"""
- if self.is_paused:
- logger.debug(f"{self.log_string} is already paused")
- return
+ if self.is_paused:
+ logger.debug(f"{self.log_string} is already paused")
+ return
- logger.debug(f"Pausing {self.id}")
- try:
- self.is_paused = True
- self.paused_at = datetime.now()
+ logger.debug(f"Pausing {self.id}")
+ try:
+ self.is_paused = True
+ self.paused_at = datetime.now()
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
- except Exception as e:
- logger.error(f"Failed to pause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
+ except Exception as e:
+ logger.error(f"Failed to pause {self.log_string}: {str(e)}")
+ raise
def unpause(self) -> None:
"""Resume processing of this media item"""
-
- if not self.is_paused:
- logger.debug(f"{self.log_string} is not paused")
- return
+ if not self.is_paused:
+ logger.debug(f"{self.log_string} is not paused")
+ return
- logger.debug(f"Unpausing {self.id}")
- try:
- self.is_paused = False
- self.paused_at = None
+ logger.debug(f"Unpausing {self.id}")
+ try:
+ self.is_paused = False
+ self.paused_at = None
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} unpaused")
- except Exception as e:
- logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} unpaused")
+ except Exception as e:
+ logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
+ raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| + if self.is_paused: | |
| + logger.debug(f"{self.log_string} is already paused") | |
| + return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise |
🧰 Tools
🪛 Ruff
419-419: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
419-419: SyntaxError: Simple statements must be separated by newlines or semicolons
419-420: SyntaxError: Expected an expression
421-421: SyntaxError: Expected an identifier, but found a keyword 'return' that cannot be used here
423-423: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/program/media/item.py (1)
417-456: Consider refactoring pause/unpause methods to reduce duplication.The pause and unpause methods share similar patterns for session handling and error handling. Consider extracting common functionality into a helper method.
Here's a suggested refactoring:
def _update_pause_state(self, pause: bool) -> None: """Helper method to update pause state with proper error handling.""" action = "pause" if pause else "unpause" if pause == self.is_paused: logger.debug(f"{self.log_string} is already {'paused' if pause else 'not paused'}") return logger.debug(f"{action.capitalize()}ing {self.id}") try: self.is_paused = pause self.paused_at = datetime.now() if pause else None session = object_session(self) if session: session.flush() logger.info(f"{self.log_string} {action}d") except Exception as e: logger.error(f"Failed to {action} {self.log_string}: {str(e)}") raise def pause(self) -> None: """Pause processing of this media item""" self._update_pause_state(True) def unpause(self) -> None: """Resume processing of this media item""" self._update_pause_state(False)🧰 Tools
🪛 Ruff
419-419: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/program/media/item.py(5 hunks)
🧰 Additional context used
🪛 Ruff
src/program/media/item.py
419-419: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
🔇 Additional comments (3)
src/program/media/item.py (3)
80-82: LGTM: Well-structured column definitions.
The new pause-related columns are properly defined with appropriate types, nullability, and defaults.
93-93: LGTM: Index properly added.
The requested index for the is_paused column has been added as suggested.
265-266: LGTM: Consistent dictionary representation.
The pause-related fields are properly added to the dictionary representation, with consistent datetime handling.
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| if self.is_paused: | ||
| logger.debug(f"{self.log_string} is already paused") | ||
| return | ||
|
|
||
| logger.debug(f"Pausing {self.id}") | ||
| try: | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
||
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
|
|
||
| if not self.is_paused: | ||
| logger.debug(f"{self.log_string} is not paused") | ||
| return | ||
|
|
||
| logger.debug(f"Unpausing {self.id}") | ||
| try: | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} unpaused") | ||
| except Exception as e: | ||
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
Fix indentation in pause/unpause methods.
The methods have inconsistent indentation that needs to be fixed.
Apply this diff to fix the indentation:
def pause(self) -> None:
"""Pause processing of this media item"""
- if self.is_paused:
- logger.debug(f"{self.log_string} is already paused")
- return
+ if self.is_paused:
+ logger.debug(f"{self.log_string} is already paused")
+ return
- logger.debug(f"Pausing {self.id}")
- try:
- self.is_paused = True
- self.paused_at = datetime.now()
+ logger.debug(f"Pausing {self.id}")
+ try:
+ self.is_paused = True
+ self.paused_at = datetime.now()
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
- except Exception as e:
- logger.error(f"Failed to pause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
+ except Exception as e:
+ logger.error(f"Failed to pause {self.log_string}: {str(e)}")
+ raise
def unpause(self) -> None:
"""Resume processing of this media item"""
-
- if not self.is_paused:
- logger.debug(f"{self.log_string} is not paused")
- return
+ if not self.is_paused:
+ logger.debug(f"{self.log_string} is not paused")
+ return
- logger.debug(f"Unpausing {self.id}")
- try:
- self.is_paused = False
- self.paused_at = None
+ logger.debug(f"Unpausing {self.id}")
+ try:
+ self.is_paused = False
+ self.paused_at = None
- session = object_session(self)
- if session:
- session.flush()
+ session = object_session(self)
+ if session:
+ session.flush()
- logger.info(f"{self.log_string} unpaused")
- except Exception as e:
- logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
- raise
+ logger.info(f"{self.log_string} unpaused")
+ except Exception as e:
+ logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
+ raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise |
🧰 Tools
🪛 Ruff
419-419: SyntaxError: Unexpected indentation
437-437: SyntaxError: unindent does not match any outer indentation level
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/program/media/item.py (1)
265-266: Consider consistent datetime string conversion.The
paused_atconversion could be made consistent with other datetime fields in the dictionary (e.g.,requested_at,scraped_at).- "paused_at": str(self.paused_at) if self.paused_at else None, + "paused_at": str(self.paused_at) if self.paused_at else None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/program/media/item.py(5 hunks)
🧰 Additional context used
🪛 Ruff
src/program/media/item.py
444-444: SyntaxError: unindent does not match any outer indentation level
449-449: SyntaxError: unindent does not match any outer indentation level
450-450: SyntaxError: Unexpected indentation
🔇 Additional comments (2)
src/program/media/item.py (2)
80-82: LGTM: Well-defined pause-related attributes.
The new attributes are properly typed and have appropriate default values.
93-93: Index addition already reviewed.
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
|
|
||
| if self.is_paused: | ||
| logger.debug(f"{self.log_string} is already paused") | ||
| return | ||
|
|
||
|
|
||
| logger.debug(f"Pausing {self.id}") | ||
| try: | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
|
|
||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
|
|
||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
There was a problem hiding this comment.
Fix indentation in pause method.
The method has inconsistent indentation that should be fixed.
def pause(self) -> None:
"""Pause processing of this media item"""
-
if self.is_paused:
logger.debug(f"{self.log_string} is already paused")
return
-
logger.debug(f"Pausing {self.id}")
try:
self.is_paused = True
self.paused_at = datetime.now()
-
session = object_session(self)
if session:
session.flush()
-
logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
except Exception as e:
logger.error(f"Failed to pause {self.log_string}: {str(e)}")
raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise |
src/program/media/item.py
Outdated
| def unpause(self) -> None: | ||
| """Resume processing of this media item""" | ||
|
|
||
| if not self.is_paused: | ||
| logger.debug(f"{self.log_string} is not paused") | ||
| return | ||
|
|
||
|
|
||
| logger.debug(f"Unpausing {self.id}") | ||
| try: | ||
| self.is_paused = False | ||
| self.paused_at = None | ||
|
|
||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
|
|
||
| logger.info(f"{self.log_string} unpaused") | ||
| except Exception as e: | ||
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | ||
| raise | ||
|
|
There was a problem hiding this comment.
Fix indentation and improve logging in unpause method.
The method has indentation issues and inconsistent logging compared to the pause method.
def unpause(self) -> None:
- """Resume processing of this media item"""
+ """Resume processing of this media item"""
if not self.is_paused:
logger.debug(f"{self.log_string} is not paused")
return
-
logger.debug(f"Unpausing {self.id}")
try:
self.is_paused = False
self.paused_at = None
-
session = object_session(self)
if session:
session.flush()
-
- logger.info(f"{self.log_string} unpaused")
+ logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, paused_at={self.paused_at}")
except Exception as e:
logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
raise📝 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 unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise | |
| def unpause(self) -> None: | |
| """Resume processing of this media item""" | |
| if not self.is_paused: | |
| logger.debug(f"{self.log_string} is not paused") | |
| return | |
| logger.debug(f"Unpausing {self.id}") | |
| try: | |
| self.is_paused = False | |
| self.paused_at = None | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to unpause {self.log_string}: {str(e)}") | |
| raise |
🧰 Tools
🪛 Ruff
444-444: SyntaxError: unindent does not match any outer indentation level
449-449: SyntaxError: unindent does not match any outer indentation level
450-450: SyntaxError: Unexpected indentation
| @@ -17,45 +17,62 @@ def process_event(emitted_by: Service, existing_item: MediaItem | None = None, c | |||
| no_further_processing: ProcessedEvent = (None, []) | |||
| items_to_submit = [] | |||
|
|
|||
| #TODO - Reindex non-released badly indexed items here | |||
| # Skip processing if item is paused | |||
There was a problem hiding this comment.
This modification is still missing
|
@Gaisberg i have removed the rest in state transition, please let me know if anything else need to be changed |
Please squash and amend the commit message to follow conventional commits |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/routers/secure/items.py (2)
501-516: Optimize database query and fix dependency injection.The endpoint is well-structured but could be improved:
- Fix dependency injection pattern
- Select only required fields in the database query
Apply this diff:
@router.get("/{id}/pause", response_model=PauseStateResponse, operation_id="get_pause_state") -async def get_pause_state(request: Request, id: str, db: Session = Depends(get_db)): +async def get_pause_state(request: Request, id: str): + db = next(get_db()) """Check if a media item is paused""" item = db.execute( - select(MediaItem).where(MediaItem.id == id) + select( + MediaItem.id, + MediaItem.title, + MediaItem.is_paused, + MediaItem.paused_at + ).where(MediaItem.id == id) ).unique().scalar_one_or_none()🧰 Tools
🪛 Ruff
502-502: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
518-559: Optimize query performance in get_all_paused endpoint.The implementation is good but could be improved:
- Add pagination to prevent potential memory issues with large result sets
- Consider adding sorting options
Would you like me to provide an implementation with pagination and sorting support?
🧰 Tools
🪛 Ruff
522-522: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(5 hunks)src/program/state_transition.py(1 hunks)src/routers/secure/items.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py
- src/program/media/item.py
- src/program/state_transition.py
🧰 Additional context used
🪛 Ruff
src/routers/secure/items.py
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
🔇 Additional comments (3)
src/routers/secure/items.py (3)
23-63: LGTM! Well-structured response models.
The response models are well-defined with proper type hints and clear docstrings.
561-596: LGTM! Well-implemented count endpoint.
The implementation includes:
- Proper error handling with SQLAlchemyError
- Type validation
- Clear logging
- Sensible defaults
🧰 Tools
🪛 Ruff
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
448-499: Verify integration with state transition system.
The pause functionality may impact the state transition system. Let's verify the integration:
✅ Verification successful
Pause functionality is safely integrated with state transition system
Based on the codebase analysis:
-
The pause functionality is properly isolated from the state transition system:
is_pausedandpaused_atare separate fields from the state machine- State transitions are skipped for paused items (src/program/state_transition.py:
if existing_item and existing_item.is_paused:) - Pause/unpause operations don't interfere with the item's state (
last_stateremains unchanged)
-
The implementation includes proper safeguards:
- Database schema has an index on
is_pausedfor efficient queries - Pause operations are wrapped in try-catch blocks with proper rollback
- State transitions are preserved when items are unpaused
- Database schema has an index on
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for state transition handling of paused items
# Search for state transition logic handling paused items
ast-grep --pattern 'if $item.is_paused:
$$$
'
# Search for potential race conditions in state transitions
rg -A 5 'process_event.*is_paused'
Length of output: 86
Script:
#!/bin/bash
# Let's check the MediaItem model and state transition logic
# Find MediaItem class definition and its state-related methods
ast-grep --pattern 'class MediaItem($$$):
$$$
pause($$$) {
$$$
}
$$$
'
# Look for state transition handling
rg -l "state.*transition|process.*state|state.*machine"
# Check for pause-related state fields
rg -A 3 "is_paused|paused_at|state"
# Look for event processing related to pause
rg -A 5 "def process|handle.*event"
Length of output: 55351
🧰 Tools
🪛 Ruff
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/routers/secure/items.py (1)
561-596: LGTM! Well-implemented error handling.The get_paused_count endpoint has proper error handling and uses SQLAlchemy's query builder correctly. Just fix the dependency injection pattern.
Apply this small diff:
-async def get_paused_count(request: Request, type: Optional[str] = None, db: Session = Depends(get_db)): +async def get_paused_count(request: Request, type: Optional[str] = None): + db = next(get_db())🧰 Tools
🪛 Ruff
565-565: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
589-589: Undefined name
SQLAlchemyError(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(5 hunks)src/program/state_transition.py(1 hunks)src/routers/secure/items.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py
- src/program/media/item.py
- src/program/state_transition.py
🧰 Additional context used
🪛 Ruff
src/routers/secure/items.py
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
🔇 Additional comments (2)
src/routers/secure/items.py (2)
23-63: LGTM! Well-structured response models.
The response models are well-defined with appropriate type hints and documentation.
518-559: 🛠️ Refactor suggestion
Add error handling and improve query building.
The endpoint needs error handling for database operations and should use parameterized queries consistently.
Apply this diff:
-async def get_all_paused(request: Request, type: Optional[str] = None, db: Session = Depends(get_db)):
+async def get_all_paused(request: Request, type: Optional[str] = None):
+ db = next(get_db())
"""Get all paused media items, optionally filtered by type"""
- # Use raw SQL to verify data
- query = select(
- MediaItem.id,
- MediaItem.title,
- MediaItem.type,
- MediaItem.is_paused,
- MediaItem.paused_at
- ).where(MediaItem.is_paused.is_(True))
+ try:
+ query = select(
+ MediaItem.id,
+ MediaItem.title,
+ MediaItem.type,
+ MediaItem.is_paused,
+ MediaItem.paused_at
+ ).where(MediaItem.is_paused.is_(True))Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
src/program/state_transition.py (1)
26-26: Remove the uncertain comment.Based on previous review comments from Gaisberg, the state transition modification for paused items is the only change needed. You can safely remove this comment.
src/routers/secure/items.py (2)
448-500: Consider refactoring to reduce code duplicationThe
pause_itemsandunpause_itemsfunctions share significant duplicated code. Refactoring common logic into a helper function can improve maintainability.You can create a helper function that handles pausing and unpausing items based on a parameter. This reduces duplication and centralizes the logic.
🧰 Tools
🪛 Ruff (0.8.0)
450-450: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
468-468: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
477-477: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
491-491: Undefined name
SQLAlchemyError(F821)
494-494: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
525-525: Update misleading commentThe comment
# Use raw SQL to verify datais misleading because the code uses SQLAlchemy's query builder, not raw SQL. Consider updating the comment to accurately reflect the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/program/media/item.py(5 hunks)src/program/state_transition.py(1 hunks)src/routers/secure/items.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
src/routers/secure/items.py
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
🔇 Additional comments (8)
src/program/state_transition.py (1)
21-25: LGTM! Early exit for paused items is well implemented.
The implementation correctly checks for paused items and exits early with appropriate logging.
src/program/media/item.py (4)
64-66: LGTM! Well-structured model attributes.
The pause-related columns are properly defined with appropriate types, nullability, and defaults.
77-77: LGTM! Index added as specified.
The index on is_paused has been added with proper naming convention, which will optimize queries filtering on the pause state.
249-250: LGTM! Consistent dictionary representation.
The pause-related fields are properly included in the API response with consistent datetime formatting and null handling.
421-440: 🛠️ Refactor suggestion
Improve unpause method implementation.
Similar improvements needed for consistency with the pause method:
- Remove extra blank lines
- Make logging consistent with pause method
- Consider moving session handling to a separate method
Apply this diff:
def unpause(self) -> None:
"""Resume processing of this media item"""
-
if not self.is_paused:
logger.debug(f"{self.log_string} is not paused")
return
-
logger.debug(f"Unpausing {self.id}")
try:
self.is_paused = False
self.paused_at = None
-
+ self.paused_by = None
session = object_session(self)
if session:
session.flush()
-
logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, paused_at={self.paused_at}")
except Exception as e:
logger.error(f"Failed to unpause {self.log_string}: {str(e)}")
raiseLikely invalid or redundant comment.
src/routers/secure/items.py (3)
491-491:
Undefined name SQLAlchemyError
The exception SQLAlchemyError is used without importing it. Please import it from sqlalchemy.exc.
Apply this diff:
+from sqlalchemy.exc import SQLAlchemyError # Add at the top with other imports🧰 Tools
🪛 Ruff (0.8.0)
491-491: Undefined name SQLAlchemyError
(F821)
589-589:
Undefined name SQLAlchemyError
The exception SQLAlchemyError is used without importing it. Please import it from sqlalchemy.exc.
Apply this diff:
+from sqlalchemy.exc import SQLAlchemyError # Add at the top with other imports🧰 Tools
🪛 Ruff (0.8.0)
589-589: Undefined name SQLAlchemyError
(F821)
565-565: 🛠️ Refactor suggestion
Avoid function calls in default argument values
Avoid using Depends(get_db) in default arguments. Obtain the database session inside the function.
Apply this diff:
-async def get_paused_count(
- request: Request,
- type: Optional[str] = None,
- db: Session = Depends(get_db)
-):
+async def get_paused_count(
+ request: Request,
+ type: Optional[str] = None
+):
+ db: Session = Depends(get_db)Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/program/media/item.py
Outdated
| # Pause related fields | ||
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False) | ||
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) |
There was a problem hiding this comment.
Add missing paused_by column.
The PR objectives mention a paused_by column, but it's not implemented in the model.
Add the following column definition:
# Pause related fields
is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False)
paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True)
+ paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True)📝 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.
| # Pause related fields | |
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False) | |
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| # Pause related fields | |
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False) | |
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True) |
src/program/media/item.py
Outdated
| def pause(self) -> None: | ||
| """Pause processing of this media item""" | ||
| if self.is_paused: | ||
| logger.debug(f"{self.log_string} is already paused") | ||
| return | ||
|
|
||
| logger.debug(f"Pausing {self.id}") | ||
| try: | ||
| self.is_paused = True | ||
| self.paused_at = datetime.now() | ||
|
|
||
| session = object_session(self) | ||
| if session: | ||
| session.flush() | ||
|
|
||
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve pause method implementation.
The pause method has several areas for improvement:
- Consider adding the
paused_byfield mentioned in PR objectives - Remove extra blank lines for better readability
- Consider moving session handling to a separate method
Apply this diff:
def pause(self) -> None:
"""Pause processing of this media item"""
if self.is_paused:
logger.debug(f"{self.log_string} is already paused")
return
-
logger.debug(f"Pausing {self.id}")
try:
self.is_paused = True
self.paused_at = datetime.now()
-
+ self.paused_by = None # TODO: Add user context
session = object_session(self)
if session:
session.flush()
-
logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}")
except Exception as e:
logger.error(f"Failed to pause {self.log_string}: {str(e)}")
raise📝 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 pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise | |
| def pause(self) -> None: | |
| """Pause processing of this media item""" | |
| if self.is_paused: | |
| logger.debug(f"{self.log_string} is already paused") | |
| return | |
| logger.debug(f"Pausing {self.id}") | |
| try: | |
| self.is_paused = True | |
| self.paused_at = datetime.now() | |
| self.paused_by = None # TODO: Add user context | |
| session = object_session(self) | |
| if session: | |
| session.flush() | |
| logger.info(f"{self.log_string} paused, is_paused={self.is_paused}, paused_at={self.paused_at}") | |
| except Exception as e: | |
| logger.error(f"Failed to pause {self.log_string}: {str(e)}") | |
| raise |
src/routers/secure/items.py
Outdated
| db.rollback() | ||
| logger.error(f"Failed to unpause items: {str(e)}") | ||
| raise HTTPException(status_code=500, detail="Failed to unpause items") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining when re-raising exceptions
When raising an exception in an except block, use from e to maintain the original traceback information.
Apply this diff:
- raise HTTPException(status_code=500, detail="Failed to unpause items")
+ raise HTTPException(status_code=500, detail="Failed to unpause items") from e📝 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.
| raise HTTPException(status_code=500, detail="Failed to unpause items") | |
| raise HTTPException(status_code=500, detail="Failed to unpause items") from e |
🧰 Tools
🪛 Ruff (0.8.0)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/routers/secure/items.py
Outdated
| db.rollback() | ||
| logger.error(f"Failed to update items: {str(e)}") | ||
| raise HTTPException(status_code=500, detail="Failed to update items.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining when re-raising exceptions
When raising an exception inside an except block, use from e to preserve the original traceback and provide better context.
Apply this diff:
- raise HTTPException(status_code=500, detail="Failed to update items.")
+ raise HTTPException(status_code=500, detail="Failed to update items.") from e📝 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.
| raise HTTPException(status_code=500, detail="Failed to update items.") | |
| raise HTTPException(status_code=500, detail="Failed to update items.") from e |
🧰 Tools
🪛 Ruff (0.8.0)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/routers/secure/items.py
Outdated
| @router.post("/{id}/unpause", response_model=PauseResponse, operation_id="unpause_item") | ||
| @router.post("/unpause", response_model=PauseResponse, operation_id="unpause_items") | ||
| async def unpause_items(request: Request, ids: str = None, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid function calls in default argument values
Using Depends(get_db) in function defaults is discouraged. Obtain the database session within the function body.
Apply this diff:
-async def unpause_items(request: Request, ids: str = None, db: Session = Depends(get_db)):
+async def unpause_items(request: Request, ids: str = None):
+ db: Session = Depends(get_db)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/routers/secure/items.py
Outdated
|
|
||
| @router.get("/{id}/pause", response_model=PauseStateResponse, operation_id="get_pause_state") | ||
| async def get_pause_state(request: Request, id: str, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid function calls in default argument values
Using Depends(get_db) in default arguments can lead to unexpected behavior. It's better to obtain the database session inside the function.
Apply this diff:
-async def get_pause_state(request: Request, id: str, db: Session = Depends(get_db)):
+async def get_pause_state(request: Request, id: str):
+ db: Session = Depends(get_db)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/routers/secure/items.py
Outdated
| request: Request, | ||
| type: Optional[str] = None, | ||
| db: Session = Depends(get_db) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid function calls in default argument values
Using Depends(get_db) as a default argument is not recommended. Acquire the database session within the function body instead.
Apply this diff:
-async def get_all_paused(
- request: Request,
- type: Optional[str] = None,
- db: Session = Depends(get_db)
-):
+async def get_all_paused(
+ request: Request,
+ type: Optional[str] = None
+):
+ db: Session = Depends(get_db)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/routers/secure/items.py
Outdated
| @router.post("/{id}/pause", response_model=PauseResponse, operation_id="pause_item") | ||
| @router.post("/pause", response_model=PauseResponse, operation_id="pause_items") | ||
| async def pause_items(request: Request, ids: str = None, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid function calls in default argument values
Using Depends(get_db) in function argument defaults can lead to unexpected behavior. It's recommended to obtain the database session inside the function body.
Apply this diff:
-async def pause_items(request: Request, ids: str = None, db: Session = Depends(get_db)):
+async def pause_items(request: Request, ids: str = None):
+ db: Session = Depends(get_db)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py (1)
26-29: Consider using timezone-aware DateTime columns.For accurate tracking of pause/unpause events across different timezones, consider using
DateTime(timezone=True).- sa.Column('paused_at', sa.DateTime(), nullable=True)) + sa.Column('paused_at', sa.DateTime(timezone=True), nullable=True)) - sa.Column('unpaused_at', sa.DateTime(), nullable=True)) + sa.Column('unpaused_at', sa.DateTime(timezone=True), nullable=True))src/routers/secure/items.py (1)
561-596: Improve error handling and logging security.
- Avoid logging raw SQL queries
- Consider returning a 500 status code on database errors instead of a success response with error field
Apply this diff:
- logger.debug(f"Executing count query: {str(query)}") + logger.debug("Executing get_paused_count query") count = db.scalar(query) or 0 # Default to 0 if None logger.debug(f"Found {count} paused items") return { "total": count, "type": type if type else "all" } except SQLAlchemyError as e: logger.error(f"Error in get_paused_count: {str(e)}") - # Return 0 count on error - return { - "total": 0, - "type": type if type else "all", - "error": "Failed to get count" - } + raise HTTPException(status_code=500, detail="Failed to get paused count") from e🧰 Tools
🪛 Ruff (0.8.0)
565-565: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
589-589: Undefined name
SQLAlchemyError(F821)
src/program/media/item.py (1)
425-446: Make logging consistent between pause/unpause methods.The unpause method should follow the same logging pattern as the pause method for consistency.
Update the logging in unpause method:
- logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, unpaused_at={self.unpaused_at}") + logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, paused_at={self.paused_at}, unpaused_at={self.unpaused_at}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(5 hunks)src/routers/secure/items.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
src/routers/secure/items.py
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
465-465: Undefined name SQLAlchemyError
(F821)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
565-565: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
589-589: Undefined name SQLAlchemyError
(F821)
🔇 Additional comments (9)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py (4)
16-16: Replace placeholder revision ID with the actual identifier.
The revision ID on line 16 still contains a placeholder while lines 3-4 have the correct value.
33-34: LGTM! Index creation looks good.
The index on is_paused is well-named and will optimize the performance of queries filtering on pause state.
37-43: LGTM! Downgrade operations are complete and correctly ordered.
The downgrade function properly reverses all changes made in the upgrade function, maintaining referential integrity by dropping the index before the columns.
22-34: Verify the migration's impact on existing data.
The migration adds new columns but doesn't populate them with data. Please verify:
- If existing records need a data backfill strategy
- If the application code handles null values appropriately for these new columns
✅ Verification successful
Migration is safe, no data backfill needed
The codebase shows proper handling of the new pause-related columns:
-
All new columns are nullable with appropriate defaults:
is_pausedhas default=Falsepaused_at,unpaused_at, andpaused_byare nullable
-
The application code in
src/program/media/item.pyproperly handles these fields:- Null checks are present when accessing datetime fields
- Clear pause/unpause logic that manages these fields appropriately
- State transitions skip paused items safely
-
API responses in
src/routers/secure/items.pyhandle null values correctly through Optional types and null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for null checks on these columns in the codebase
rg -A 5 "is_paused|paused_at|unpaused_at|paused_by" --type python
# Look for potential data population logic
ast-grep --pattern 'def upgrade() {
$$$
# Look for data updates
op.execute($_)
$$$
}'
Length of output: 194
Script:
#!/bin/bash
# Search for null checks and usage of these columns in the codebase
rg -A 5 "is_paused|paused_at|unpaused_at|paused_by"
# Look for models and schema definitions
rg -A 10 "class MediaItem"
# Look for any data population or migration scripts
fd "migration|populate" --type f
Length of output: 10241
src/routers/secure/items.py (2)
23-63: LGTM! Well-structured response models.
The response models are well-defined with proper type hints and documentation.
475-499: Apply the same fixes as the pause endpoint.
The unpause endpoint has the same dependency injection and exception handling issues.
🧰 Tools
🪛 Ruff (0.8.0)
477-477: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
491-491: Undefined name SQLAlchemyError
(F821)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/program/media/item.py (3)
64-68: LGTM! Column definitions are well-structured.
The pause-related columns are properly defined with appropriate types, nullable constraints, and default values.
79-79: LGTM! Index on is_paused implemented as requested.
The index will improve query performance when filtering paused items.
770-770: LGTM! Clear error handling for invalid item types.
The error message clearly indicates the issue when an invalid item type is provided.
| op.add_column('MediaItem', | ||
| sa.Column('paused_by', sa.String(), nullable=True)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add length constraint to the paused_by String column.
Unbounded string columns can lead to storage issues. Consider adding a reasonable length constraint that aligns with your user ID or username field lengths.
- sa.Column('paused_by', sa.String(), nullable=True))
+ sa.Column('paused_by', sa.String(length=255), nullable=True))📝 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.
| op.add_column('MediaItem', | |
| sa.Column('paused_by', sa.String(), nullable=True)) | |
| op.add_column('MediaItem', | |
| sa.Column('paused_by', sa.String(length=255), nullable=True)) |
src/routers/secure/items.py
Outdated
| async def get_pause_state(request: Request, id: str, db: Session = Depends(get_db)): | ||
| """Check if a media item is paused""" | ||
| item = db.execute( | ||
| select(MediaItem).where(MediaItem.id == id) | ||
| ).unique().scalar_one_or_none() | ||
|
|
||
| if not item: | ||
| raise HTTPException(status_code=404, detail="Item not found") | ||
|
|
||
| return { | ||
| "is_paused": item.is_paused, | ||
| "paused_at": str(item.paused_at) if item.paused_at else None, | ||
| "item_id": item.id, | ||
| "title": item.title | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The endpoint should handle potential database errors and use proper dependency injection.
Apply this diff:
-async def get_pause_state(request: Request, id: str, db: Session = Depends(get_db)):
+async def get_pause_state(request: Request, id: str):
+ db = next(get_db())
"""Check if a media item is paused"""
- item = db.execute(
- select(MediaItem).where(MediaItem.id == id)
- ).unique().scalar_one_or_none()
+ try:
+ item = db.execute(
+ select(MediaItem).where(MediaItem.id == id)
+ ).unique().scalar_one_or_none()
+ except SQLAlchemyError as e:
+ logger.error(f"Failed to fetch pause state: {str(e)}")
+ raise HTTPException(status_code=500, detail="Failed to fetch pause state") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
502-502: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
src/routers/secure/items.py
Outdated
| @router.post("/pause", response_model=PauseResponse, operation_id="pause_items") | ||
| async def pause_items(request: Request, ids: str = None, db: Session = Depends(get_db)): | ||
| """Pause media items from being processed""" | ||
| item_ids = handle_ids(ids) | ||
|
|
||
| items = db.execute( | ||
| select(MediaItem).where(MediaItem.id.in_(item_ids)) | ||
| ).unique().scalars().all() | ||
|
|
||
| if not items: | ||
| raise HTTPException(status_code=404, detail="No items found") | ||
|
|
||
| try: | ||
| for item in items: | ||
| item.pause() | ||
| db.commit() | ||
| except SQLAlchemyError as e: | ||
| db.rollback() | ||
| logger.error(f"Failed to pause items: {str(e)}") | ||
| raise HTTPException(status_code=500, detail="Failed to pause items") | ||
|
|
||
| return { | ||
| "message": f"Successfully paused {len(items)} items", | ||
| "ids": item_ids | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix dependency injection and exception handling.
Several improvements needed:
- Move database session acquisition into the function body
- Add exception chaining
- Import SQLAlchemyError
Apply this diff:
+from sqlalchemy.exc import SQLAlchemyError
+
-async def pause_items(request: Request, ids: str = None, db: Session = Depends(get_db)):
+async def pause_items(request: Request, ids: str = None):
+ db = next(get_db())
"""Pause media items from being processed"""
item_ids = handle_ids(ids)
# ... existing code ...
except SQLAlchemyError as e:
db.rollback()
logger.error(f"Failed to pause items: {str(e)}")
- raise HTTPException(status_code=500, detail="Failed to pause items")
+ raise HTTPException(status_code=500, detail="Failed to pause items") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
450-450: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
465-465: Undefined name SQLAlchemyError
(F821)
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/routers/secure/items.py
Outdated
| async def get_all_paused( | ||
| request: Request, | ||
| type: Optional[str] = None, | ||
| db: Session = Depends(get_db) | ||
| ): | ||
| """Get all paused media items, optionally filtered by type""" | ||
| # Use raw SQL to verify data | ||
| query = select( | ||
| MediaItem.id, | ||
| MediaItem.title, | ||
| MediaItem.type, | ||
| MediaItem.is_paused, | ||
| MediaItem.paused_at | ||
| ).where(MediaItem.is_paused.is_(True)) | ||
|
|
||
| if type: | ||
| valid_types = [t.value for t in MediaType] | ||
| if type not in valid_types: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | ||
| ) | ||
| query = query.where(MediaItem.type == type) | ||
|
|
||
| logger.debug(f"Executing query: {str(query)}") | ||
| result = db.execute(query) | ||
| rows = result.fetchall() | ||
| logger.debug(f"Raw SQL found {len(rows)} rows") | ||
|
|
||
| # Map results to response model | ||
| items = [ | ||
| { | ||
| "id": row.id, | ||
| "title": row.title, | ||
| "type": row.type, | ||
| "paused_at": str(row.paused_at) if row.paused_at else None | ||
| } | ||
| for row in rows | ||
| ] | ||
|
|
||
| return AllPausedResponse(count=len(items), items=items) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve logging security.
- Add error handling for database operations
- Avoid logging raw SQL queries as they might contain sensitive data
Apply this diff:
if type:
valid_types = [t.value for t in MediaType]
if type not in valid_types:
raise HTTPException(
status_code=400,
detail=f"Invalid type. Must be one of: {', '.join(valid_types)}"
)
query = query.where(MediaItem.type == type)
- logger.debug(f"Executing query: {str(query)}")
+ logger.debug("Executing get_all_paused query")
- result = db.execute(query)
- rows = result.fetchall()
- logger.debug(f"Raw SQL found {len(rows)} rows")
+ try:
+ result = db.execute(query)
+ rows = result.fetchall()
+ logger.debug(f"Found {len(rows)} paused items")
+ except SQLAlchemyError as e:
+ logger.error(f"Failed to fetch paused items: {str(e)}")
+ raise HTTPException(status_code=500, detail="Failed to fetch paused items") from e📝 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.
| @router.get("/paused", response_model=AllPausedResponse, operation_id="get_all_paused", responses={200: {"description": "Success, even if no items found"}}, response_model_exclude_none=True) | |
| async def get_all_paused( | |
| request: Request, | |
| type: Optional[str] = None, | |
| db: Session = Depends(get_db) | |
| ): | |
| """Get all paused media items, optionally filtered by type""" | |
| # Use raw SQL to verify data | |
| query = select( | |
| MediaItem.id, | |
| MediaItem.title, | |
| MediaItem.type, | |
| MediaItem.is_paused, | |
| MediaItem.paused_at | |
| ).where(MediaItem.is_paused.is_(True)) | |
| if type: | |
| valid_types = [t.value for t in MediaType] | |
| if type not in valid_types: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | |
| ) | |
| query = query.where(MediaItem.type == type) | |
| logger.debug(f"Executing query: {str(query)}") | |
| result = db.execute(query) | |
| rows = result.fetchall() | |
| logger.debug(f"Raw SQL found {len(rows)} rows") | |
| # Map results to response model | |
| items = [ | |
| { | |
| "id": row.id, | |
| "title": row.title, | |
| "type": row.type, | |
| "paused_at": str(row.paused_at) if row.paused_at else None | |
| } | |
| for row in rows | |
| ] | |
| return AllPausedResponse(count=len(items), items=items) | |
| @router.get("/paused", response_model=AllPausedResponse, operation_id="get_all_paused", responses={200: {"description": "Success, even if no items found"}}, response_model_exclude_none=True) | |
| async def get_all_paused( | |
| request: Request, | |
| type: Optional[str] = None, | |
| db: Session = Depends(get_db) | |
| ): | |
| """Get all paused media items, optionally filtered by type""" | |
| # Use raw SQL to verify data | |
| query = select( | |
| MediaItem.id, | |
| MediaItem.title, | |
| MediaItem.type, | |
| MediaItem.is_paused, | |
| MediaItem.paused_at | |
| ).where(MediaItem.is_paused.is_(True)) | |
| if type: | |
| valid_types = [t.value for t in MediaType] | |
| if type not in valid_types: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Invalid type. Must be one of: {', '.join(valid_types)}" | |
| ) | |
| query = query.where(MediaItem.type == type) | |
| logger.debug("Executing get_all_paused query") | |
| try: | |
| result = db.execute(query) | |
| rows = result.fetchall() | |
| logger.debug(f"Found {len(rows)} paused items") | |
| except SQLAlchemyError as e: | |
| logger.error(f"Failed to fetch paused items: {str(e)}") | |
| raise HTTPException(status_code=500, detail="Failed to fetch paused items") from e | |
| # Map results to response model | |
| items = [ | |
| { | |
| "id": row.id, | |
| "title": row.title, | |
| "type": row.type, | |
| "paused_at": str(row.paused_at) if row.paused_at else None | |
| } | |
| for row in rows | |
| ] | |
| return AllPausedResponse(count=len(items), items=items) |
🧰 Tools
🪛 Ruff (0.8.0)
522-522: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/routers/secure/items.py (6)
72-76: Handle empty strings in comma-separated listThe function should filter out empty strings when splitting comma-separated IDs.
def handle_ids(ids: Optional[str]) -> list[str]: """Handle comma-separated IDs or single ID""" if not ids: raise HTTPException(status_code=400, detail="No item ID provided") - return [str(id) for id in ids.split(",")] if "," in ids else [str(ids)] + if "," in ids: + id_list = [str(id.strip()) for id in ids.split(",") if id.strip()] + if not id_list: + raise HTTPException(status_code=400, detail="No valid IDs provided") + return id_list + return [str(ids)]
11-12: Remove unused importsThe following imports are not used in the code:
NoResultFoundfrom sqlalchemy.excwith_polymorphicfrom sqlalchemy.orm🧰 Tools
🪛 Ruff (0.8.0)
11-11:
sqlalchemy.exc.NoResultFoundimported but unusedRemove unused import:
sqlalchemy.exc.NoResultFound(F401)
12-12:
sqlalchemy.orm.with_polymorphicimported but unusedRemove unused import:
sqlalchemy.orm.with_polymorphic(F401)
269-271: Use exception chaining in error handlingWhen re-raising exceptions, use
from eto preserve the original traceback.except SQLAlchemyError as e: logger.error(f"Database error: {e}") - raise HTTPException(status_code=500, detail="Database error") + raise HTTPException(status_code=500, detail="Database error") from e🧰 Tools
🪛 Ruff (0.8.0)
271-271: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
455-455: Use Pythonic boolean comparisonsAvoid explicit comparisons with
True.- .where(MediaItem.is_paused == True) + .where(MediaItem.is_paused.is_(True))Also applies to: 508-508
🧰 Tools
🪛 Ruff (0.8.0)
455-455: Avoid equality comparisons to
True; useif MediaItem.is_paused:for truth checksReplace with
MediaItem.is_paused(E712)
488-491: Use exception chaining consistentlyAdd exception chaining to all error handlers to preserve the original traceback.
Example fix:
except SQLAlchemyError as e: logger.error(f"Database error: {e}") raise HTTPException( status_code=500, - detail=f"Database error occurred: {str(e)}" + detail=f"Database error occurred: {str(e)}" + ) from eApply this pattern to all similar error handling blocks.
Also applies to: 522-525, 549-552, 601-604, 651-654, 690-693
🧰 Tools
🪛 Ruff (0.8.0)
488-491: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
554-654: Consider bulk database operationsThe pause/unpause operations could be optimized using bulk updates instead of iterating through items.
Consider using SQLAlchemy's bulk update operations:
from sqlalchemy import update # Example bulk update stmt = ( update(MediaItem) .where(MediaItem.id.in_(item_ids)) .values( is_paused=True, paused_at=datetime.utcnow(), paused_by=request.state.user.username if hasattr(request.state, 'user') else None ) ) session.execute(stmt) session.commit()🧰 Tools
🪛 Ruff (0.8.0)
560-560: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
601-604: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
612-612: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
651-654: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
src/program/media/item.py (2)
21-26: Enhance enum documentation.Consider adding more detailed documentation to describe the purpose and usage of each enum value.
class MediaType(str, Enum): - """Media type enum""" + """Enum representing the different types of media items. + + Values: + MOVIE: Represents a single movie + SHOW: Represents a TV show containing seasons + SEASON: Represents a season within a show + EPISODE: Represents an episode within a season + """
431-452: Improve unpause method implementation.Add type hints to docstring and remove extra blank lines for consistency.
def unpause(self) -> None: - """Resume processing of this media item""" + """Resume processing of this media item. + + Note: The paused_at field is preserved for history tracking. + """ - if not self.is_paused: logger.debug(f"{self.log_string} is not paused") return - logger.debug(f"Unpausing {self.id}") try: self.is_paused = False self.unpaused_at = datetime.now() - # Keep paused_at for history - session = object_session(self) if session: session.flush() - logger.info(f"{self.log_string} unpaused, is_paused={self.is_paused}, unpaused_at={self.unpaused_at}") except Exception as e: logger.error(f"Failed to unpause {self.log_string}: {str(e)}") raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py(1 hunks)src/program/media/item.py(6 hunks)src/routers/secure/items.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/alembic/versions/20241114_1400_c99239e3445f_add_pause_functionality.py
🧰 Additional context used
🪛 Ruff (0.8.0)
src/routers/secure/items.py
11-11: sqlalchemy.exc.NoResultFound imported but unused
Remove unused import: sqlalchemy.exc.NoResultFound
(F401)
12-12: sqlalchemy.orm.with_polymorphic imported but unused
Remove unused import: sqlalchemy.orm.with_polymorphic
(F401)
254-254: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
271-271: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
443-443: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
455-455: Avoid equality comparisons to True; use if MediaItem.is_paused: for truth checks
Replace with MediaItem.is_paused
(E712)
488-491: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
496-496: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
508-508: Avoid equality comparisons to True; use if MediaItem.is_paused: for truth checks
Replace with MediaItem.is_paused
(E712)
522-525: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
530-530: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
549-552: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
560-560: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
601-604: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
612-612: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
651-654: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
666-666: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
690-693: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
src/routers/secure/items.py (1)
21-64: Well-structured response models!
The response models are well-designed with:
- Clear separation of concerns
- Proper type hints
- Descriptive docstrings
src/program/media/item.py (1)
257-259: Add missing paused_by field to dictionary representation.
The paused_by field is defined in the model but not included in the to_dict() method.
| logger.debug(f"Query parameters: {query.compile().params}") | ||
|
|
||
| result = session.execute(query).scalars().unique().all() | ||
| logger.debug(f"Found {len(result)} paused items") | ||
|
|
||
| # Add debug logging for each item | ||
| for item in result: | ||
| logger.debug(f"Paused item: id={item.id}, type={item.type}, is_paused={item.is_paused}, class={item.__class__.__name__}") | ||
| logger.debug(f"Item dict before conversion: {vars(item)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce debug logging to prevent sensitive data exposure
The current debug logging includes raw SQL queries and object dumps which might expose sensitive data.
- logger.debug(f"Executing query: {query}")
- logger.debug(f"Query parameters: {query.compile().params}")
+ logger.debug("Executing get_all_paused query")
result = session.execute(query).scalars().unique().all()
logger.debug(f"Found {len(result)} paused items")
- # Add debug logging for each item
- for item in result:
- logger.debug(f"Paused item: id={item.id}, type={item.type}, is_paused={item.is_paused}, class={item.__class__.__name__}")
- logger.debug(f"Item dict before conversion: {vars(item)}")📝 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.
| logger.debug(f"Executing query: {query}") | |
| logger.debug(f"Query parameters: {query.compile().params}") | |
| result = session.execute(query).scalars().unique().all() | |
| logger.debug(f"Found {len(result)} paused items") | |
| # Add debug logging for each item | |
| for item in result: | |
| logger.debug(f"Paused item: id={item.id}, type={item.type}, is_paused={item.is_paused}, class={item.__class__.__name__}") | |
| logger.debug(f"Item dict before conversion: {vars(item)}") | |
| logger.debug("Executing get_all_paused query") | |
| result = session.execute(query).scalars().unique().all() | |
| logger.debug(f"Found {len(result)} paused items") |
| # Pause related fields | ||
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True, index=True) | ||
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | ||
| unpaused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | ||
| paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate index definition.
The is_paused column has an index defined both at the column level (index=True) and in __table_args__. This is redundant and could cause confusion. Remove the column-level index since the table-level index provides more control and visibility.
- is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True, index=True)
+ is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True)📝 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.
| # Pause related fields | |
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True, index=True) | |
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| unpaused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True) | |
| # Pause related fields | |
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True) | |
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| unpaused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | |
| paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True) |
| # Pause related fields | ||
| is_paused: Mapped[Optional[bool]] = mapped_column(sqlalchemy.Boolean, default=False, nullable=True, index=True) | ||
| paused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | ||
| unpaused_at: Mapped[Optional[datetime]] = mapped_column(sqlalchemy.DateTime, nullable=True) | ||
| paused_by: Mapped[Optional[str]] = mapped_column(sqlalchemy.String, nullable=True) |
src/program/media/item.py
Outdated
| logger.debug(f"{self.log_string} is already paused") | ||
| return | ||
|
|
||
| logger.debug(f"Pausing {self.id}") |
There was a problem hiding this comment.
Lets add a more identifyin log string here
| @@ -17,45 +17,62 @@ def process_event(emitted_by: Service, existing_item: MediaItem | None = None, c | |||
| no_further_processing: ProcessedEvent = (None, []) | |||
| items_to_submit = [] | |||
|
|
|||
| #TODO - Reindex non-released badly indexed items here | |||
| # Skip processing if item is paused | |||
There was a problem hiding this comment.
I think you misunderstood. Restore the original state_transition.py and just add the 3 line addition.
| message: str | ||
| ids: list[str] | ||
|
|
||
| return [item.to_dict() for item in items] |
There was a problem hiding this comment.
This was extended before
| query = ( | ||
| select(MediaItem) | ||
| .outerjoin(Movie, Movie.id == MediaItem.id) |
There was a problem hiding this comment.
are all these outerjoins needed?
feat: Add media item pause functionality with FastAPI endpoints
Implement comprehensive pause/unpause API endpoints for media item processing control.
API Endpoints:
Response Models:
Features:
Database Changes:
Breaking changes: None
Migration: Required (adds new columns to MediaItem table)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
These updates improve user control over media processing and streamline interactions with the system.