Add SQL resource for TagoSQL stored queries - #65
Conversation
felipefdl
left a comment
There was a problem hiding this comment.
Block
Version bump is incomplete, and a few type/test edges should be fixed before merge.
In this PR
Solid additive SQL resource: routes, wire shapes, Cache clears, and date parsing match sibling Resources modules. Intentional omission of /sql/eval and ad-hoc POST /sql/execute is clean on the head.
Blockers
pyproject.toml:7/uv.lock: project version is5.1.4inpyproject.tomlbutuv.lockstill records5.1.3. Prior release commits (for example74e69fe) always bump both. Regenerate and commituv.lockwith the version bump.
Questions
pyproject.toml:7: this is a net-new publicResources.sqlsurface. Pure semver would be a minor (5.2.0). Is patch (5.1.4) intentional for this repo's release habit?- PR description (ad-hoc execute): body cites server#1688 as removing
POST /sql/execute. Confirm that removal is shipped or queued so omitting an ad-hoc SDK method is the long-term contract, not premature.
Improvements
src/tagoio_sdk/modules/Resources/SQL_Types.py:20:SQLCreateInfoistotal=False, so type checkers acceptcreate({})even though the server requiresnameandqueryon create/replace. Prefer requiredname/query(Secrets uses required fields on create; optional only on edit).src/tagoio_sdk/modules/Resources/SQL_Types.py:59:filter: Optional[SQLInfo]is wider than the list filter (name/active/tags). A dedicatedSQLFilterTypedDict (likeSecretsFilter) would match the wire contract.src/tagoio_sdk/modules/Resources/SQL_Types.py:32:SQLInfoomits response fields the server returns on create/info/edit (cache_version,versionsif present on the live contract). Worth surfacing if clients need them.src/tagoio_sdk/modules/Resources/SQL.py:48: iforderByis present butNoneor short, indexing raises before the request. Guard for a real two-item pair (same footgun as other list helpers, still worth tightening here).src/tagoio_sdk/modules/Resources/SQL.py:246:tables({"filter": None})emitsfilter=Noneon the path instead of omitting the key. Only append when the value is a non-empty string.tests/Resources/test_sql.py:129:tables({"filter": "sensor"})never asserts the request URL/query includesfilter=sensor, so a regression that drops the path workaround still passes. Assert the request.docs/source/Resources/index.rst: no Sphinx toctree entry for SQL. Other Resources modules are listed; py.sdk.tago.io will not show this surface until docs are added.
Praise
- Paths, verbs,
[{key, value}]params, execute envelope (served_from_cache), and tables filter-on-path workaround all line up with the shareddoRequestfilter expansion behavior. - Mutating methods call
Cache.clear_cache(); list/info/version usedateParser/dateParserListconsistently with Devices/Dashboards. - Tests cover every public method with the right HTTP verbs and paths.
Risk (CIA)
Likelihood: 🟢 Low | Impact: 🟢 Low | Exposure: 🟢 Low.
Confirmed. Additive SDK client only; no auth or crypto changes. Residual risk is incomplete release bookkeeping and unasserted tables-filter wire shape, not production breakage.
Address review on PR #65: - sync uv.lock tagoio-sdk version to 5.1.4 (matches pyproject) - SQLCreateInfo requires name and query on create/replace; other fields optional - SQLInfo becomes a standalone total=False response type; add cache_version and versions - add dedicated SQLFilter (name/active/tags) for the list filter - guard orderBy to a real two-item pair before building the string - tables() only appends filter to the path for a non-empty string; None omits the key - assert the tables request URL carries filter=sensor and add a None-filter omission test Claude-Session: https://claude.ai/code/session_018B8uKfmuuoBf9p3z3GRfcN
Add docs/source/Resources/SQL index and type reference mirroring the Secrets module, and register it in the Resources toctree so the surface shows on py.sdk.tago.io. Claude-Session: https://claude.ai/code/session_018B8uKfmuuoBf9p3z3GRfcN
|
All findings addressed in 1989cd6 and 152ebfb: Blocker: Questions answered:
Improvements, all applied:
Gates: ruff clean on the changed files, 209 tests passing (TZ=UTC). Ready for re-review. |
Summary
TagoSQL shipped in server v10.3.0 with no SDK coverage, so applications and analyses had to call the /sql routes with raw requests. This adds a
sqlresource toResourcescovering stored-query CRUD (list,create,info,edit,delete), version snapshots (getVersion), execution (executewith per-key param overrides and test mode), and schema discovery (tables). The legacy/sql/evalendpoint and the removed ad-hocPOST /sql/executeroute (tago-io/server#1688) are intentionally not covered.Stored-query info/list responses now expose a read-only, server-derived
session_contextboolean (true when the query text uses session functions; never accepted as input), and thetablescatalog gains afunctionsarray (SQLFunctionInfo:name,kind,args,description, andexampleon session entries) listing everything callable in a query (tago-io/server#1690).Test plan
tests/Resources/test_sql.py(requests-mock round trips for all methods)uv run pytest tests/passes (209 tests) anduv run ruff checkis clean on the new filesdocs/TagoSQL.md(params as[{key, value}]arrays, execute envelope withserved_from_cache, list fields allowlist); thetablessubstring filter is sent on the path because the shareddoRequestexpands object filters intofilter[...]pairsRisk (CIA)
Likelihood: 🟢 Low | Impact: 🟢 Low | Exposure: 🟢 Low