Islverse markers api#482
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| markers = _ensure_verse_zero(markers) | ||
| record.verse_markers_json = markers |
There was a problem hiding this comment.
Suggestion: The update path writes marker data without validating verse values against clean_bible, unlike the create path. This allows updates to persist non-existent verses/ranges for the book/chapter, causing inconsistent data and downstream lookup failures. Run the same verse validation used in bulk create before saving. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ ISL verse marker updates can persist non-existent verse IDs.
- ⚠️ Bulk create enforces verse validity; updates bypass it.
- ⚠️ Downstream consumers may misinterpret or skip invalid markers.Steps of Reproduction ✅
1. Open `backend/app/crud/isl_verse_markers_crud.py` and locate `update_verse_markers` at
lines 16–50; note that it only calls `_ensure_verse_zero(markers)` at line 44 and then
assigns `record.verse_markers_json = markers` at line 45 before committing at line 47.
2. In a Python shell or test, create an `IslVideo` row and corresponding `IslVerseMarkers`
row for `isl_bible_id=X` using the same SQLAlchemy models imported at lines 5 and 24–26,
and ensure that `db_models.CleanBible` contains verses only up to a certain maximum (e.g.,
1–10) for that `book_id`/`chapter`.
3. Still in the shell/test, call `update_verse_markers(db_session, isl_bible_id=X,
markers=[{"verse": 999, "time": "00:00:10:00"}])`, where verse `999` does not exist in
`db_models.CleanBible` for that book/chapter; observe that `update_verse_markers` runs
without calling `_validate_marker_verses` (defined at lines 120–158 and used only in
`add_verse_markers_bulk` at line 248).
4. Query `db_models.IslVerseMarkers` for `isl_video_id=X` after the update and see that
`verse_markers_json` now contains verse `999`, demonstrating that the update path
persisted an invalid verse that the bulk-create path (`add_verse_markers_bulk` at lines
174–264) would have rejected via `_validate_marker_verses`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/app/crud/isl_verse_markers_crud.py
**Line:** 44:45
**Comment:**
*Incomplete Implementation: The update path writes marker data without validating verse values against `clean_bible`, unlike the create path. This allows updates to persist non-existent verses/ranges for the book/chapter, causing inconsistent data and downstream lookup failures. Run the same verse validation used in bulk create before saving.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| except SQLAlchemyError as exc: | ||
| logger.error("Error deleting ISL Bible %s: %s", isl_id, exc) | ||
| errors.append(f"Error deleting ISL Bible {isl_id}: {exc}") | ||
|
|
||
| db_session.commit() |
There was a problem hiding this comment.
Suggestion: The loop catches SQLAlchemyError but never rolls back the session; if an ORM error occurs, the session can remain in a failed transaction state and the final commit() will raise, turning a partial-failure response into a 500. Roll back inside the exception path (or isolate each delete in its own transaction) before continuing. [logic error]
Severity Level: Major ⚠️
- ❌ Bulk delete can raise 500 after first database error.
- ⚠️ Session remains in failed state for subsequent operations.
- ⚠️ Partial successes are lost due to failing final commit.Steps of Reproduction ✅
1. Open `backend/app/crud/isl_verse_markers_crud.py` and inspect
`delete_verse_markers_bulk` at lines 79–108; note that it wraps the per-ID delete logic in
a `try` block (lines 89–100), catches `SQLAlchemyError` at line 102 to log and append an
error, but does not call `db_session.rollback()` before eventually calling
`db_session.commit()` at line 106.
2. In a test or shell, obtain a `db_session` and construct `isl_bible_ids` such that
deleting one of the corresponding `IslVerseMarkers` rows triggers a `SQLAlchemyError` (for
example, by forcing a database-level error or simulating a constraint/connection issue so
that `db_session.delete(record)` or the pending flush fails).
3. Call `delete_verse_markers_bulk(db_session, isl_bible_ids)`; when the problematic ID is
processed, the `except SQLAlchemyError as exc:` block at lines 102–104 will run, logging
the error and recording it in the `errors` list but leaving the SQLAlchemy session in a
failed transaction state.
4. After the loop completes, observe that `db_session.commit()` at line 106 is invoked on
this failed session and raises a further SQLAlchemy error (e.g., `PendingRollbackError`),
causing the bulk delete caller (such as an API endpoint or job that uses this CRUD
function) to see a 500 error instead of a structured partial-success response.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/app/crud/isl_verse_markers_crud.py
**Line:** 102:106
**Comment:**
*Logic Error: The loop catches `SQLAlchemyError` but never rolls back the session; if an ORM error occurs, the session can remain in a failed transaction state and the final `commit()` will raise, turning a partial-failure response into a 500. Roll back inside the exception path (or isolate each delete in its own transaction) before continuing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
CodeAnt AI finished reviewing your PR. |
User description
#476
apis for isl verse markers
CodeAnt-AI Description
Add ISL verse marker management endpoints with validation
What Changed
Impact
✅ Faster ISL verse marker setup✅ Fewer invalid marker uploads✅ Clearer bulk delete results🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.