Conversation
Commit the comprehensive database service layer that was previously gitignored. Includes modular services for analytics, content, pages, reports, and Wikidata operations. Each service provides high-level database operations with consistent error handling and session management patterns.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis change adds SQLAlchemy-backed database services for analytics, content, pages, reports, utilities, and Wikidata, consolidates package exports, adds deletion and session guards, removes the legacy database wrapper, and applies several targeted behavior and typing fixes. ChangesDatabase service layer
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service
participant Database
Caller->>Service: invoke database operation
Service->>Database: query or mutate ORM record
Database-->>Service: return committed result
Service-->>Caller: return record, mapping, or status
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of SQLAlchemy-based database services under src/db/tools/services/ to manage analytics, content, page translations, and Wikidata mappings, alongside minor bug fixes and typing updates in existing scripts. The review feedback identifies several critical issues in these new services: set_page_target and set_user_page_target attempt to modify detached ORM instances without merging them into the active session, and add_translate_row_to_db performs a bulk update but fails to commit the transaction. Additionally, the db_guard_rollback decorator manages an unused session that does not affect the decorated functions, and there is an unused helper function _row_to_dict that should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
src/db/tools/services/analytics/views_new_service.py (1)
152-158: 🚀 Performance & Scalability | 🔵 TrivialUse SQL
SUM()instead of loading all records into Python.
get_total_views_for_targetcallslist_views_by_target(which loads all matching ORM objects into memory) then sumsr.viewsin Python. For targets with many records, this is inefficient. A SQL-side aggregation avoids materializing all rows.♻️ Proposed refactor
from sqlalchemy import func def get_total_views_for_target( target: str, lang: str | None = None, ) -> int: """Get total views across all years for a target.""" with get_session() as session: query = session.query(func.coalesce(func.sum(ViewsNewRecord.views), 0)).filter( ViewsNewRecord.target == target ) if lang: query = query.filter(ViewsNewRecord.lang == lang) return query.scalar() or 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/analytics/views_new_service.py` around lines 152 - 158, Update get_total_views_for_target to perform the total using a database-side SQL SUM aggregation filtered by target and optional lang, rather than calling list_views_by_target or materializing ORM records; return zero when the aggregate result is null.src/db/tools/services/analytics/word_service.py (1)
91-107: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnrestricted
setattrfrom**kwargsallows overwriting any column, including the primary key.
hasattr(orm_obj, key)matches every mapped attribute (w_idincluded), so a caller passingw_id=...inkwargswould silently reassign the primary key. An explicit allow-list of updatable fields is safer. The same pattern recurs inpage_service.update_pageanduser_page_service.update_user_page.🔧 Proposed fix
- for key, value in kwargs.items(): - if hasattr(orm_obj, key): - setattr(orm_obj, key, value) + allowed_fields = {"w_title", "w_lead_words", "w_all_words"} + for key, value in kwargs.items(): + if key in allowed_fields: + setattr(orm_obj, key, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/analytics/word_service.py` around lines 91 - 107, Restrict the dynamic updates in update_word to an explicit allow-list of mutable WordRecord fields instead of using hasattr, excluding the primary key w_id and any other protected attributes. Apply the same allow-list approach in page_service.update_page and user_page_service.update_user_page, while preserving existing commit, refresh, and return behavior.src/db/tools/services/pages/user_page_service.py (1)
1-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis module is a near-complete structural duplicate of
page_service.py(same forqid_others_service.pyvsqid_service.py).
list_user_pages/list_pages,add_user_page/add_page,insert_user_page_target/insert_page_target,find_user_page_record/find_page_record, etc. are essentially identical logic parameterized only by model class. A generic factory (e.g. a small class/module builder that takes the ORM model and produces these CRUD functions) would remove most of this duplication across both file pairs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/pages/user_page_service.py` around lines 1 - 125, Extract the duplicated CRUD logic from list_user_pages, add_user_page, and insert_user_page_target into a shared generic service/factory parameterized by the ORM model, then reuse it from the corresponding page_service and qid service modules. Preserve each module’s existing public function names, signatures, ordering, filtering, and error behavior while removing the duplicated implementations.src/db/tools/services/reports/pages_users_to_main_service.py (1)
55-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnrestricted kwargs update, same pattern as
project_service.update_project.Any attribute matching the model (including the PK
id) can be overwritten viasetattr. Consider an explicit allow-list of updatable fields (new_target,new_user,new_qid).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/reports/pages_users_to_main_service.py` around lines 55 - 75, The update_pages_users_to_main function currently permits arbitrary model attributes, including the primary key, to be modified. Restrict its kwargs update loop to the explicit updatable fields new_target, new_user, and new_qid, ignoring or rejecting all other keys before calling setattr.src/db/tools/services/content/project_service.py (1)
63-89: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnrestricted kwargs update allows overwriting protected fields.
update_project's generic loop lets any caller-supplied key matching an existing attribute (including the primary keyg_id) be set viasetattr. Consider restricting to an explicit allow-list of updatable fields.♻️ Proposed fix
+_UPDATABLE_FIELDS = {"g_title", "title"} + def update_project(project_id: int, **kwargs) -> ProjectRecord: ... for key, value in kwargs.items(): - if not hasattr(orm_obj, key): + if key not in _UPDATABLE_FIELDS or not hasattr(orm_obj, key): continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/content/project_service.py` around lines 63 - 89, Restrict the kwargs handled by update_project to an explicit allow-list of mutable project fields, excluding protected attributes such as g_id and any primary-key or identity fields. Apply the existing title/g_title validation only to allowed title fields, and ignore or reject keys outside the allow-list without assigning them via setattr.src/db/tools/services/pages/pages_users_to_main_service.py (1)
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_row_to_dictis unused dead code.
list_pendingconstructs dicts manually (lines 56-69) and never calls_row_to_dict. Remove it or refactorlist_pendingto use it.♻️ Optional: remove dead code
-def _row_to_dict(row: Any) -> Dict[str, Any]: - """Convert a result row (tuple of columns from a join) into a dict.""" - if hasattr(row, "_asdict"): - return dict(row._asdict()) - return dict(row) - - def list_pending(lang: str = "All") -> List[Dict[str, Any]]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/pages/pages_users_to_main_service.py` around lines 22 - 26, Remove the unused _row_to_dict helper, since list_pending currently constructs its result dictionaries directly and does not call it; leave the existing list_pending conversion behavior unchanged.src/db/tools/services/pages/__init__.py (1)
69-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
__all__is not sorted (RUF022).Ruff flags this list as unsorted. The same applies to
__all__inin_process_service.py,pages_users_to_main_service.py,results_2026_service.py, andtranslate_type_service.py.
↕️ Optional: sort `__all__`__all__ = [ + "add_in_process", + "add_page", + "add_translate_row_to_db", + "add_translate_type", + "add_user_page", + "can_translate_full", + "can_translate_lead", + "check_main_page_exists", + "count_category_members", + "count_translated", + "delete_in_process", + "delete_in_process_by_title_user_lang", + "delete_page", + "delete_translate_type", + "delete_user_page", + "exists_by_lang_and_category", + "find_page_record", + "find_user_page_record", + "get_in_process", + "get_in_process_by_title_user_lang", + "get_in_process_counts_by_user", + "get_page_by_id", + "get_translate_type", + "get_translate_type_by_title", + "get_user_page", + "get_user_page_by_id", + "insert_page_target", + "insert_user_page_target", + "is_in_process", + "list_full_enabled_types", + "list_in_process", + "list_in_process_by_lang", + "list_in_process_by_user", + "list_lead_enabled_types", + "list_new_titles", + "list_pages", + "list_pages_by_lang_cat", + "list_pending", + "list_translated", + "list_translate_types", + "list_user_pages", + "missing_by_lang_and_category", + "set_page_target", + "set_user_page_target", + "statics_by_category", + "update_in_process", + "update_page", + "update_translate_type", + "update_user_page", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/pages/__init__.py` around lines 69 - 119, Sort the exported names in the __all__ list alphabetically in __init__.py and the corresponding __all__ lists in in_process_service.py, pages_users_to_main_service.py, results_2026_service.py, and translate_type_service.py, without changing their contents.Source: Linters/SAST tools
src/db/tools/services/pages/translate_type_service.py (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
list_new_titleswraps the subquery in an unnecessary extrasession.query().
existing_titlesis already a subquery (line 35). Passing it directly tonotin_()avoids the extra nesting.♻️ Optional: simplify subquery usage
def list_new_titles() -> List[str]: """Return titles in the qids table that are not yet in translate_type.""" with get_session() as session: - existing_titles = session.query(TranslateTypeRecord.tt_title).subquery() + existing_titles = session.query(TranslateTypeRecord.tt_title) rows = ( session.query(QidRecord.title) - .filter(QidRecord.title.notin_(session.query(existing_titles.c.tt_title))) + .filter(QidRecord.title.notin_(existing_titles)) .distinct() .order_by(QidRecord.title.asc()) .all() ) return [row[0] for row in rows if row[0]]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/tools/services/pages/translate_type_service.py` around lines 35 - 38, Update list_new_titles to pass the existing_titles subquery directly to QidRecord.title.notin_(), removing the unnecessary session.query wrapper while preserving the existing filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/tools/services/analytics/word_service.py`:
- Around line 67-88: Update add_or_update_word to handle the concurrent-insert
IntegrityError during session.commit, matching the existing protection in
add_word. Roll back the session and apply the established duplicate-title
handling behavior instead of allowing the exception to propagate.
In `@src/db/tools/services/content/category_service.py`:
- Around line 18-22: Update add_category and update_category so is_default is
modified only when the caller explicitly provides it, rather than defaulting
omitted values to 0. Preserve the existing behavior for explicitly supplied
values and ensure unrelated field updates do not clear an existing default flag.
- Around line 38-65: Wrap the commit logic in both add_category and
update_category with IntegrityError handling, following the established pattern
in lang_service.add_lang and project_service.add_project. Roll back the session
on failure and raise a clean ValueError for unique category conflicts, while
preserving the existing successful commit and refresh behavior.
In `@src/db/tools/services/content/lang_service.py`:
- Around line 44-65: Add a database-level unique constraint or unique index for
the code column in the LangRecord model and corresponding langs table
DDL/migration. Keep add_lang’s IntegrityError handling intact so duplicate codes
reliably raise the existing ValueError, and ensure add_or_update_lang uses the
constrained schema.
In `@src/db/tools/services/content/project_service.py`:
- Around line 63-89: Update update_project and update_project_title to catch the
database IntegrityError raised during commit when a g_title rename conflicts
with an existing project, translating it into the same friendly ValueError
behavior used by add_project; preserve existing validation and successful-update
behavior, and ensure the session handles the failed transaction appropriately
before returning or propagating other errors.
In `@src/db/tools/services/delete_service.py`:
- Around line 76-102: The delete_user_page_to_main function must return False
when neither PagesUsersToMainRecord nor UserPageRecord exists before deletion.
Track whether at least one row was found, preserve the existing deletion,
commit, rollback, and post-operation verification behavior, and return True only
when a row existed and both records are absent afterward.
- Around line 149-169: Remove the duplicate "delete_record_by_pk" entry from the
__all__ list, keeping one export entry and preserving all other listed symbols.
In `@src/db/tools/services/pages/in_process_service.py`:
- Around line 120-122: Update update_in_process so kwargs are applied only when
the field name is in an explicit whitelist of mutable attributes, excluding
primary-key and immutable fields such as id and add_date; replace the broad
hasattr check while preserving updates for approved fields.
In `@src/db/tools/services/pages/page_service.py`:
- Around line 161-177: Update set_page_target to re-attach the detached
PageRecord within the newly opened session before changing target and pupdate,
by loading the persisted row or merging record and then mutating the attached
instance. Commit the attached entity so the target update is persisted while
preserving the existing rollback and failure behavior.
- Around line 221-233: Move the title underscore-to-space normalization before
the get_word_counts_for_title(title) call in the word == 0 branch, so the lookup
receives the normalized title. Preserve the existing translate_type selection
and retain the remaining parameter normalizations in their current flow.
In `@src/db/tools/services/pages/user_page_service.py`:
- Around line 156-172: Update set_user_page_target so the provided
UserPageRecord is attached to the get_session() session before mutating target
and pupdate. Load the corresponding row in that session or merge record, apply
the updates to the session-managed instance, then preserve the existing commit,
rollback, logging, and boolean return behavior.
In `@src/db/tools/services/reports/pages_users_to_main_service.py`:
- Around line 35-52: Update add_pages_users_to_main so id is required rather
than defaulting to None, and validate it before creating or committing
PagesUsersToMainRecord. Raise a clear ValueError for a missing id, matching the
upfront validation pattern used by other add_* functions, while preserving the
existing persistence behavior for valid ids.
In `@src/db/tools/services/wikidata/qid_others_service.py`:
- Around line 156-191: Update the validation-exception path in update so the
session rolls back before returning False after orm_obj.title and orm_obj.qid
are mutated. Preserve the existing logging and failure return behavior, ensuring
the invalid changes cannot be committed by get_session() on normal exit.
In `@src/db/tools/services/wikidata/qid_service.py`:
- Around line 153-188: Update the validation-failure path in update() to call
session.rollback() before returning False after orm_obj.validate() raises,
preventing get_session() from committing the invalid title and qid mutations.
Apply the same rollback behavior to qid_others_service.update().
In `@src/md_core_helps/one_time/priorviews/bots/gt_blame.py`:
- Line 22: Update the type annotations and defaults for refnames in
match_ref_names(), FindInHistory.__init__(), and search_history() from list[str]
to an appropriate string-keyed mapping type, preserving the existing keyed
membership and access behavior for values from json_langs_by_langs and
json_en_all.
In `@src/md_core_helps/one_time/wikiblame/bot.py`:
- Line 67: Update the content attribute declaration in __init__ to use the
nullable type str | None, reflecting its initial None state; keep the
response.text assignment unchanged and do not re-annotate self.content at that
assignment.
---
Nitpick comments:
In `@src/db/tools/services/analytics/views_new_service.py`:
- Around line 152-158: Update get_total_views_for_target to perform the total
using a database-side SQL SUM aggregation filtered by target and optional lang,
rather than calling list_views_by_target or materializing ORM records; return
zero when the aggregate result is null.
In `@src/db/tools/services/analytics/word_service.py`:
- Around line 91-107: Restrict the dynamic updates in update_word to an explicit
allow-list of mutable WordRecord fields instead of using hasattr, excluding the
primary key w_id and any other protected attributes. Apply the same allow-list
approach in page_service.update_page and user_page_service.update_user_page,
while preserving existing commit, refresh, and return behavior.
In `@src/db/tools/services/content/project_service.py`:
- Around line 63-89: Restrict the kwargs handled by update_project to an
explicit allow-list of mutable project fields, excluding protected attributes
such as g_id and any primary-key or identity fields. Apply the existing
title/g_title validation only to allowed title fields, and ignore or reject keys
outside the allow-list without assigning them via setattr.
In `@src/db/tools/services/pages/__init__.py`:
- Around line 69-119: Sort the exported names in the __all__ list alphabetically
in __init__.py and the corresponding __all__ lists in in_process_service.py,
pages_users_to_main_service.py, results_2026_service.py, and
translate_type_service.py, without changing their contents.
In `@src/db/tools/services/pages/pages_users_to_main_service.py`:
- Around line 22-26: Remove the unused _row_to_dict helper, since list_pending
currently constructs its result dictionaries directly and does not call it;
leave the existing list_pending conversion behavior unchanged.
In `@src/db/tools/services/pages/translate_type_service.py`:
- Around line 35-38: Update list_new_titles to pass the existing_titles subquery
directly to QidRecord.title.notin_(), removing the unnecessary session.query
wrapper while preserving the existing filtering behavior.
In `@src/db/tools/services/pages/user_page_service.py`:
- Around line 1-125: Extract the duplicated CRUD logic from list_user_pages,
add_user_page, and insert_user_page_target into a shared generic service/factory
parameterized by the ORM model, then reuse it from the corresponding
page_service and qid service modules. Preserve each module’s existing public
function names, signatures, ordering, filtering, and error behavior while
removing the duplicated implementations.
In `@src/db/tools/services/reports/pages_users_to_main_service.py`:
- Around line 55-75: The update_pages_users_to_main function currently permits
arbitrary model attributes, including the primary key, to be modified. Restrict
its kwargs update loop to the explicit updatable fields new_target, new_user,
and new_qid, ignoring or rejecting all other keys before calling setattr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef77e7b1-29a6-459a-a036-0c70b480a232
📒 Files selected for processing (38)
.gitignoresrc/db/mdapi_sql/services/sql_qids.pysrc/db/mdapi_sql/services/sql_qids_others.pysrc/db/mdapi_sql/wikidb.pysrc/db/tools/services/analytics/__init__.pysrc/db/tools/services/analytics/assessment_service.pysrc/db/tools/services/analytics/enwiki_pageview_service.pysrc/db/tools/services/analytics/mdwiki_revid_service.pysrc/db/tools/services/analytics/refs_count_service.pysrc/db/tools/services/analytics/views_new_service.pysrc/db/tools/services/analytics/word_service.pysrc/db/tools/services/content/__init__.pysrc/db/tools/services/content/category_service.pysrc/db/tools/services/content/lang_service.pysrc/db/tools/services/content/project_service.pysrc/db/tools/services/delete_service.pysrc/db/tools/services/pages/__init__.pysrc/db/tools/services/pages/in_process_service.pysrc/db/tools/services/pages/missing_stats_service.pysrc/db/tools/services/pages/page_service.pysrc/db/tools/services/pages/pages_users_to_main_service.pysrc/db/tools/services/pages/results_2026_service.pysrc/db/tools/services/pages/translate_type_service.pysrc/db/tools/services/pages/user_page_service.pysrc/db/tools/services/pages_query_service.pysrc/db/tools/services/reports/__init__.pysrc/db/tools/services/reports/pages_users_to_main_service.pysrc/db/tools/services/utils/__init__.pysrc/db/tools/services/utils/db_guard_model.pysrc/db/tools/services/wikidata/__init__.pysrc/db/tools/services/wikidata/allqid_service.pysrc/db/tools/services/wikidata/qid_others_service.pysrc/db/tools/services/wikidata/qid_service.pysrc/md_core/mdpy/fixref/fixref_text_new.pysrc/md_core/p11143_bot/wd_helps.pysrc/md_core_helps/one_time/priorviews/bots/gt_blame.pysrc/md_core_helps/one_time/wikiblame/bot.pysrc/td_core/fix_user_pages/fix_it_db.py
💤 Files with no reviewable changes (4)
- .gitignore
- src/db/mdapi_sql/wikidb.py
- src/db/mdapi_sql/services/sql_qids_others.py
- src/db/mdapi_sql/services/sql_qids.py
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor