From 50a383730c9e03ebd9cf4f31763f724548a4d231 Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 16:56:46 -0600 Subject: [PATCH] fix(tools): derive ESRI sublayer ID from LAYERDEFS when LAYERS is absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug session 2026-05-21 turn 1 with gemini-flash, full user prompt: "Add the China Flowlines ESRI Image and Map Service layer ... make it queryable, set params.LAYERDEFS to 0: rivercountry = 'China'. Alias the comid attribute to River ID." The LLM correctly translated "alias the comid attribute to River ID" to attribute_variables = {"comid": "River ID"} on add_esri_image_layer. But the user's params spec used LAYERDEFS to encode both the sublayer ID (0) and the WHERE clause — no LAYERS field, no layer_id arg. Server-side, the ESRI-sublayer-name resolver only looked at params.LAYERS: effective_layer_id = flat_source_props.get("params", {}).get("LAYERS") resolved = _resolve_esri_layer_name(url, effective_layer_id) With LAYERS absent, effective_layer_id was None. _resolve_esri_layer_name returned None immediately (it requires a non-None layer_id). The server fell back to keying attributeVariables by the display name ("Bolivia Flowlines" / "China Flowlines") instead of the ESRI service's actual sublayer name ("Flow Forecast (m³/sec)"). The React popup-render path (AttributesPane.js:322-356) keys lookups by the ESRI sublayer name fetched fresh from ?f=json. Key mismatch -> "Variable Input Name" column rendered empty at runtime even though the data was persisted. CLAUDE.md MCP Data Contract Rule 5 already calls this out: "ESRI attributeVariables key: Use the ESRI service's actual layer name (fetched from {url}?f=json), not the client display name." Fix: when LAYERS is absent, fall back to extracting the layer index from LAYERDEFS. LAYERDEFS encodes ":" — split on the first colon, validate the prefix is all digits, pass to _resolve_esri_layer_name. LAYERS still wins when both are set (existing precedence preserved). Non-digit LAYERDEFS prefixes (raw filter strings without a layer-id prefix) are correctly NOT extracted, so the resolver gets None and the fallback-to-display-name behavior is unchanged for that case. Tests: 5 new in TestAddEsriImageLayer: - test_layerdefs_only_resolves_attribute_variables_key: motivating case - test_layerdefs_with_space_after_colon: real user prompt had "0: " - test_layerdefs_only_resolver_fails_falls_back_to_display_name: preserves fallback path - test_layers_and_layerdefs_both_present_layers_wins: LAYERS precedence - test_layerdefs_non_digit_prefix_no_extraction: raw LAYERDEFS string not misparsed as a layer-id prefix Full suite: 935 passed (930 baseline + 5 new). One change at one site; no regression to the existing LAYERS-based code path. --- test_mcp/test_per_source_type_layer_tools.py | 119 +++++++++++++++++++ tethysdash_mcp/mcp_server.py | 15 +++ 2 files changed, 134 insertions(+) diff --git a/test_mcp/test_per_source_type_layer_tools.py b/test_mcp/test_per_source_type_layer_tools.py index 7ae7559..75123d6 100644 --- a/test_mcp/test_per_source_type_layer_tools.py +++ b/test_mcp/test_per_source_type_layer_tools.py @@ -406,6 +406,125 @@ def test_params_path_resolves_attribute_variables_correctly(self, mock_resolve): assert "My Display Name" not in layer["attributeVariables"] assert layer["attributeVariables"]["River Gauges"] == {"STAGE": "stage_var"} + # Debug session 2026-05-21 turn 1 (real user prompt): LLM emitted + # add_esri_image_layer with params={"LAYERDEFS": "0:rivercountry = 'China'"} + # and attribute_variables — but no `layer_id` arg and no `params.LAYERS`. + # _resolve_esri_layer_name was called with `effective_layer_id=None` + # (because LAYERS was absent) → returned None immediately → server fell + # back to the display name. Result: attributeVariables was keyed by the + # display name, but the React popup-render path queries by the ESRI + # service's actual sublayer name fetched from ?f=json → silent lookup + # miss → "Variable Input Name" column empty at runtime. + # + # Fix: when LAYERS is absent, extract the layer index from LAYERDEFS + # (which encodes sublayer ID as the prefix before the first colon). + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_layerdefs_only_resolves_attribute_variables_key(self, mock_resolve): + """LAYERDEFS-only path (no LAYERS, no layer_id) extracts layer ID and resolves.""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="Bolivia Flowlines", + url="https://example.com/arcgis/rest/services/MyService/MapServer", + params={"LAYERDEFS": "0:rivercountry = 'Bolivia'"}, + attribute_variables={"comid": "River ID"}, + ) + layer = _get_layer_config(result) + # Resolver was called with the layer index "0" extracted from LAYERDEFS. + mock_resolve.assert_called_once_with( + "https://example.com/arcgis/rest/services/MyService/MapServer", "0" + ) + # Persisted key uses the resolved sublayer name, not the display name. + assert "Flow Forecast" in layer["attributeVariables"] + assert "Bolivia Flowlines" not in layer["attributeVariables"] + assert layer["attributeVariables"]["Flow Forecast"] == {"comid": "River ID"} + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_layerdefs_with_space_after_colon(self, mock_resolve): + """LAYERDEFS="0: " (space after colon) still extracts "0".""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="Bolivia Flowlines", + url="https://example.com/arcgis/rest/services/MyService/MapServer", + params={"LAYERDEFS": "0: rivercountry = 'Bolivia'"}, + attribute_variables={"comid": "River ID"}, + ) + mock_resolve.assert_called_once_with( + "https://example.com/arcgis/rest/services/MyService/MapServer", "0" + ) + layer = _get_layer_config(result) + assert layer["attributeVariables"]["Flow Forecast"] == {"comid": "River ID"} + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value=None, + ) + def test_layerdefs_only_resolver_fails_falls_back_to_display_name( + self, mock_resolve + ): + """When LAYERDEFS-derived resolver returns None, fallback to display name (regression preservation).""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="Bolivia Flowlines", + url="https://example.com/arcgis/rest/services/MyService/MapServer", + params={"LAYERDEFS": "0:rivercountry = 'Bolivia'"}, + attribute_variables={"comid": "River ID"}, + ) + # Resolver IS called with the extracted "0" (didn't pass None). + mock_resolve.assert_called_once_with( + "https://example.com/arcgis/rest/services/MyService/MapServer", "0" + ) + layer = _get_layer_config(result) + # Resolver returned None → fallback to display name (current behavior). + assert "Bolivia Flowlines" in layer["attributeVariables"] + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="From LAYERS", + ) + def test_layers_and_layerdefs_both_present_layers_wins(self, mock_resolve): + """When both LAYERS and LAYERDEFS are set, LAYERS takes precedence for layer-ID extraction.""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="My Display Name", + url="https://example.com/arcgis/rest/services/MyService/MapServer", + layer_id="5", # Will canonicalize to "show:5" in LAYERS + params={"LAYERDEFS": "0:rivercountry = 'X'"}, + attribute_variables={"comid": "River ID"}, + ) + # Resolver got "show:5" (from LAYERS), not "0" (from LAYERDEFS). + mock_resolve.assert_called_once_with( + "https://example.com/arcgis/rest/services/MyService/MapServer", "show:5" + ) + layer = _get_layer_config(result) + assert layer["attributeVariables"]["From LAYERS"] == {"comid": "River ID"} + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_layerdefs_non_digit_prefix_no_extraction(self, mock_resolve): + """LAYERDEFS with a non-digit prefix (e.g., raw filter string) → resolver gets None.""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="Bolivia Flowlines", + url="https://example.com/arcgis/rest/services/MyService/MapServer", + params={"LAYERDEFS": "rivercountry = 'Bolivia'"}, # no ":" prefix + attribute_variables={"comid": "River ID"}, + ) + # The colon-less LAYERDEFS shouldn't be parsed as a layer ID; resolver + # is called with None. The resolver mock returns "Flow Forecast" but + # only if called with a real id — we test the call signature. + mock_resolve.assert_called_once_with( + "https://example.com/arcgis/rest/services/MyService/MapServer", None + ) + def test_canonicalizes_integer_layers_value(self): """Non-string LAYERS value (e.g., LLM passing integer) is coerced and canonicalized.""" result = add_esri_image_layer( diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index ec41631..27dd037 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -2060,6 +2060,21 @@ def add_esri_image_layer( attr_key = name if attribute_variables: effective_layer_id = flat_source_props.get("params", {}).get("LAYERS") + if effective_layer_id is None: + # LAYERDEFS encodes the sublayer ID as the prefix before the + # first colon: ":" (e.g., + # "0:rivercountry = 'China'"). When the LLM sets LAYERDEFS but + # not LAYERS or the layer_id arg, derive the layer index from + # LAYERDEFS so the ?f=json lookup can resolve the actual ESRI + # sublayer name. Without this fallback, attributeVariables ends + # up keyed by the user-facing display name (the `name` arg), + # while the React popup-render path queries by the ESRI + # service's sublayer name — silent click-time lookup failure. + layerdefs = flat_source_props.get("params", {}).get("LAYERDEFS") + if isinstance(layerdefs, str) and ":" in layerdefs: + candidate = layerdefs.split(":", 1)[0].strip() + if candidate.isdigit(): + effective_layer_id = candidate resolved = _resolve_esri_layer_name(url, effective_layer_id) if resolved: attr_key = resolved