refactor(api-tools): decouple db.session in ToolLabelManager, ApiToolManageService, WorkflowToolManageService#34549
Closed
HeYin-OS wants to merge 16 commits intolanggenius:mainfrom
Conversation
dosubot
Bot
added
size:S
HeYin-OS
changed the title
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-05 00:13:16.113189942 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-05 00:13:07.578209624 +0000
@@ -343,6 +343,8 @@
--> services/document_indexing_proxy/duplicate_document_indexing_task_proxy.py:15:5
ERROR `handled_tenant_count` was assigned in the current scope before the nonlocal declaration [unknown-name]
--> services/plugin/plugin_migration.py:81:34
+ERROR Default `None` is not assignable to parameter `session` with type `Session` [bad-function-definition]
+ --> services/tools/api_tools_manage_service.py:327:103
ERROR Object of class `dict` has no attribute `encode`
ERROR Object of class `dict` has no attribute `encode`
ERROR Returned type `EndUser | Unknown | None` is not assignable to declared return type `Account | EndUser` [bad-return]
|
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
4 times, most recently
from
84200db to
18afa10
Compare
dosubot
Bot
added
size:L
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
from
18afa10 to
53e67b1
Compare
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-07 04:39:36.592994940 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-07 04:39:27.652871677 +0000
@@ -1110,17 +1110,17 @@
ERROR Argument `Literal['end_user']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.web.SavedMessage.__init__` [bad-argument-type]
--> tests/test_containers_integration_tests/services/test_saved_message_service.py:602:67
ERROR Argument `Literal['active']` is not assignable to parameter `status` with type `AccountStatus | SQLCoreOperations[AccountStatus]` in function `models.account.Account.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:59:20
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:53:20
ERROR Argument `Literal['normal']` is not assignable to parameter `status` with type `SQLCoreOperations[TenantStatus] | TenantStatus` in function `models.account.Tenant.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:68:20
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:62:20
ERROR Argument `Literal['app']` is not assignable to parameter `type` with type `SQLCoreOperations[TagType] | TagType` in function `models.model.Tag.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:336:18
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:330:18
ERROR Argument `Literal['app']` is not assignable to parameter `type` with type `SQLCoreOperations[TagType] | TagType` in function `models.model.Tag.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:345:18
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:339:18
ERROR Argument `Literal['app']` is not assignable to parameter `type` with type `SQLCoreOperations[TagType] | TagType` in function `models.model.Tag.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:354:18
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:348:18
ERROR Argument `Literal['app']` is not assignable to parameter `type` with type `SQLCoreOperations[TagType] | TagType` in function `models.model.Tag.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_tag_service.py:364:18
+ --> tests/test_containers_integration_tests/services/test_tag_service.py:358:18
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> tests/test_containers_integration_tests/services/test_trigger_provider_service.py:169:13
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ApiToolManageService to support optional SQLAlchemy session injection so callers can control transaction scope (instead of always using and committing db.session).
Changes:
- Added optional
session: Session | None = Noneparameters to several service methods and routed queries/mutations through_session = session or db.session. - Changed internal commit behavior to only auto-commit when
sessionis not provided. - Expanded method docstrings to include parameter/return descriptions.
Comments suppressed due to low confidence (1)
api/services/tools/api_tools_manage_service.py:366
- Same transaction issue as in create_api_tool_provider: ToolLabelManager.update_tool_labels() always uses db.session and commits, so passing an explicit session here will not actually give the caller transaction control (and may commit mid-transaction). To make session injection effective, propagate the session through label updates (and avoid internal commits when an external session is supplied).
_session.add(provider)
if not session:
_session.commit()
# delete cache
cache.delete()
# update labels
ToolLabelManager.update_tool_labels(provider_controller, labels)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
from
1de7fe2 to
8653eaf
Compare
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
2 times, most recently
from
fb9a358 to
780dc4e
Compare
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
2 times, most recently
from
516b435 to
7176840
Compare
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-08 03:54:45.566146959 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-08 03:54:37.645133389 +0000
@@ -347,6 +347,14 @@
--> services/hit_testing_service.py:94:21
ERROR `handled_tenant_count` was assigned in the current scope before the nonlocal declaration [unknown-name]
--> services/plugin/plugin_migration.py:91:34
+ERROR Argument `sqlalchemy.orm.session.Session | scoped_session[flask_sqlalchemy.session.Session]` is not assignable to parameter `session` with type `sqlalchemy.orm.session.Session | None` in function `core.tools.tool_label_manager.ToolLabelManager.update_tool_labels` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:200:86
+ERROR Argument `sqlalchemy.orm.session.Session | scoped_session[flask_sqlalchemy.session.Session]` is not assignable to parameter `session` with type `sqlalchemy.orm.session.Session | None` in function `core.tools.tool_label_manager.ToolLabelManager.get_tool_labels` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:264:71
+ERROR Argument `sqlalchemy.orm.session.Session | scoped_session[flask_sqlalchemy.session.Session]` is not assignable to parameter `session` with type `sqlalchemy.orm.session.Session | None` in function `core.tools.tool_label_manager.ToolLabelManager.update_tool_labels` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:369:86
+ERROR Argument `sqlalchemy.orm.session.Session | scoped_session[flask_sqlalchemy.session.Session]` is not assignable to parameter `session` with type `sqlalchemy.orm.session.Session | None` in function `core.tools.tool_label_manager.ToolLabelManager.get_tool_labels` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:554:84
ERROR Object of class `dict` has no attribute `encode`
ERROR Object of class `dict` has no attribute `encode`
ERROR Returned type `EndUser | Unknown | None` is not assignable to declared return type `Account | EndUser` [bad-return]
|
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-08 11:02:21.617645268 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-08 11:02:12.089569717 +0000
@@ -293,6 +293,10 @@
--> core/tools/mcp_tool/provider.py:33:14
ERROR Class member `PluginToolProviderController.entity` overrides parent class `BuiltinToolProviderController` in an inconsistent manner [bad-override]
--> core/tools/plugin_tool/provider.py:12:5
+ERROR `Sequence[str]` is not assignable to variable `labels` with type `list[str]` [bad-assignment]
+ --> core/tools/tool_label_manager.py:81:22
+ERROR `Sequence[ToolLabelBinding]` is not assignable to variable `labels` with type `list[ToolLabelBinding]` [bad-assignment]
+ --> core/tools/tool_label_manager.py:110:22
ERROR `(method: str, url: str, max_retries: int = ..., **kwargs: Any) -> httpx._models.Response` is not assignable to attribute `perform_request` with type `(self: CloudScraper, method: Unknown, url: Unknown, *args: Unknown, **kwargs: Unknown) -> requests.models.Response` [bad-assignment]
--> core/tools/utils/web_reader_tool.py:66:35
ERROR `list[Never]` is not assignable to attribute `tools` with type `Never` [bad-assignment]
@@ -347,6 +351,14 @@
--> services/hit_testing_service.py:94:21
ERROR `handled_tenant_count` was assigned in the current scope before the nonlocal declaration [unknown-name]
--> services/plugin/plugin_migration.py:91:34
+ERROR Argument `ApiToolProvider | None` is not assignable to parameter `db_provider` with type `ApiToolProvider` in function `core.tools.custom_tool.provider.ApiToolProviderController.from_db` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:483:65
+ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
+ --> services/tools/api_tools_manage_service.py:488:12
+ERROR `Sequence[ApiToolProvider]` is not assignable to variable `providers` with type `list[ApiToolProvider]` [bad-assignment]
+ --> services/tools/api_tools_manage_service.py:528:25
+ERROR `Sequence[WorkflowToolProvider]` is not assignable to variable `providers` with type `list[WorkflowToolProvider]` [bad-assignment]
+ --> services/tools/workflow_tools_manage_service.py:244:25
ERROR Object of class `dict` has no attribute `encode`
ERROR Object of class `dict` has no attribute `encode`
ERROR Returned type `EndUser | Unknown | None` is not assignable to declared return type `Account | EndUser` [bad-return]
|
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-08 11:05:31.395346328 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-08 11:05:23.237308875 +0000
@@ -293,6 +293,10 @@
--> core/tools/mcp_tool/provider.py:33:14
ERROR Class member `PluginToolProviderController.entity` overrides parent class `BuiltinToolProviderController` in an inconsistent manner [bad-override]
--> core/tools/plugin_tool/provider.py:12:5
+ERROR `Sequence[str]` is not assignable to variable `labels` with type `list[str]` [bad-assignment]
+ --> core/tools/tool_label_manager.py:72:22
+ERROR `Sequence[ToolLabelBinding]` is not assignable to variable `labels` with type `list[ToolLabelBinding]` [bad-assignment]
+ --> core/tools/tool_label_manager.py:101:22
ERROR `(method: str, url: str, max_retries: int = ..., **kwargs: Any) -> httpx._models.Response` is not assignable to attribute `perform_request` with type `(self: CloudScraper, method: Unknown, url: Unknown, *args: Unknown, **kwargs: Unknown) -> requests.models.Response` [bad-assignment]
--> core/tools/utils/web_reader_tool.py:66:35
ERROR `list[Never]` is not assignable to attribute `tools` with type `Never` [bad-assignment]
@@ -347,6 +351,14 @@
--> services/hit_testing_service.py:94:21
ERROR `handled_tenant_count` was assigned in the current scope before the nonlocal declaration [unknown-name]
--> services/plugin/plugin_migration.py:91:34
+ERROR Argument `ApiToolProvider | None` is not assignable to parameter `db_provider` with type `ApiToolProvider` in function `core.tools.custom_tool.provider.ApiToolProviderController.from_db` [bad-argument-type]
+ --> services/tools/api_tools_manage_service.py:483:65
+ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
+ --> services/tools/api_tools_manage_service.py:488:12
+ERROR `Sequence[ApiToolProvider]` is not assignable to variable `providers` with type `list[ApiToolProvider]` [bad-assignment]
+ --> services/tools/api_tools_manage_service.py:528:25
+ERROR `Sequence[WorkflowToolProvider]` is not assignable to variable `providers` with type `list[WorkflowToolProvider]` [bad-assignment]
+ --> services/tools/workflow_tools_manage_service.py:226:25
ERROR Object of class `dict` has no attribute `encode`
ERROR Object of class `dict` has no attribute `encode`
ERROR Returned type `EndUser | Unknown | None` is not assignable to declared return type `Account | EndUser` [bad-return]
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
api/services/tools/api_tools_manage_service.py:1
- The condition that decides whether to create a “fake” provider is inverted. Currently, when a provider exists in DB (
provider is not None), it gets overwritten with an empty placeholder, which breaks preview (e.g., skips decrypting stored credentials becauseprovider.idbecomes empty). This should only create a fake provider when the DB provider is not found (provider is None).
import json
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
170
| workflow_tool_provider: WorkflowToolProvider | None = None | ||
| with sessionmaker(db.engine, expire_on_commit=False).begin() as _session: | ||
| workflow_tool_provider = _session.scalar( | ||
| select(WorkflowToolProvider) | ||
| .where(WorkflowToolProvider.tenant_id == tenant_id, WorkflowToolProvider.id == workflow_tool_id) | ||
| .limit(1) | ||
| ) | ||
|
|
||
| # if not found raise error | ||
| if workflow_tool_provider is None: | ||
| raise ValueError(f"Tool {workflow_tool_id} not found") | ||
|
|
There was a problem hiding this comment.
The provider is loaded inside a session context and then mutated after that session is closed, but there is no subsequent merge/add + commit in an active session (the previous db.session.commit() was removed). As a result, updates will not be persisted. Fix by performing the mutations inside the same sessionmaker(...).begin() block where the provider is loaded, or by attaching the detached instance to a new session via merge() (or re-querying it) before committing.
Comment on lines
192
to
195
| # update workflow tool provider | ||
| workflow_tool_provider.name = name | ||
| workflow_tool_provider.label = label | ||
| workflow_tool_provider.icon = json.dumps(icon) |
There was a problem hiding this comment.
The provider is loaded inside a session context and then mutated after that session is closed, but there is no subsequent merge/add + commit in an active session (the previous db.session.commit() was removed). As a result, updates will not be persisted. Fix by performing the mutations inside the same sessionmaker(...).begin() block where the provider is loaded, or by attaching the detached instance to a new session via merge() (or re-querying it) before committing.
|
|
||
| # create provider entity | ||
| provider_controller = ApiToolProviderController.from_db(db_provider, auth_type) | ||
| provider_controller = ApiToolProviderController.from_db(provider, auth_type) |
There was a problem hiding this comment.
The condition that decides whether to create a “fake” provider is inverted. Currently, when a provider exists in DB (provider is not None), it gets overwritten with an empty placeholder, which breaks preview (e.g., skips decrypting stored credentials because provider.id becomes empty). This should only create a fake provider when the DB provider is not found (provider is None).
|
|
||
| # decrypt credentials | ||
| if db_provider.id: | ||
| if provider.id: |
There was a problem hiding this comment.
The condition that decides whether to create a “fake” provider is inverted. Currently, when a provider exists in DB (provider is not None), it gets overwritten with an empty placeholder, which breaks preview (e.g., skips decrypting stored credentials because provider.id becomes empty). This should only create a fake provider when the DB provider is not found (provider is None).
| :param privacy_policy: The privacy policy URL or text. | ||
| :param custom_disclaimer: Custom disclaimer text. | ||
| :param labels: A list of labels for the provider. | ||
| :param session: Optional SQLAlchemy session. |
There was a problem hiding this comment.
The docstring documents a :param session: parameter, but the function signature does not accept a session argument. This is misleading for callers and for generated docs; either remove the docstring param entry or add an actual optional session parameter and honor it (so callers can control transaction scope).
Suggested change
| :param session: Optional SQLAlchemy session. |
Comment on lines
+422
to
435
| ) -> dict[str, Any]: | ||
| """ | ||
| test api tool before adding api tool provider | ||
| Test an API tool before adding the API tool provider. | ||
|
|
||
| :param tenant_id: The ID of the workspace/tenant. | ||
| :param provider_name: The name of the API tool provider. | ||
| :param tool_name: The name of the specific tool to test. | ||
| :param credentials: The credentials for the provider. | ||
| :param parameters: The parameters to pass to the tool. | ||
| :param schema_type: The type of schema (e.g., OpenAPI). | ||
| :param schema: The raw schema string. | ||
| :param session: Optional SQLAlchemy session. | ||
| :return: A dictionary containing the result or error message. | ||
| """ |
There was a problem hiding this comment.
Similar to update_api_tool_provider, this docstring references a session parameter that is not present in the function signature. Please remove it or introduce an optional session argument and use it to avoid forcing internal transaction boundaries.
Comment on lines
+100
to
107
| with sessionmaker(db.engine).begin() as _session: | ||
| _session.add(workflow_tool_provider) | ||
|
|
||
| # keep the session open to make orm instances in the same session | ||
| if labels is not None: | ||
| ToolLabelManager.update_tool_labels( | ||
| ToolTransformService.workflow_provider_to_controller(workflow_tool_provider), labels | ||
| ) |
There was a problem hiding this comment.
The comment at line 103 is incorrect: the session is not kept open here (the context manager has already exited). Additionally, this sessionmaker(db.engine) call is inconsistent with the rest of the file (no expire_on_commit=False), which makes it easier to introduce subtle detached/expired ORM attribute issues when workflow_tool_provider is used after commit. Fix by either (a) moving label updates into the same session/transaction (or passing the provider id rather than a detached ORM object), and (b) standardizing sessionmaker configuration (e.g., one module-level factory with consistent expire_on_commit behavior).
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
from
f19a4d0 to
cbfc1a7
Compare
dosubot
Bot
added
size:XL
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
from
86cce6e to
383c30f
Compare
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-09 03:19:22.542957956 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-09 03:19:14.080932987 +0000
@@ -298,7 +298,7 @@
ERROR Returned type `dict[object, str]` is not assignable to declared return type `dict[str, Any] | None` [bad-return]
--> core/workflow/human_input_compat.py:183:16
ERROR No matching overload found for function `redis.client.Redis.__init__` called with arguments: (connection_pool=ConnectionPool) [no-matching-overload]
- --> extensions/ext_redis.py:282:38
+ --> extensions/ext_redis.py:244:38
ERROR Cannot index into `Literal['']` [bad-index]
--> extensions/storage/huawei_obs_storage.py:27:23
ERROR Cannot index into `Literal['']` [bad-index]
@@ -6055,8 +6055,6 @@
--> tests/unit_tests/extensions/otel/test_celery_sqlcommenter.py:139:20
ERROR Cannot index into `object` [bad-index]
--> tests/unit_tests/extensions/otel/test_celery_sqlcommenter.py:140:20
-ERROR Object of class `Retry` has no attribute `_retries` [missing-attribute]
- --> tests/unit_tests/extensions/test_redis.py:31:16
ERROR Argument `dict[str, bytes | str]` is not assignable to parameter `headers` with type `Headers | Mapping[bytes, bytes] | Mapping[str, str] | Sequence[tuple[bytes, bytes]] | Sequence[tuple[str, str]] | None` in function `httpx._models.Response.__init__` [bad-argument-type]
--> tests/unit_tests/factories/test_build_from_mapping.py:75:21
ERROR Object of class `NoneType` has no attribute `storage_key` [missing-attribute]
|
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
3 times, most recently
from
981c992 to
b43501d
Compare
…ion atomicity in ToolLabelManager
…on atomicity in ToolLabelManager
…on atomicity in WorkflowToolManageService
…py` due to refactoring engineering on `db.session -> sessionmaker`
HeYin-OS
force-pushed
the
refactor/api-tools-service/delete-session-decoupling
branch
from
b43501d to
ada962d
Compare
HeYin-OS
deleted the
refactor/api-tools-service/delete-session-decoupling
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#24115
Important
Ref #<issue number>.Summary
Motivation & Context:
Currently,
ToolLabelManager, ApiToolManageService, WorkflowToolManageServicetightly couples database operations with the globaldb.sessionand auto-commits. This prevents external transaction control (e.g., executing multiple service methods within a single atomic transaction) and makes unit testing difficult without a real database connection.Changes in this Draft PR:
with sessionmakerScreenshots
N/A - Backend architectural refactoring only.
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods