Api v2 delete global task #975
Conversation
…n_alert_filter_is_not_found
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors hook invocation signatures across the codebase to use positional arguments instead of keyword arguments, implements global task deletion functionality, consolidates database deletion operations through a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/app/datamgmt/datastore/datastore_db.py (1)
403-554: datastore_filter_tree now returns a tuple; ensure all callers are updated.
datastore_filter_treenow returns(path_tree, 'Success')and(None, str(e))on error, rather than justpath_tree:except Exception as e: return None, str(e) ... return path_tree, 'Success'This is a public contract change; any existing callers that expect a single return value will now get a tuple and likely break.
Please confirm that:
- All internal call sites have been updated to unpack
(tree, status)(or similar), and- The new status string is handled consistently (or consider returning a richer status object/enum if this is part of a broader pattern).
source/app/blueprints/rest/case/case_assets_routes.py (1)
407-420: Comment deletion misses asset/case binding, can break referential integrity.The new logic:
comment = get_comment(iris_current_user, com_id) if not comment: return response_error('You are not allowed to delete this comment') delete_asset_comment(cur_id, comment) call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid)has good intent (only the author can delete their comment) but misses a key check:
get_commentonly filters bycomment_idandcomment_user_id, not by asset or case.delete_asset_comment(cur_id, comment)(incase_assets_db) assumes the comment is linked to the given asset viaAssetComments. If it isn’t, the association row is not deleted, butdb_delete(comment)still removes thecommentsrow, leading to:
- A likely FK violation if
asset_comments.comment_idhas a foreign key, or- Orphaned
asset_commentsrows if the DB doesn’t enforce referential integrity.A safer pattern is to ensure the comment is both:
- Linked to the specified asset, and
- Owned by the current user.
You already have
get_case_asset_commentimported; we can re-use it:def case_comment_asset_delete(cur_id, com_id, caseid): - comment = get_comment(iris_current_user, com_id) - if not comment: - return response_error('You are not allowed to delete this comment') - - delete_asset_comment(cur_id, comment) - - call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid) + # Ensure the comment belongs to this asset + comment = get_case_asset_comment(cur_id, com_id) + if not comment or comment.comment_user_id != iris_current_user.id: + return response_error('You are not allowed to delete this comment') + + delete_asset_comment(cur_id, comment) + + call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid)This guarantees we only delete comments that are:
- Attached to the asset identified by
cur_id, and- Authored by the current user,
and avoids inconsistent
AssetComments/Commentsstate.
🧹 Nitpick comments (3)
source/app/datamgmt/case/case_db.py (1)
269-278: Use.is_(None)for SQLAlchemy NULL comparisons.Line 277 uses
== Nonewhich can work but SQLAlchemy best practices recommend using.is_(None)for explicit NULL comparisons to ensure proper SQL generation across different database backends.Apply this diff:
return Cases.query.filter( Cases.owner_id == user_identifier, - Cases.close_date == None + Cases.close_date.is_(None) ).all()source/app/blueprints/rest/dashboard_routes.py (1)
36-41: Hook refactor and global task imports look good; minor consistency nit.
- Importing
list_user_cases,list_user_reviews,list_user_tasks, and global task helpers from their dedicated datamgmt modules is cleaner and aligns with the PR’s refactor.- Switching to positional
datawhen callingcall_modules_hookinadd_gtaskandgtask_deleteis correct and matches the helper’s signature.One small consistency nit:
gtask = call_modules_hook( 'on_postload_global_task_update', data=gtask, caseid=caseid)Everywhere else in this file you now pass
datapositionally. You may want to align this call for style/readability:- gtask = call_modules_hook( - 'on_postload_global_task_update', data=gtask, caseid=caseid) + gtask = call_modules_hook( + 'on_postload_global_task_update', gtask, caseid=caseid)Given the endpoint is being deprecated, the existing manual delete logic in
gtask_deleteis acceptable, but if you want to centralize behavior you could also delegate to the new global‑task delete helper later.Also applies to: 203-236, 255-265, 274-295
source/app/datamgmt/global_tasks.py (1)
21-29:delete_global_taskworks but could be more defensive and reuse existing helpers.The new helper:
def delete_global_task(task: GlobalTasks): GlobalTasks.query.filter(GlobalTasks.id == task.id).delete() db.session.commit()is straightforward, but a couple of points to consider:
- It assumes
taskis a non‑NoneGlobalTasksinstance; if a caller passesNone, this will raiseAttributeError. If callers are guaranteed to resolve/validate beforehand (e.g. via a dedicatedget/operationslayer that raisesObjectNotFoundError), that’s fine; otherwise, a defensive early return or explicit exception would make the contract clearer.- Elsewhere in the codebase, deletions are being centralized via
db_delete(element)inapp.datamgmt.db_operations. For consistency you might want to usedb_delete(task)here instead of a bulk query delete, unless you intentionally want to avoid loading the instance.Example defensive variant:
-def delete_global_task(task: GlobalTasks): - GlobalTasks.query.filter(GlobalTasks.id == task.id).delete() - db.session.commit() +def delete_global_task(task: GlobalTasks): + if task is None: + return + + db.session.delete(task) + db.session.commit()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
.vulture.ignore(1 hunks)pyproject.toml(1 hunks)source/app/blueprints/pages/dashboard/dashboard_routes.py(1 hunks)source/app/blueprints/rest/alerts_routes.py(12 hunks)source/app/blueprints/rest/case/case_assets_routes.py(7 hunks)source/app/blueprints/rest/case/case_evidences_routes.py(2 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(4 hunks)source/app/blueprints/rest/case/case_notes_routes.py(4 hunks)source/app/blueprints/rest/case/case_tasks_routes.py(2 hunks)source/app/blueprints/rest/case/case_timeline_routes.py(7 hunks)source/app/blueprints/rest/dashboard_routes.py(6 hunks)source/app/blueprints/rest/dim_tasks_routes.py(1 hunks)source/app/blueprints/rest/manage/manage_cases_routes.py(2 hunks)source/app/blueprints/rest/v2/global_tasks.py(3 hunks)source/app/business/alerts.py(3 hunks)source/app/business/assets.py(2 hunks)source/app/business/cases.py(5 hunks)source/app/business/comments.py(8 hunks)source/app/business/evidences.py(3 hunks)source/app/business/global_tasks.py(3 hunks)source/app/business/iocs.py(3 hunks)source/app/business/notes.py(3 hunks)source/app/business/reports/reports.py(3 hunks)source/app/business/tasks.py(4 hunks)source/app/datamgmt/alerts/alerts_db.py(3 hunks)source/app/datamgmt/case/case_assets_db.py(2 hunks)source/app/datamgmt/case/case_db.py(1 hunks)source/app/datamgmt/case/case_events_db.py(2 hunks)source/app/datamgmt/case/case_iocs_db.py(2 hunks)source/app/datamgmt/case/case_notes_db.py(3 hunks)source/app/datamgmt/case/case_rfiles_db.py(2 hunks)source/app/datamgmt/case/case_tasks_db.py(3 hunks)source/app/datamgmt/client/client_db.py(2 hunks)source/app/datamgmt/comments.py(3 hunks)source/app/datamgmt/datastore/datastore_db.py(4 hunks)source/app/datamgmt/db_operations.py(1 hunks)source/app/datamgmt/global_tasks.py(2 hunks)source/app/datamgmt/iris_engine/modules_db.py(3 hunks)source/app/datamgmt/manage/manage_case_templates_db.py(4 hunks)source/app/datamgmt/manage/manage_groups_db.py(2 hunks)source/app/iris_engine/module_handler/module_handler.py(4 hunks)tests/tests_rest_global_tasks.py(1 hunks)tests/tests_rest_module_tasks.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (40)
source/app/business/notes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
tests/tests_rest_global_tasks.py (2)
source/app/blueprints/rest/v2/global_tasks.py (2)
create(43-51)delete(61-68)tests/iris.py (3)
create(60-61)delete(69-70)get(63-64)
tests/tests_rest_module_tasks.py (1)
tests/iris.py (1)
get_module_identifier_by_name(156-162)
source/app/blueprints/rest/case/case_ioc_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/manage/manage_groups_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/datamgmt/case/case_iocs_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/datamgmt/alerts/alerts_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/business/assets.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_notes_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/datamgmt/comments.py (2)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/comments.py (1)
Comments(33-48)
source/app/datamgmt/case/case_events_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/business/tasks.py (3)
source/app/datamgmt/case/case_tasks_db.py (1)
list_user_tasks(347-373)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/states.py (1)
update_tasks_state(94-95)
source/app/blueprints/rest/case/case_notes_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/blueprints/rest/case/case_timeline_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_tasks_db.py (3)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)source/app/models/models.py (3)
CaseTasks(459-482)TaskStatus(450-456)TaskAssignee(514-524)source/app/models/cases.py (1)
Cases(46-114)
source/app/blueprints/rest/dim_tasks_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_db.py (2)
source/app/models/cases.py (1)
Cases(46-114)source/app/models/models.py (1)
ReviewStatus(688-692)
source/app/blueprints/rest/case/case_evidences_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/iris_engine/module_handler/module_handler.py (1)
source/app/datamgmt/iris_engine/modules_db.py (1)
deregister_module_from_hook(262-269)
source/app/business/alerts.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/iocs.py (2)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_iocs_db.py (1)
delete_ioc(83-96)
source/app/datamgmt/iris_engine/modules_db.py (1)
source/app/models/models.py (2)
IrisModuleHook(614-628)IrisHook(606-611)
source/app/blueprints/pages/dashboard/dashboard_routes.py (1)
source/app/datamgmt/case/case_tasks_db.py (1)
get_tasks_status(41-42)
source/app/business/reports/reports.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/evidences.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/comments.py (2)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_assets_db.py (1)
delete_asset_comment(362-368)
source/app/datamgmt/case/case_rfiles_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/blueprints/rest/v2/global_tasks.py (3)
source/app/blueprints/rest/endpoints.py (3)
response_api_deleted(55-56)response_api_success(29-30)response_api_not_found(72-73)source/app/business/global_tasks.py (2)
global_tasks_get(42-46)global_tasks_delete(49-55)source/app/datamgmt/global_tasks.py (2)
get_global_task(56-79)delete_global_task(27-29)
source/app/datamgmt/manage/manage_case_templates_db.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/blueprints/rest/case/case_assets_routes.py (3)
source/app/datamgmt/comments.py (1)
get_comment(141-145)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_assets_db.py (1)
delete_asset_comment(362-368)
source/app/datamgmt/client/client_db.py (3)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/errors.py (1)
ElementInUseError(49-51)source/app/models/customers.py (1)
Client(32-44)
source/app/blueprints/rest/case/case_tasks_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/global_tasks.py (3)
source/app/datamgmt/global_tasks.py (2)
delete_global_task(27-29)get_global_task_by_identifier(82-85)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/iris_engine/utils/tracker.py (1)
track_activity(28-64)
source/app/blueprints/rest/alerts_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/cases.py (6)
source/app/datamgmt/case/case_db.py (4)
case_db_exists(78-80)list_user_cases(269-278)case_db_save(235-249)list_user_reviews(252-266)source/app/models/cases.py (2)
Cases(46-114)ReviewStatusList(199-204)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/manage/manage_cases_db.py (1)
delete_case(403-458)source/app/iris_engine/utils/tracker.py (1)
track_activity(28-64)source/app/models/errors.py (1)
BusinessProcessingError(22-32)
source/app/datamgmt/datastore/datastore_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/blueprints/rest/dashboard_routes.py (5)
source/app/datamgmt/case/case_db.py (2)
list_user_reviews(252-266)list_user_cases(269-278)source/app/datamgmt/case/case_tasks_db.py (2)
get_tasks_status(41-42)list_user_tasks(347-373)source/app/datamgmt/global_tasks.py (2)
list_global_tasks(32-53)get_global_task(56-79)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/case/case_assets_db.py (3)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/comments.py (2)
Comments(33-48)AssetComments(84-92)source/app/blueprints/rest/v2/assets_routes/comments.py (1)
delete(98-108)
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/global_tasks.py (2)
source/app/models/models.py (2)
GlobalTasks(527-548)TaskStatus(450-456)source/app/blueprints/rest/v2/global_tasks.py (2)
delete_global_task(90-91)delete(61-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Database migration tests
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
- GitHub Check: Test API
- GitHub Check: Test API
- GitHub Check: Database migration tests
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
🔇 Additional comments (61)
source/app/blueprints/pages/dashboard/dashboard_routes.py (1)
27-27: Import refactoring verified as complete.The refactor is properly executed across the codebase. No stray imports from the old
app.datamgmt.dashboard.dashboard_dblocation remain, and all four usages ofget_tasks_statushave been updated to the new location inapp.datamgmt.case.case_tasks_db(dashboard_routes.py, rest/dashboard_routes.py, rest/case/case_tasks_routes.py, and pages/case/case_tasks_routes.py). The function behavior is unchanged and the refactoring aligns with the stated objective to reorganize persistence layer code by domain.source/app/business/alerts.py (1)
88-88: LGTM! Standardized hook calling convention.The migration from keyword arguments to positional arguments for
call_modules_hookinvocations is correct and consistent across all five call sites in this file. The changes align with the function signature and maintain existing behavior.Also applies to: 138-138, 141-141, 144-144, 164-164
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
191-191: LGTM! Hook invocations correctly standardized.The refactor from keyword-argument style (
data=case) to positional-argument passing aligns with the function signature and the project-wide standardization described in the PR objectives. All four invocations correctly pass the payload object as the second positional argument.Also applies to: 225-225, 229-229, 236-236
source/app/business/reports/reports.py (3)
31-31: Preload hook standardization looks good.The change from keyword to positional argument for
report_idaligns with the repository-wide hook invocation standardization.
50-57: Postload hook standardization looks good.The variable rename from
_datatodataand passing it as a positional argument aligns with the standardized calling convention.
63-63: Preload hook standardization looks good.The change from keyword to positional argument for
report_idaligns with the repository-wide hook invocation standardization.source/app/datamgmt/client/client_db.py (1)
25-25: LGTM: Centralized deletion pattern applied correctly.The refactor from direct ORM deletions to the
db_deleteutility maintains identical behavior while improving consistency across the codebase.Also applies to: 144-144, 151-151
source/app/datamgmt/db_operations.py (1)
27-29: LGTM: Clean deletion utility.The
db_deletefunction provides a consistent interface for deletions, mirroring the existingdb_createpattern.source/app/datamgmt/iris_engine/modules_db.py (1)
23-23: LGTM: Centralized logging.Switching to the centralized
loggerimport improves consistency.Also applies to: 245-245
source/app/datamgmt/case/case_rfiles_db.py (1)
23-23: LGTM: Consistent deletion pattern.The refactor correctly applies the centralized
db_deleteutility.Also applies to: 182-182
source/app/datamgmt/manage/manage_groups_db.py (1)
19-19: LGTM: Centralized deletion applied.The refactor correctly uses the
db_deleteutility for group deletion.Also applies to: 209-209
source/app/iris_engine/module_handler/module_handler.py (2)
413-417: LGTM: Helpful documentation.The extended docstring clarifies the purpose of the
caseidparameter in task retrieval.
35-35: Verify transaction commit in deregister_module_from_hook.The refactor delegates hook deregistration to
deregister_module_from_hook, but that function (insource/app/datamgmt/iris_engine/modules_db.pylines 262-269) doesn't commit the deletions. Ensure the missing commit is added.Also applies to: 402-403
source/app/business/iocs.py (1)
59-59: LGTM: Hook invocation standardized.The refactor from keyword (
data=...) to positional arguments forcall_modules_hookaligns with the PR-wide standardization effort.Also applies to: 84-84, 97-97, 101-101
source/app/datamgmt/case/case_tasks_db.py (2)
25-25: LGTM: Centralized deletion pattern.The refactor correctly applies the
db_deleteutility for comment deletion.Also applies to: 316-316
347-373: LGTM: Well-structured query.The function correctly joins multiple tables and filters out completed/canceled tasks.
source/app/business/assets.py (1)
50-50: LGTM! Hook invocation standardization applied correctly.The refactoring consistently updates all three hook calls to use positional arguments instead of keyword
data=arguments, aligning with the broader codebase standardization effort.Also applies to: 62-62, 65-65
source/app/business/tasks.py (2)
26-26: LGTM! Import reorganization improves modularity.Moving
list_user_tasksfromdashboard_dbtocase_tasks_dbproperly consolidates task-related data access functions into their domain-specific module.
41-41: LGTM! Hook invocations standardized correctly.All four hook calls consistently updated to use positional arguments for the data payload, maintaining the
caseidkeyword argument where applicable.Also applies to: 45-45, 57-57, 89-89
source/app/business/evidences.py (1)
38-38: LGTM! Hook invocation refactoring applied consistently.All four evidence lifecycle hooks updated to use positional arguments for the payload, with
caseidproperly passed as a keyword argument where needed.Also applies to: 55-55, 70-70, 72-72
source/app/datamgmt/case/case_notes_db.py (1)
25-25: LGTM! Deletion operations properly centralized.Refactoring to use the
db_deletehelper function standardizes database deletion operations across the codebase, improving maintainability and consistency.Also applies to: 70-70, 431-431
source/app/business/notes.py (1)
65-65: LGTM! Hook calls standardized correctly.All four note lifecycle hooks updated to pass payloads as positional arguments, maintaining consistency with the broader refactoring effort.
Also applies to: 92-92, 107-107, 109-109
source/app/datamgmt/case/case_db.py (1)
252-266: LGTM! User reviews listing function implemented correctly.The function properly filters cases by reviewer and excludes terminal review statuses ('Reviewed', 'Not reviewed'), which aligns with the typical workflow for pending reviews.
source/app/datamgmt/manage/manage_case_templates_db.py (1)
192-192: LGTM! Case template hook invocations standardized.All four hook calls (two for tasks, two for notes) properly updated to use positional arguments for payloads during case template population, maintaining consistency with the codebase refactoring.
Also applies to: 203-203, 224-226, 238-238
.vulture.ignore (1)
1-5: LGTM! External API properly documented.Correctly documents methods used by the
iris-module-interfacepackage, preventing false-positive unused code warnings while maintaining clear documentation of the external dependency.tests/tests_rest_module_tasks.py (1)
31-41: LGTM! Test structure is clear and validates case identifier presence.The test correctly verifies that the case identifier appears in the task list response, which aligns with the PR objective to prevent removal of this field.
source/app/blueprints/rest/alerts_routes.py (1)
372-378: LGTM! Hook invocations correctly refactored to use positional arguments.The changes consistently update
call_modules_hookinvocations from keyword argumentdata=to positional argument, aligning with the function signature and the broader codebase refactoring.source/app/blueprints/rest/case/case_timeline_routes.py (1)
103-103: LGTM! Consistent hook invocation refactoring.All hook calls in this file now use positional arguments for the data parameter, consistent with the standardized calling convention.
source/app/business/comments.py (2)
118-118: LGTM! Hook invocations standardized to positional arguments.All
call_modules_hookinvocations now use positional arguments for the data parameter, consistent with the codebase-wide refactoring.
306-306: LGTM! Updated to match new delete_asset_comment signature.The call now correctly passes the
commentobject instead ofcomment_id, aligning with the updateddelete_asset_commentsignature that expects a Comments object (as seen in case_assets_db.py).pyproject.toml (1)
42-54: LGTM! Import linter contracts refined to match architectural refactoring.The updated contracts appropriately reflect the reorganized module boundaries:
- Contract 1 broadens coverage to all of
app.datamgmtwhile narrowing the forbidden scope toaccess_controls- Contract 2 specifically targets
case_assets_dbto preventiris_userimportsThese changes ensure the persistence layer maintains proper separation from the API layer.
source/app/blueprints/rest/dim_tasks_routes.py (1)
142-142: LGTM! Hook invocation standardized to positional arguments.The change correctly uses positional arguments for
hook_nameandobj_targets, consistent with the standardized calling convention across the codebase.source/app/datamgmt/case/case_events_db.py (2)
21-21: LGTM! Imported db_delete for centralized deletion.Adding
db_deleteto the imports enables the centralized deletion pattern used at line 174.
174-174: LGTM! Deletion centralized through db_delete utility.Replacing direct
db.session.delete()anddb.session.commit()calls withdb_delete(comment)aligns with the codebase-wide refactoring to centralize deletion operations.source/app/datamgmt/case/case_iocs_db.py (2)
21-21: LGTM! Imported db_delete for centralized deletion.Adding
db_deleteto the imports enables the centralized deletion pattern used at line 278.
278-278: LGTM! Deletion centralized through db_delete utility.Replacing direct
db.session.delete()anddb.session.commit()calls withdb_delete(comment)aligns with the codebase-wide refactoring to centralize deletion operations.source/app/datamgmt/comments.py (4)
18-18: LGTM!The Optional type hint is appropriate for the new
get_commentfunction that returnsNonewhen no comment is found.
23-23: LGTM!Importing the centralized
db_deletehelper promotes consistency across the codebase.
110-110: LGTM!Using the centralized
db_deleteutility is a good refactor that ensures consistent deletion behavior across the codebase.
141-145: LGTM!The
get_commentfunction properly scopes comment retrieval to the requesting user, which is important for authorization. The filtering by bothcomment_idandcomment_user_idensures users can only retrieve their own comments.source/app/blueprints/rest/case/case_evidences_routes.py (1)
192-192: LGTM!The refactor to pass hook data as a positional argument instead of using the
data=keyword is consistent with thecall_modules_hooksignature and aligns with similar changes across the codebase.Also applies to: 228-228
source/app/business/global_tasks.py (3)
25-25: LGTM!Importing the necessary functions from the new
global_tasksmodule enables the delete functionality.
36-36: LGTM!Refactoring to pass the global_task as a positional argument is consistent with the standardized calling convention for
call_modules_hook.
49-55: LGTM!The
global_tasks_deletefunction follows the established pattern:
- Pre-delete hook for extension points
- Actual deletion via the persistence layer
- Post-delete hook for notifications
- Activity tracking
This is consistent with similar delete operations elsewhere in the codebase.
source/app/blueprints/rest/case/case_tasks_routes.py (1)
233-233: LGTM!Refactoring hook invocations to use positional arguments aligns with the standardized calling convention across the codebase.
Also applies to: 273-273
tests/tests_rest_global_tasks.py (1)
51-64: LGTM!The new tests provide good coverage for the delete functionality:
test_delete_global_task_should_return_204verifies successful deletion returns the correct HTTP statustest_get_global_task_should_return_404_when_deletedensures deleted tasks are no longer retrievableThese tests follow the existing patterns and validate the expected REST behavior.
source/app/blueprints/rest/case/case_ioc_routes.py (1)
182-182: LGTM!All
call_modules_hookinvocations have been consistently updated to use positional arguments, aligning with the standardized calling convention across the codebase.Also applies to: 194-194, 307-307, 345-345
source/app/blueprints/rest/case/case_notes_routes.py (1)
141-141: LGTM!All hook invocations have been refactored to pass data as positional arguments, maintaining consistency with the standardized calling convention.
Also applies to: 210-210, 429-429, 465-465
source/app/datamgmt/alerts/alerts_db.py (2)
75-75: LGTM!Adding
db_deleteto the imports enables consistent use of the centralized deletion utility.
1285-1285: LGTM!Both deletion call sites now use the centralized
db_deleteutility, promoting consistency across the codebase.Also applies to: 1424-1424
source/app/datamgmt/datastore/datastore_db.py (1)
252-253: db_delete in tight loops changes transactional semantics and may hurt performance.Using
db_deletehere centralizes deletion, but becausedb_deletecommits on each call, the behavior differs from the typical pattern of deleting many rows and committing once:
- In
datastore_iter_deletionanddatastore_delete_files_of_path, you now potentially commit once per path/file.- In
datastore_delete_file, you commit immediately for the single file (which is fine, but different from batching with other work).Implications:
- More round‑trips and contention if many files/paths are deleted in one request.
- Loss of “all‑or‑nothing” behavior when multiple deletions are logically part of one operation (you can end up with some files deleted and others not if a later delete fails).
If the intent is to keep atomic multi‑step deletions, consider:
- Introducing a non‑committing helper (e.g.,
db_delete_nocommit) and doing a singledb.session.commit()at the end of the outer operation, or- Passing a flag to
db_deleteto control committing, and usingcommit=Falsein loops.Example refactor sketch for the loop (conceptual):
- db_delete(dsf_list_item) + db.session.delete(dsf_list_item) ... - return + db.session.commit() + return[uggest_recommended_refactor]
Also applies to: 264-271, 328-343
source/app/blueprints/rest/case/case_assets_routes.py (1)
47-48: Hook invocation refactor to positionaldatais consistent.Across asset creation, bulk upload, update, commenting, and comment delete hooks you’ve switched from
data=...to passing the payload as the second positional argument:
on_preload_asset_create(single and CSV upload)on_postload_asset_createon_preload_asset_updateon_postload_asset_commentedon_postload_asset_comment_deleteThis matches
call_modules_hook(hook_name, data, caseid=...)and keeps the call style consistent with the rest of the codebase. No functional issues spotted here.Also applies to: 170-171, 247-248, 257-258, 309-310, 379-380, 417-418
source/app/blueprints/rest/v2/global_tasks.py (5)
31-35: Imports for delete behavior are appropriate and usedThe added imports
response_api_deletedandglobal_tasks_deleteare correctly referenced in the new delete flow and keep REST and business responsibilities separated.
55-57: read() naming cleanup is correct and behavior-preservingUsing
global_taskfor both retrieval and serialization improves clarity and remains consistent withglobal_tasks_getand the schema dump. No functional issues.
61-68: DELETE operation wiring and error handling look soundThe new
delete()method correctly reusesglobal_tasks_getfor existence checking, delegates deletion toglobal_tasks_delete, returnsresponse_api_deleted()on success, and mapsObjectNotFoundErrortoresponse_api_not_found(). This matches the established pattern and HTTP semantics.
76-85: Route renames to global_task terminology are consistentThe
create_global_taskandget_global_taskendpoints are cleanly mapped to the corresponding operations and keepac_api_requiresin place; naming now aligns with the resource (global_tasks).
88-91: New DELETE /global-tasks/int:identifier endpoint is correctly hooked upThe DELETE route is registered on the same blueprint, uses an
intidentifier consistent withglobal_tasks_get, and delegates toGlobalTasksOperations.deleteunder the same access control decorator. This completes the REST surface for global task deletion.source/app/business/cases.py (4)
31-35: Refactored imports to case_db and models are coherentImporting
case_db_exists,list_user_cases,case_db_save,list_user_reviews,Cases, andReviewStatusListfrom their domain-specific modules matches how they’re used later (existence checks, listing, saving, and review status handling). This improves cohesion of persistence-related logic.Also applies to: 58-59
136-136: on_postload_case_create hook call simplified without behavior change
call_modules_hook('on_postload_case_create', case)drops an unnecessary extra positional argument while still passing the createdcaseas the hook data, consistent with the hook function signature. No functional impact.
152-157: Case delete hooks now use positional data and explicit caseidPassing
case_identifieras the positional data argument andcaseid=case_identifierfor bothon_preload_case_deleteandon_postload_case_deletekeeps hook data explicit and aligned with other callers, while preserving prior behavior (data and caseid were already the same value).
186-191: Alert post-update hooks now use positional data consistentlyBoth
on_postload_alert_updateandon_postload_alert_resolution_updatenow pass thealertobject positionally withcaseid=case.case_id, matching the call_modules_hook contract and broader positional-argument refactor without altering semantics.
| else: | ||
| raise BusinessProcessingError('Report error', data='Unknown report format.') | ||
| call_modules_hook('on_postload_activities_report_create', data=report_id, caseid=caseid) | ||
| call_modules_hook('on_postload_activities_report_create', report_id, caseid=caseid) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify inconsistent data passed to postload hook.
Unlike the investigation report's postload hook (line 57), which passes a data dictionary containing the encoded file, file path, case details, and user name, this activities report postload hook only passes report_id. This inconsistency may cause issues if hooks expect similar data structures or result in missing functionality.
Please verify whether this difference is intentional or if the activities report should also:
- Encode the generated file (similar to lines 47-48)
- Create a data dictionary with report details (similar to lines 50-56)
- Pass that dictionary to the postload hook
Run the following script to check how these hooks are implemented in active modules:
🏁 Script executed:
#!/bin/bash
# Search for implementations of both postload hooks to understand expected data structures
echo "=== Searching for on_postload_report_create implementations ==="
rg -n -A 10 "on_postload_report_create" --type py
echo -e "\n=== Searching for on_postload_activities_report_create implementations ==="
rg -n -A 10 "on_postload_activities_report_create" --type pyLength of output: 3431
🏁 Script executed:
#!/bin/bash
# Get the full context of both functions to understand data structures
echo "=== Context of generate_report function (lines 40-60) ==="
sed -n '40,60p' source/app/business/reports/reports.py
echo -e "\n=== Context of generate_activities_report function (lines 62-82) ==="
sed -n '62,82p' source/app/business/reports/reports.py
echo -e "\n=== Search for actual hook implementations in modules ==="
find source -name "*.py" -type f -exec grep -l "def on_postload_report_create\|def on_postload_activities_report_create" {} \;Length of output: 1949
🏁 Script executed:
#!/bin/bash
# Search more broadly for hook implementations that might not have exact function defs
echo "=== Search for any references to on_postload_activities_report_create ==="
rg -n "on_postload_activities_report_create" --type py -B2 -A2
echo -e "\n=== Check if there are handler patterns in modules dir ==="
find source -type d -name "modules" -o -name "plugins"
echo -e "\n=== Search for call_modules_hook to understand how hooks are invoked ==="
rg -n "def call_modules_hook" --type py -A 5Length of output: 1863
Pass consistent data structure to on_postload_activities_report_create hook.
The activities report postload hook (line 80) should mirror the investigation report's hook (line 57) by passing a data dictionary with report details and encoded file content, rather than just report_id.
Update line 80 to:
- Encode the generated file (similar to lines 47-48)
- Create a data dictionary with report_id, file_path, case_id, user_name, and file
- Pass that dictionary instead of just report_id
Both hooks share the same description ("before download of the document"), confirming they should follow the same pattern for consistency and to enable hooks to access the same contextual information.
🤖 Prompt for AI Agents
In source/app/business/reports/reports.py around line 80, the call to
call_modules_hook('on_postload_activities_report_create', report_id,
caseid=caseid) must be changed to pass the same data dictionary used for the
investigation report hook: after generating the file, base64-encode its bytes
(as done on lines ~47-48), build a dict with keys report_id, file_path, case_id,
user_name, and file (the encoded content), and call
call_modules_hook('on_postload_activities_report_create', data_dict) instead of
passing just report_id so hooks receive the same contextual info.
| def get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: | ||
| return AssetComments.query.filter( | ||
| return Comments.query.join(AssetComments.comment).filter( | ||
| AssetComments.comment_asset_id == asset_id, | ||
| AssetComments.comment_id == comment_id | ||
| ).with_entities( | ||
| Comments.comment_id, | ||
| Comments.comment_text, | ||
| Comments.comment_date, | ||
| Comments.comment_update_date, | ||
| Comments.comment_uuid, | ||
| Comments.comment_user_id, | ||
| Comments.comment_case_id, | ||
| User.name, | ||
| User.user | ||
| ).join( | ||
| AssetComments.comment | ||
| ).join( | ||
| Comments.user | ||
| Comments.comment_id == comment_id | ||
| ).first() |
There was a problem hiding this comment.
🧩 Analysis chain
Comment lookup & deletion need tighter asset binding (potential runtime errors).
Two issues here:
-
Join target in
get_case_asset_comment:Comments.query.join(AssetComments.comment)relies on a relationship defined onAssetComments, while the query’s base entity isComments. This is a non‑standard pattern in SQLAlchemy and may fail or be brittle depending on the SQLAlchemy version and configuration.To make this robust, it’s safer to join explicitly on the association table, mirroring
get_case_asset_comments:-def get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: - return Comments.query.join(AssetComments.comment).filter( - AssetComments.comment_asset_id == asset_id, - Comments.comment_id == comment_id - ).first() +def get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: + return Comments.query.join( + AssetComments, + Comments.comment_id == AssetComments.comment_id, + ).filter( + AssetComments.comment_asset_id == asset_id, + Comments.comment_id == comment_id, + ).first()
-
delete_asset_commentassumes the comment belongs to the given asset:The function deletes links for
(asset_id, comment.comment_id)and then unconditionally callsdb_delete(comment). If the caller passes aCommentsinstance that is not linked to this asset (e.g. wrongasset_id), theAssetComments.delete()will delete 0 rows, butdb_delete(comment)will still attempt to remove thecommentsrow. With a FK fromasset_comments.comment_idtocomments.comment_id, this can cause an integrity error or leave orphans, depending on DB constraints.This is made more likely by the current caller (
case_comment_asset_delete) fetching the comment viaget_comment(iris_current_user, com_id), which does not check the asset or case.I recommend either:
- Enforcing the asset binding at the caller (preferred; see comment on
case_comment_asset_deleteincase_assets_routes.py), so this function is only ever called with a comment linked toasset_id, or - Making the function defensive, e.g.:
def delete_asset_comment(asset_id, comment: Comments): - AssetComments.query.filter( - AssetComments.comment_asset_id == asset_id, - AssetComments.comment_id == comment.comment_id - ).delete() - - db_delete(comment) + deleted_links = AssetComments.query.filter( + AssetComments.comment_asset_id == asset_id, + AssetComments.comment_id == comment.comment_id, + ).delete() + + if not deleted_links: + # Nothing was linked to this asset; avoid deleting the underlying comment + db.session.commit() + return + + db_delete(comment)
This keeps the referential integrity safe even if a caller passes a mismatched asset/comment pair.
- Enforcing the asset binding at the caller (preferred; see comment on
Also applies to: 362-369
🏁 Script executed:
#!/bin/bash
# Examine the case_assets_db.py file around the functions in question
fd case_assets_db.py --type f | head -5Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for model definitions and relationships
fd -e py --type f | xargs rg -l "class AssetComments|class Comments" | head -5Length of output: 254
🏁 Script executed:
#!/bin/bash
# Find case_assets_routes.py to see the caller
fd case_assets_routes.py --type f | head -3Length of output: 108
🏁 Script executed:
#!/bin/bash
# Get context around the functions mentioned
rg -n "def get_case_asset_comment|def delete_asset_comment|def get_case_asset_comments" source/app/datamgmt/case/ -A 10Length of output: 2236
🏁 Script executed:
#!/bin/bash
# Look at the full file structure to understand imports and relationships
cat source/app/datamgmt/case/case_assets_db.py | head -100Length of output: 3358
🏁 Script executed:
#!/bin/bash
# Find model definitions
rg -n "class Comments|class AssetComments" --type py -A 8 | head -60Length of output: 4762
🏁 Script executed:
#!/bin/bash
# Check case_assets_routes.py for the caller mentioned
rg -n "case_comment_asset_delete|get_comment" source/app/datamgmt/case/case_assets_routes.py -B 3 -A 10Length of output: 86
🏁 Script executed:
#!/bin/bash
# Look for the join patterns in both functions
rg -n "\.join\(" source/app/datamgmt/case/case_assets_db.py -B 2 -A 5Length of output: 1638
🏁 Script executed:
#!/bin/bash
# Get full definition of delete_asset_comment
rg -n "def delete_asset_comment" source/app/datamgmt/case/case_assets_db.py -A 20Length of output: 487
🏁 Script executed:
#!/bin/bash
# Get full definition of get_case_asset_comment
rg -n "def get_case_asset_comment" source/app/datamgmt/case/case_assets_db.py -A 10Length of output: 840
🏁 Script executed:
#!/bin/bash
# Check for FK constraints in models
rg -n "ForeignKey|relationship" --type py source/app/datamgmt/ -C 2 | grep -A 5 -B 5 "comment"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at get_case_asset_comments to compare join patterns
rg -n "def get_case_asset_comments" source/app/datamgmt/case/case_assets_db.py -A 15Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check SQLAlchemy version or imports
rg -n "from sqlalchemy|import sqlalchemy" source/app/datamgmt/case/ | head -10Length of output: 733
🏁 Script executed:
#!/bin/bash
# Look for db_delete function definition
rg -n "def db_delete" source/ -A 5Length of output: 198
🏁 Script executed:
#!/bin/bash
# Check the actual AssetComments model structure
fd models.py --type f -path "*/datamgmt/case/*" | xargs cat | head -200Length of output: 168
🏁 Script executed:
#!/bin/bash
# Get all relationship definitions in case module
rg -n "class.*Comments|class.*Asset" source/app/datamgmt/case/ --type py -A 20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the models module
fd -e py -path "*/datamgmt/*" -name models | xargs ls -laLength of output: 2318
🏁 Script executed:
#!/bin/bash
# Search more broadly for model definitions
rg "^class (AssetComments|Comments)" --type py -A 10Length of output: 8588
🏁 Script executed:
#!/bin/bash
# Check imports in case_assets_db.py to find where models come from
head -50 source/app/datamgmt/case/case_assets_db.pyLength of output: 1914
🏁 Script executed:
#!/bin/bash
# Understand the models better - find their definitions
fd models --type f | xargs cat | grep -A 15 "class Comments\|class AssetComments"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Try a different approach - list all Python files in datamgmt
find source/app/datamgmt -name "*.py" -type f | head -20Length of output: 813
🏁 Script executed:
#!/bin/bash
# Look at the structure
ls -la source/app/datamgmt/Length of output: 1411
🏁 Script executed:
#!/bin/bash
# Look for model files
ls -la source/app/datamgmt/case/Length of output: 784
🏁 Script executed:
#!/bin/bash
# Check what's available
pwd && find . -name "case_assets_db.py" 2>/dev/null | head -3Length of output: 63
🏁 Script executed:
#!/bin/bash
# Direct inspection
cat source/app/datamgmt/case/case_assets_db.pyLength of output: 10651
🏁 Script executed:
#!/bin/bash
# If the above fails, check the directory structure
ls -R source/app/datamgmt/ | head -50Length of output: 681
🏁 Script executed:
#!/bin/bash
# Try to find Python files related to comments
find . -name "*.py" -type f | xargs grep -l "class Comments" 2>/dev/null | head -5Length of output: 290
🏁 Script executed:
#!/bin/bash
# Look for the database models in common locations
find . -type f -name "*.py" -path "*/models*" 2>/dev/null | head -10Length of output: 325
🏁 Script executed:
#!/bin/bash
# Check core or shared locations
ls -la source/app/database/ 2>/dev/null || ls -la source/app/db/ 2>/dev/null || ls -la source/app/ | grep -i modelLength of output: 58
🏁 Script executed:
#!/bin/bash
# Basic file structure check
ls -la source/app/ | head -20Length of output: 1186
🏁 Script executed:
#!/bin/bash
# Simplified: just try to read the file in question
cat source/app/datamgmt/case/case_assets_db.py 2>/dev/null | wc -l && echo "---" && cat source/app/datamgmt/case/case_assets_db.py 2>/dev/null | sed -n '350,375p'Length of output: 749
🏁 Script executed:
#!/bin/bash
# Get the functions and surrounding context
python3 << 'PYTHON_EOF'
import os
try:
with open('source/app/datamgmt/case/case_assets_db.py', 'r') as f:
lines = f.readlines()
# Show lines 350-380 to get the functions in context
for i, line in enumerate(lines[349:380], start=350):
print(f"{i}: {line.rstrip()}")
except FileNotFoundError as e:
print(f"File not found: {e}")
# Try to list what exists
for root, dirs, files in os.walk('source/app'):
for f in files:
if 'case' in f.lower() and f.endswith('.py'):
print(os.path.join(root, f))
PYTHON_EOFLength of output: 893
🏁 Script executed:
#!/bin/bash
# Find callers of delete_asset_comment and get_case_asset_comment
rg -n "delete_asset_comment|get_case_asset_comment" source/app/blueprints/ --type py -B 2 -A 5Length of output: 3961
🏁 Script executed:
#!/bin/bash
# Check both case_assets_routes files
cat source/app/blueprints/pages/case/case_assets_routes.py | head -200Length of output: 5375
🏁 Script executed:
#!/bin/bash
# Check REST routes
cat source/app/blueprints/rest/case/case_assets_routes.py | head -200Length of output: 8073
🏁 Script executed:
#!/bin/bash
# Search for get_comment function used in caller context
rg -n "def get_comment|\.get_comment\(" source/app/ --type py -B 2 -A 3Length of output: 395
🏁 Script executed:
#!/bin/bash
# Look at the full models/comments.py file to see relationships and FK definitions
cat source/app/models/comments.pyLength of output: 3650
🏁 Script executed:
#!/bin/bash
# Check if there's SQLAlchemy documentation or version info
cat source/app/db.pyLength of output: 1166
🏁 Script executed:
#!/bin/bash
# Search for ondelete cascade or FK constraints in models
rg -n "ondelete|cascade|ForeignKey" source/app/models/comments.py -A 2 -B 2Length of output: 1832
🏁 Script executed:
#!/bin/bash
# Look for any tests that might call these functions
find . -name "*test*" -type f -path "*/case*" -o -path "*/asset*" | grep -i test | head -10Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search the datamgmt comments module
cat source/app/datamgmt/case/case_comments.pyLength of output: 1239
🏁 Script executed:
#!/bin/bash
# Look for any usage patterns of the problematic functions
rg -rn "get_case_asset_comment\|delete_asset_comment" source/app/ --type pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Test SQLAlchemy join syntax validity - check imports and version
grep -r "sqlalchemy" source/app/db.py source/app/models/comments.py | head -20Length of output: 645
🏁 Script executed:
#!/bin/bash
# Extract full comments.py file
wc -l source/app/models/comments.py && head -150 source/app/models/comments.pyLength of output: 3684
🏁 Script executed:
#!/bin/bash
# Get the relationships and FK info from AssetComments specifically
sed -n '84,93p' source/app/models/comments.pyLength of output: 319
🏁 Script executed:
#!/bin/bash
# Check pyproject.toml for SQLAlchemy version
grep -i sqlalchemy pyproject.tomlLength of output: 176
🏁 Script executed:
#!/bin/bash
# Search for where delete_asset_comment is actually called
rg "delete_asset_comment" source/ --type py -B 5 -A 10Length of output: 6994
🏁 Script executed:
#!/bin/bash
# Search for where get_case_asset_comment is called
rg "get_case_asset_comment" source/ --type py -B 5 -A 10Length of output: 10101
🏁 Script executed:
#!/bin/bash
# Look for comment delete routes more broadly
rg "delete.*comment|comment.*delete" source/app/blueprints/ --type py -lLength of output: 758
🏁 Script executed:
#!/bin/bash
# Examine the assets routes more carefully
rg -n "comment" source/app/blueprints/pages/case/case_assets_routes.pyLength of output: 416
🏁 Script executed:
#!/bin/bash
# Check REST routes for assets
rg -n "comment" source/app/blueprints/rest/case/case_assets_routes.pyLength of output: 2992
🏁 Script executed:
#!/bin/bash
# Look for all asset route files
fd case_assets -type f --path "*/blueprints/*"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Test if the join syntax is valid by checking SQLAlchemy behavior
python3 << 'PYEOF'
# Check what the join statement does
test_code = """Comments.query.join(AssetComments.comment).filter(...)"""
print("Analyzing join pattern:")
print(test_code)
print("\nThis attempts to join on a relationship property.")
print("SQLAlchemy behavior: relationship properties can be used as join targets")
print("However, when base entity is Comments and you join on AssetComments.comment (rel to Comments),")
print("this creates: Comments JOIN AssetComments ON <relationship>, which can be ambiguous")
PYEOFLength of output: 381
🏁 Script executed:
#!/bin/bash
# Final check: look for any configuration or dialect-specific code
rg "dialect|engine|bind" source/app/db.py | head -20Length of output: 77
Fix join pattern and add validation before deleting comments.
Two issues confirmed:
-
Join pattern (lines 355-359): Inconsistent with
get_case_asset_comments. Use explicit join on the association table instead of relationship navigation:def get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: - return Comments.query.join(AssetComments.comment).filter( + return Comments.query.join( + AssetComments, + Comments.comment_id == AssetComments.comment_id, + ).filter( AssetComments.comment_asset_id == asset_id, Comments.comment_id == comment_id ).first()
-
Deletion without validation (lines 362-368): Route caller fetches comment via
get_comment(user, com_id), which only validates user ownership, not asset binding. If a mismatched asset/comment pair reachesdelete_asset_comment, theAssetCommentsdelete finds no rows butdb_delete(comment)still executes, potentially orphaning the comment. Add defensive validation:def delete_asset_comment(asset_id, comment: Comments): - AssetComments.query.filter( + deleted_links = AssetComments.query.filter( AssetComments.comment_asset_id == asset_id, AssetComments.comment_id == comment.comment_id - ).delete() + ).delete() + + if not deleted_links: + db.session.commit() + return db_delete(comment)
📝 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 get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: | |
| return AssetComments.query.filter( | |
| return Comments.query.join(AssetComments.comment).filter( | |
| AssetComments.comment_asset_id == asset_id, | |
| AssetComments.comment_id == comment_id | |
| ).with_entities( | |
| Comments.comment_id, | |
| Comments.comment_text, | |
| Comments.comment_date, | |
| Comments.comment_update_date, | |
| Comments.comment_uuid, | |
| Comments.comment_user_id, | |
| Comments.comment_case_id, | |
| User.name, | |
| User.user | |
| ).join( | |
| AssetComments.comment | |
| ).join( | |
| Comments.user | |
| Comments.comment_id == comment_id | |
| ).first() | |
| def get_case_asset_comment(asset_id, comment_id) -> Optional[Comments]: | |
| return Comments.query.join( | |
| AssetComments, | |
| Comments.comment_id == AssetComments.comment_id, | |
| ).filter( | |
| AssetComments.comment_asset_id == asset_id, | |
| Comments.comment_id == comment_id | |
| ).first() |
🤖 Prompt for AI Agents
In source/app/datamgmt/case/case_assets_db.py around lines 355-368, the
get_case_asset_comment uses relationship navigation in the join and should
instead join explicitly on the association table (AssetComments) like
get_case_asset_comments; change the join to join(AssetComments,
AssetComments.comment_id == Comments.comment_id) and filter on
AssetComments.comment_asset_id and Comments.comment_id. Also add defensive
validation in the delete path (lines ~362-368): after loading the Comment (via
get_comment) verify an associated AssetComments row exists for that asset_id and
comment_id; if no association exists, abort/raise and do not call
db_delete(comment) to avoid orphaning the comment; only delete both association
and comment when the validation passes.
| def update_utask_status(task_id, status, case_id): | ||
| if task_id != 0: | ||
| task = CaseTasks.query.filter( | ||
| CaseTasks.id == task_id, | ||
| CaseTasks.task_case_id == case_id | ||
| ).first() | ||
| if task: | ||
| try: | ||
| task.task_status_id = status | ||
|
|
||
| db.session.commit() | ||
| return True | ||
|
|
||
| except: | ||
| pass | ||
|
|
||
| return False |
There was a problem hiding this comment.
Replace bare except with specific exception handling.
The bare except: clause (lines 389-390) silently swallows all exceptions, including KeyboardInterrupt and SystemExit, making debugging difficult and potentially masking critical errors.
Apply this diff:
def update_utask_status(task_id, status, case_id):
if task_id != 0:
task = CaseTasks.query.filter(
CaseTasks.id == task_id,
CaseTasks.task_case_id == case_id
).first()
if task:
try:
task.task_status_id = status
db.session.commit()
return True
- except:
- pass
+ except Exception as e:
+ logger.exception(f"Failed to update task {task_id} status: {e}")
+ db.session.rollback()
return FalseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In source/app/datamgmt/case/case_tasks_db.py around lines 376 to 392, replace
the bare "except:" that swallows all errors with specific exception handling:
catch broad runtime/database exceptions (e.g., Exception or SQLAlchemyError) as
a variable, call db.session.rollback() on error, log or re-raise the caught
exception (use the logger or raise after logging) and then return False; ensure
KeyboardInterrupt/SystemExit are not swallowed by not using a bare except.
| def deregister_module_from_hook(module_id: int, iris_hook_name: str): | ||
| hooks = IrisModuleHook.query.filter( | ||
| IrisModuleHook.module_id == module_id, | ||
| IrisHook.hook_name == iris_hook_name, | ||
| IrisModuleHook.hook_id == IrisHook.id | ||
| ).all() | ||
| for hook in hooks: | ||
| db.session.delete(hook) |
There was a problem hiding this comment.
Missing commit after deletions.
The function deletes IrisModuleHook records in a loop but never commits the transaction. Deletions won't persist to the database.
Apply this diff:
def deregister_module_from_hook(module_id: int, iris_hook_name: str):
hooks = IrisModuleHook.query.filter(
IrisModuleHook.module_id == module_id,
IrisHook.hook_name == iris_hook_name,
IrisModuleHook.hook_id == IrisHook.id
).all()
for hook in hooks:
db.session.delete(hook)
+ db.session.commit()📝 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 deregister_module_from_hook(module_id: int, iris_hook_name: str): | |
| hooks = IrisModuleHook.query.filter( | |
| IrisModuleHook.module_id == module_id, | |
| IrisHook.hook_name == iris_hook_name, | |
| IrisModuleHook.hook_id == IrisHook.id | |
| ).all() | |
| for hook in hooks: | |
| db.session.delete(hook) | |
| def deregister_module_from_hook(module_id: int, iris_hook_name: str): | |
| hooks = IrisModuleHook.query.filter( | |
| IrisModuleHook.module_id == module_id, | |
| IrisHook.hook_name == iris_hook_name, | |
| IrisModuleHook.hook_id == IrisHook.id | |
| ).all() | |
| for hook in hooks: | |
| db.session.delete(hook) | |
| db.session.commit() |
🤖 Prompt for AI Agents
In source/app/datamgmt/iris_engine/modules_db.py around lines 262 to 269, the
function deregister_module_from_hook deletes IrisModuleHook rows in a loop but
never commits the transaction; add a db.session.commit() after the deletion loop
(or replace the loop with a single query delete and then commit) so the
deletions are persisted to the database.
Implementation of
DELETE /api/v2/global-tasks/{identifier}POST /global/tasks/delete/<int:cur_id>.vulture.ignoreapp.datamgmt.dashboard.dashboard_dbinto other files of the persistence layer to group requests by tablecall_modules_hookGET /dim/tasks/list/{count}to ensure the case identifier is present in the response (and prevent a code cleanup I had wanted to perform)This PR goes with the documentation PR#98 in
iris-doc-src.Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests