[yuee3]

Summary

ApiExternalDataTool.validate_config and query each repeated the APIBasedExtension lookup by reaching for the global db.session inside the query block. Each method now reads the session into a local (session: Session | scoped_session = db.session) and runs the lookup through it, so the session a call uses is visible at the call site.

Refs #37403

Review follow-up

• The _get_api_based_extension helper is removed and the lookup is inlined in both methods.
• api/tests/unit_tests/core/external_data_tool/api/test_api.py is back to its main state. The helper-mocking tests patched the removed method, which is what failed the API unit shards; main's tests drive the real SQLite session through the module-level db binding and cover the inlined calls directly.

Checklist

• This important change requires a documentation update
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

No new tests: the inlined calls are covered by the existing test_api.py suite.

Screenshots or Videos

N/A

---

From Claude Code

[github-actions]

Pyrefly Type Coverage

Metric	Base	PR	Delta
Type coverage	83.00%	83.00%	+0.00%
Strict coverage	82.66%	82.66%	+0.00%
Typed symbols	85,599	85,612	+13
Untyped symbols	17,602	17,602	0
Modules	4373	4373	0

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.79%. Comparing base (add8fce) to head (a780c1a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #42774       +/-   ##
===========================================
+ Coverage   68.81%   89.79%   +20.97%     
===========================================
  Files        6103     6239      +136     
  Lines      368966   390931    +21965     
  Branches    77839    79283     +1444     
===========================================
+ Hits       253920   351045    +97125     
+ Misses     112237    33787    -78450     
- Partials     2809     6099     +3290     

Flag	Coverage Δ
api	89.63% <100.00%> (+37.29%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[yuee3]

Pushed 446a664 addressing both review comments: the _get_api_based_extension helper is gone, and the session is introduced directly at both call sites (session: Session | scoped_session = db.session, then session.scalar(stmt)), keeping the explicit-session pattern of #37403 without the extra indirection. Diff against main is now just the import, two session = db.session lines, and the two session.scalar(stmt) calls.

[github-actions]

Pyrefly Diff

base → PR

--- /tmp/pyrefly_base.txt	2026-10-20 03:20:08.455272740 +0000
+++ /tmp/pyrefly_pr.txt	2026-10-20 03:19:59.049235588 +0000
@@ -3012,6 +3012,10 @@
    --> tests/unit_tests/core/entities/test_entities_provider_configuration.py:974:23
 ERROR Argument `str | None` is not assignable to parameter `s` with type `bytearray | bytes | str` in function `json.loads` [bad-argument-type]
     --> tests/unit_tests/core/entities/test_entities_provider_configuration.py:1174:23
+ERROR Could not find name `sqlite_session` [unknown-name]
+   --> tests/unit_tests/core/external_data_tool/api/test_api.py:130:28
+ERROR Could not find name `sqlite_session` [unknown-name]
+   --> tests/unit_tests/core/external_data_tool/api/test_api.py:147:28
 ERROR Argument `list[FromClause]` is not assignable to parameter `tables` with type `Sequence[Table] | None` in function `sqlalchemy.sql.schema.MetaData.create_all` [bad-argument-type]
   --> tests/unit_tests/core/file/test_remote_fetcher.py:41:58
 ERROR Class member `_ModelInstance.invoke_llm` overrides parent class `ModelInstance` in an inconsistent manner [bad-override]

[yuee3]

Pushed c98f054: reverted tests/.../test_api.py to main.

The pyrefly errors came from my helper version of the change — the test updates mocked ApiExternalDataTool._get_api_based_extension and even queried it directly, which was coupling the tests to the helper this PR no longer has.

With the session inlined at the call sites, main's own test file (real SQLite via the _DatabaseBinding fixture) exercises exactly the new behavior, unchanged. The tests were my second-weakest addition after the helper itself — the PR is now one file: the two session = db.session introductions and the two session.scalar(stmt) calls.

[yuee3]

Reworked after the review: the helper is gone, each method reads the session locally, and test_api.py is back to its main state (the helper-mocking tests were what failed the API unit shards). Could you take another look when you have a moment?

[yuee3]

Reworked per the review: the session is a parameter now, not a local.

• ApiExternalDataTool.validate_config and query take session: Session | scoped_session and query through it; ExternalDataTool's two abstract methods declare the same parameter.
• ExternalDataToolFactory — their only caller — passes db.session, so the dependency is visible at the call site.
• api.py drops its db import; factory.py gains it.
• test_api.py drops the _DatabaseBinding monkeypatch in favour of passing the real SQLite session; test_factory.py and test_base.py follow the signature.

Six files, +65/-60, pushed as c48fca3.

Not run on my side: make lint && make type-check and the API unit shards — this environment has no dify checkout and no API venv. The six files compile and the call-site inventory is complete (ExternalDataToolFactory is the only production caller, ApiExternalDataTool the only ExternalDataTool subclass), but the suite has not actually been executed, and I'd rather say that plainly than imply a green run.

Could you take another look when you have a moment?

[yuee3]

Heads-up: the branch conflicted with main — since the PR was opened, main gained return annotations on api/tests/unit_tests/core/external_data_tool/api/test_api.py and .../test_base.py, which this branch also edits.

I merged main in (ddec00e) and re-applied the session rework on top of those annotated versions rather than reverting them, so the branch merges into main cleanly now and the diff is the same six files. The other four files are unchanged on main.

Still not run: make lint && make type-check and the API unit shards. Nothing in the merge changes that — it's the same rework, based on current main instead of c592610.

[yuee3]

Fixed the one red check. make type-check-core failed on dcc00e/ddec00e with eight implicit-any-type-argument errors, all from test_base.py: the strict preset in api/tests/unit_tests/pyrefly.toml rejects a bare scoped_session because _S cannot be inferred.

Naming the argument is constrained, not free: db.session is a scoped_session[flask_sqlalchemy.session.Session], so scoped_session[Session] is not assignable to it and fails on this file's two db.session call sites. I used the spelling TagService.get_tag_type already uses for the same value.

55e1ae3. I verified it before pushing by running pyrefly locally against a copy of this tree (strict preset, same flags as dev/pyrefly-check-local) — 0 errors — rather than guessing a third time. Python Style is green now and no check on the commit is failing.

One thing I did not touch: ExternalDataTool's own signatures keep the bare scoped_session. The non-strict api/pyproject.toml project accepts that, and it's the spelling get_message_data and ApiModeration already merged with, so I left it alone rather than widen the diff — say the word if you'd rather they were parameterised too.

[yuee3]

Rebased onto current main (15ed349b1) after the upstream drift: the session-threading refactor is replayed onto the latest files. Two notes for reviewers:

• test_api.py keeps main's current pytestmark (parametrized sqlite_session) and only drops the _DatabaseBinding monkeypatch fixture — the global db import no longer exists in api.py, so there is nothing to patch; sessions are passed explicitly instead.
• test_base.py is updated on top of main's current version (including the newer test_module_attribute case).

[yuee3]

Removed in d0b1d1e: the tests no longer import flask_sqlalchemy or the global extensions.ext_database handle.

• The abstract signatures only promise Session | scoped_session, so the test doubles now parameterize scoped_session with sqlalchemy's own Session (keeps pyrefly's strict preset happy without dragging the Flask stack in).
• Where a session is only there to satisfy the signature before NotImplementedError fires, the tests pass a MagicMock.
• For the factory assertions, factory.db is patched instead of importing the real handle, so the tests verify what the factory forwards without touching extensions.ext_database.

This also matches the PR's own thesis - the whole point is that callers pass the session in explicitly, so the tests shouldn't reach for the global either. Sorry for the churn on the earlier force-push.

[yuee3]

Follow-up in 9f939d4d — CI on the previous head exposed two real problems in the rebuilt branch:

1. test_query_no_result_in_response / test_query_result_not_string called api_tool.query({}, sqlite_session, "") without taking the sqlite_session fixture — the suite died on NameError (and ruff flagged undefined-name). Both now take the fixture like every other test in the file.
2. My attempt to drop the global db handle in test_base.py used MagicMock as a stand-in session, which the no-new-test-doubles guard correctly rejected. Both abstract-dispatch tests now take the real sqlite_session fixture too, so no test doubles were added anywhere.

Net effect: the session passed in every test is a real SQLAlchemy session from the shared SQLite fixture, and nothing in the tests references flask_sqlalchemy or extensions.ext_database anymore.

[yuee3]

Rebased on latest main (80265fc) to resolve the conflicts introduced by #44815 (strict typing in test files) and the Ruff B904 change in api.py:

• kept main's raise ValueError(...) from e in api.py
• kept the -> None annotations that refactor(api): enable strict typing for core/libs/services test files #44815 added to test_factory.py
• carried over the explicit session passing (validate_config(..., session) / query(inputs, session, query)) and the corresponding unit-test updates unchanged

The branch is now rebuilt as a single commit on top of current main. CI will re-run; happy to address anything the new strict-typing rules surface.