Repository navigation
fix: resolve the AssetDB data source by name - #70
Conversation
The data source id defaulted to 1, on the assumption that AssetDB is whatever Redash was given first. That holds on a deployment where nothing else was created before it and nowhere else: on Stacklet's Dev it is 6, and ids 1 and 2 do not exist at all. Querying an id Redash does not have is not a clean 404. Its query_results handler looks the data source up with get_by_id_and_org and does not catch the resulting NoResultFound, so the API answers a bare 500 and every AssetDB tool fails with an unexplained internal error. So resolve the id from the data source *name* instead, which Redash provisioning controls and which does not depend on creation order. Looked up on first use rather than at startup, because the lookup runs as the calling user -- Redash has no other credential here -- and cached for the process once it succeeds, since the mapping only changes if the data source is recreated. A failed lookup is not cached, so a user who cannot see the source does not poison it for everyone. assetdb_datasource stays as an explicit override for a deployment that needs to name an id, and now defaults to None, meaning 'look it up'. A name that matches nothing raises a tool error naming the data sources that do exist, rather than leaving the caller with Redash's 500.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assetdb_datasource: int | None = Field( | ||
| default=None, | ||
| description=( | ||
| "Redash data source id to query, for a deployment that needs to name one " | ||
| "explicitly. Left unset, the id is looked up by name -- see " | ||
| "assetdb_datasource_name -- which is what every Stacklet deployment wants: " | ||
| "Redash assigns the id at creation, so it differs between deployments and " | ||
| "cannot be assumed." | ||
| ), | ||
| ) | ||
| assetdb_datasource_name: str = Field( | ||
| default="AssetDB", | ||
| description=( | ||
| "Name of the Redash data source to resolve an id from, used when " | ||
| "assetdb_datasource is unset. Matches the name Stacklet provisions, which " | ||
| "is stable across deployments in the way the id is not." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The shipped default now resolves the data source by name instead of defaulting its ID to 1, but the repository's configuration guide still documents STACKLET_MCP_ASSETDB_DATASOURCE as defaulting to 1 and does not mention the new name setting. This can lead operators to expect behavior the server no longer has; update the configuration reference to match these settings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: stacklet/mcp/settings.py
Line: 33-50
Comment:
**Configuration guide is stale**
The shipped default now resolves the data source by name instead of defaulting its ID to `1`, but the repository's configuration guide still documents `STACKLET_MCP_ASSETDB_DATASOURCE` as defaulting to `1` and does not mention the new name setting. This can lead operators to expect behavior the server no longer has; update the configuration reference to match these settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Valid — fixed in 3b29a57. README.md:71 did still document the id as defaulting to 1. It now describes the id as unset-by-default meaning "look it up", and documents STACKLET_MCP_ASSETDB_DATASOURCE_NAME alongside it.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="stacklet/mcp/settings.py">
<violation number="1" location="stacklet/mcp/settings.py:34">
P2: README.md and CLAUDE.md still say AssetDB defaults to ID 1 and omit the name setting. Update both configuration descriptions to document the `None`/`AssetDB` defaults and the two override paths.</violation>
</file>
<file name="stacklet/mcp/assetdb/redash.py">
<violation number="1" location="stacklet/mcp/assetdb/redash.py:90">
P2: This name-only cache key reuses a data-source ID across credentials in a shared `ServerState`, although Redash data-source visibility is user-scoped. Scope the cache by authenticated organization/user or disable this cache for shared hosted state.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| default=1, | ||
| description="AssetDB datasource", | ||
| assetdb_datasource: int | None = Field( | ||
| default=None, |
There was a problem hiding this comment.
P2: README.md and CLAUDE.md still say AssetDB defaults to ID 1 and omit the name setting. Update both configuration descriptions to document the None/AssetDB defaults and the two override paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At stacklet/mcp/settings.py, line 34:
<comment>README.md and CLAUDE.md still say AssetDB defaults to ID 1 and omit the name setting. Update both configuration descriptions to document the `None`/`AssetDB` defaults and the two override paths.</comment>
<file context>
@@ -30,9 +30,23 @@ class Settings(BaseSettings):
- default=1,
- description="AssetDB datasource",
+ assetdb_datasource: int | None = Field(
+ default=None,
+ description=(
+ "Redash data source id to query, for a deployment that needs to name one "
</file context>
There was a problem hiding this comment.
Valid — fixed in 3b29a57. Both files did still say the id defaults to 1 and omit the name setting; both now document the two override paths.
| if self.configured_data_source_id is not None: | ||
| return self.configured_data_source_id | ||
| return await self.server_state.ensure_cached_async( | ||
| f"ASSETDB_DATASOURCE_ID:{self.data_source_name}", |
There was a problem hiding this comment.
P2: This name-only cache key reuses a data-source ID across credentials in a shared ServerState, although Redash data-source visibility is user-scoped. Scope the cache by authenticated organization/user or disable this cache for shared hosted state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At stacklet/mcp/assetdb/redash.py, line 90:
<comment>This name-only cache key reuses a data-source ID across credentials in a shared `ServerState`, although Redash data-source visibility is user-scoped. Scope the cache by authenticated organization/user or disable this cache for shared hosted state.</comment>
<file context>
@@ -57,7 +61,54 @@ def __init__(
+ if self.configured_data_source_id is not None:
+ return self.configured_data_source_id
+ return await self.server_state.ensure_cached_async(
+ f"ASSETDB_DATASOURCE_ID:{self.data_source_name}",
+ self._lookup_data_source_id,
+ )
</file context>
There was a problem hiding this comment.
Keyed by Redash URL in 3b29a57 — see the reply on the parallel thread for why the collision isn't reachable today.
On the user-scoping half specifically: not changing that, and I think it would be wrong to. Redash data-source visibility is user-scoped, but the name→id mapping is a property of the org, not of the viewer — the same AssetDB source has one id for everyone who can see it. A user who can't see it gets an error rather than a wrong id, and that failure isn't cached, so it can't poison the shared entry. Keying by user would turn one lookup per process into one per user for no correctness gain.
| assetdb_datasource: int = Field( | ||
| default=1, | ||
| description="AssetDB datasource", | ||
| assetdb_datasource: int | None = Field( |
There was a problem hiding this comment.
Left it in, but I'd argue it either way — here's the reasoning so you can overrule.
Against keeping it: it has no consumer. Platform's companion PR removes the only place that set it, and nothing else in the tree or the deployment configs does. The case it used to cover ("this deployment's id isn't 1") is now covered by the lookup, and the case where the source is named something else is covered by assetdb_datasource_name. So it's close to redundant.
For keeping it: it's a three-line escape hatch for a lookup that has never run in production. If GET /api/data_sources misbehaves for a reason we haven't predicted — permissions, a duplicate name, an API change — an operator can set the id and be unblocked without waiting on a release. It's also what the failure message tells them to do. Deprecating it in the same change that introduces the mechanism it backs up feels like removing the parachute before the jump.
My inclination is to leave it undeprecated until the lookup has some production mileage, then drop it outright rather than deprecate-then-remove, given there are no external consumers to stage for. Happy to mark it deprecated now if you'd rather signal it early — say the word and I'll push it.
Key the resolved-id cache by Redash URL as well as name. Every caller in a process shares one endpoint today, so this cannot currently collide, but an id means nothing without the deployment it came from and the key should say so. Cover the claim that a failed lookup is not cached, which the cache being process-wide makes load-bearing: remembering a failure would turn one under-permissioned caller into an outage for everyone until the task recycled. Assert the shipped defaults off the field declarations rather than a fresh Settings(), which reads the ambient environment and would fail for anyone with STACKLET_MCP_ASSETDB_DATASOURCE exported. Update README.md and CLAUDE.md, which both still documented the id as defaulting to 1 and did not mention the name setting.
what
assetdb_datasourcedefaults toNoneinstead of1, meaning "look the id up". A newassetdb_datasource_name(defaultAssetDB) names what to look for, andAssetDBClientresolves the id from
GET /api/data_sourceson first use, caching it for the process.An explicit
assetdb_datasourcestill wins and skips the lookup, so a deployment thatneeds to name an id can. A name matching nothing raises a tool error listing the data
sources that do exist.
why
The id defaulted to 1, on the assumption that AssetDB is whatever Redash was given
first. That holds on a deployment where nothing else was created before it and nowhere
else — on Stacklet's Dev it is 6, and ids 1 and 2 do not exist at all.
Querying an id Redash does not have is not a clean 404. Its
query_resultshandlerlooks the data source up with
get_by_id_and_organd does not catch the resultingNoResultFound, so the API answers a bare 500._make_requestraises for status onthat, and the caller sees
Error calling tool 'assetdb_sql_query'with nothing to acton. Every AssetDB tool that executes a query is affected, for every user of that
deployment.
The name is the stable half: Redash assigns the id at creation, but Stacklet
provisioning sets the name, and sets the same one (
AssetDB) in both the local-devbootstrap and the deployed path.
Resolution is on first use rather than at startup because the lookup runs as the calling
user — Redash has no other credential here — so there is no credential to do it with
before a request arrives. It is cached once it succeeds, since the mapping only changes
if the data source is recreated; a failure is not cached, so a user who cannot see the
source does not poison the cache for everyone.
testing
just test(full suite) andjust lint, both clean.New tests cover the lookup resolving a non-1 id, the result being cached rather than
re-fetched per query, an explicit id skipping the lookup entirely, and a name that
matches nothing producing the annotated error instead of a bare failure.
The shared test fixture now names an id, so the tests that exercise querying are not
also exercising the lookup; the lookup tests opt back in by clearing it.
test_settingsasserts the shipped defaults off a fresh
Settingsrather than the fixture-patchedsingleton, which would otherwise have quietly asserted the fixture.
Not covered: a real query against a deployment. That needs the companion platform change
below.
docs
None needed — the settings carry their own descriptions, including when each applies.