From a5e63128fbf9b0a1dc9e002e0d0cddb36496751e Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 20:23:56 -0600 Subject: [PATCH] fix(tools): widen ESRI Image attr_key resolution to fire for popup_options too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug audit 2026-05-21 secondary finding: add_esri_image_layer's attr_key resolution (PR #12) was gated on `if attribute_variables:`. The popup_options-alone case fell through with the display-name fallback, causing silent click-time misses in the popup-table render path. Server side (mcp_server.py:2061): `if attribute_variables:` triggered the _resolve_esri_layer_name call. popup_options provided WITHOUT attribute_variables left attr_key = name (display name). React side (utilities.js getImageArcGISRestLayerAttributes): keys alias maps by the service-side layer.name fetched from ?f=json — same as the attribute_variables path. So popup_options.{aliases,omit} keyed by display name would silently miss at lookup time. PR #11's outer-key normalization rewrites popup_options.{aliases,omit} to attr_key. With attr_key=display_name, that rewrote to the wrong key. Widen the guard from `if attribute_variables:` to `if attribute_variables or popup_options:`. The resolver is a soft-fail (returns None on network failure → fallback to display name preserved). Inline comment expanded to explain both code paths and the React popup-render contract. No new helper or behavior change for the attribute_variables case — just makes the existing resolution machinery fire for one more condition. 5 new in TestAddEsriImagePopupOptionsAttrKeyResolution: - test_popup_options_aliases_alone_uses_resolved_sublayer_name: the motivating case — popup_options.aliases alone, outer key rewritten to resolved sublayer name, not display name - test_popup_options_omit_alone_uses_resolved_sublayer_name: same for the omit field - test_both_attribute_variables_and_popup_options_same_attr_key: defensive — both fields keyed by the same resolved sublayer name - test_popup_options_alone_resolver_fails_falls_back_to_display_name: soft-fail path preserved - test_neither_attribute_variables_nor_popup_options_no_resolver_call: regression — when neither field is provided, resolver doesn't fire (no wasted network call) Full suite: 949 passed (944 baseline + 5 new). This is the 8th PR in the 2026-05-21 debug arc. Class A surface for ESRI Image now fully covered — the original PR #12 fix for attribute_variables + this PR for popup_options. Combined with PR #16 (WMS attr_key from wms_layers), all known Class A bugs are addressed. ESRI Feature / GeoJSON / KML / tile types / GeoTIFF / static image remain not-affected (caller-supplied layerName matches across server + React, or are not queryable at the attribute level). --- test_mcp/test_per_source_type_layer_tools.py | 147 ++++++++++++++++++- tethysdash_mcp/mcp_server.py | 20 ++- 2 files changed, 161 insertions(+), 6 deletions(-) diff --git a/test_mcp/test_per_source_type_layer_tools.py b/test_mcp/test_per_source_type_layer_tools.py index 88e9cdb..c3675b9 100644 --- a/test_mcp/test_per_source_type_layer_tools.py +++ b/test_mcp/test_per_source_type_layer_tools.py @@ -2235,7 +2235,6 @@ def test_consuming_types_still_accept_params(self): assert "layer_update" in result, result -# --------------------------------------------------------------------------- # WMS attribute-variables / popup-options outer-key resolution # (debug audit 2026-05-21: Class A bug — same shape as ESRI Image pre-PR-#12) # --------------------------------------------------------------------------- @@ -2315,3 +2314,149 @@ def test_no_attribute_variables_no_regression(self): # deletes the empty dicts). assert "attributeVariables" not in layer assert "attributeAliases" not in layer + + +# --------------------------------------------------------------------------- +# ESRI Image popup_options attr_key resolution (secondary Class A fix) +# (debug audit 2026-05-21: attr_key resolution must also fire when +# popup_options is provided — not only when attribute_variables is set) +# --------------------------------------------------------------------------- + + +class TestAddEsriImagePopupOptionsAttrKeyResolution: + """When popup_options is provided without attribute_variables, attr_key + must still resolve to the ESRI service's sublayer name. The React + popup-table render path (utilities.js getImageArcGISRestLayerAttributes) + keys by the service-side layer.name fetched from ?f=json — same as the + attribute_variables path. PR #11's outer-key normalization rewrites + popup_options.{aliases,omit} to attr_key, so attr_key must be the + resolved sublayer name regardless of which field triggered it. + + Originally the resolver block fired only ``if attribute_variables:`` — + the popup_options-alone case fell through with the display-name + fallback, causing silent click-time misses in the popup table. + """ + + URL = "https://example.com/arcgis/rest/services/MyService/MapServer" + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_popup_options_aliases_alone_uses_resolved_sublayer_name( + self, mock_resolve + ): + """popup_options.aliases without attribute_variables: outer key is + the resolved sublayer name, NOT the display name. + + Real-world LLM emits the display name as the outer key (because the + tool description names the `name` arg) — PR #11's outer-key + normalization rewrites the single-entry outer key to attr_key. With + the fix, attr_key is now the resolved sublayer name when + popup_options is provided (previously fired only for + attribute_variables). + """ + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="China Flowlines", + url=self.URL, + layer_id="0", + popup_options={ + "aliases": {"China Flowlines": {"comid": "River ID"}}, + }, + ) + layer = _get_layer_config(result) + # PR #11's single-entry outer-key normalize rewrote + # "China Flowlines" -> attr_key ("Flow Forecast"). + assert "Flow Forecast" in layer["attributeAliases"] + assert "China Flowlines" not in layer["attributeAliases"] + assert layer["attributeAliases"]["Flow Forecast"] == {"comid": "River ID"} + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_popup_options_omit_alone_uses_resolved_sublayer_name( + self, mock_resolve + ): + """popup_options.omit without attribute_variables: same resolution.""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="China Flowlines", + url=self.URL, + layer_id="0", + popup_options={ + "omit": {"China Flowlines": ["unused_field"]}, + }, + ) + layer = _get_layer_config(result) + assert "Flow Forecast" in layer["omittedPopupAttributes"] + assert layer["omittedPopupAttributes"]["Flow Forecast"] == ["unused_field"] + assert "China Flowlines" not in layer["omittedPopupAttributes"] + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Flow Forecast", + ) + def test_both_attribute_variables_and_popup_options_same_attr_key( + self, mock_resolve + ): + """Both fields keyed by the SAME resolved sublayer name.""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="China Flowlines", + url=self.URL, + layer_id="0", + attribute_variables={"comid": "River ID"}, + popup_options={ + "aliases": {"China Flowlines": {"comid": "Comid Alias"}}, + }, + ) + layer = _get_layer_config(result) + assert layer["attributeVariables"] == {"Flow Forecast": {"comid": "River ID"}} + assert layer["attributeAliases"] == {"Flow Forecast": {"comid": "Comid Alias"}} + + @patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value=None, + ) + def test_popup_options_alone_resolver_fails_falls_back_to_display_name( + self, mock_resolve + ): + """Soft-fail path: when resolver returns None, fallback to display + name (current behavior preserved when network down).""" + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="China Flowlines", + url=self.URL, + layer_id="0", + popup_options={ + "aliases": {"China Flowlines": {"comid": "River ID"}}, + }, + ) + layer = _get_layer_config(result) + assert "China Flowlines" in layer["attributeAliases"] + assert layer["attributeAliases"]["China Flowlines"] == {"comid": "River ID"} + + def test_neither_attribute_variables_nor_popup_options_no_resolver_call( + self, mocker + ): + """Regression: when neither field is provided, the resolver block + doesn't fire (no wasted network call) and the layer builds normally.""" + spy = mocker.patch( + "tethysdash_mcp.mcp_server._resolve_esri_layer_name", + return_value="Should Not Be Called", + ) + result = add_esri_image_layer( + map_uuid=MAP_UUID, + name="China Flowlines", + url=self.URL, + layer_id="0", + ) + assert "layer_update" in result + spy.assert_not_called() + layer = _get_layer_config(result) + # No attributeVariables / attributeAliases in the persisted layer + # (build() deletes the empty dicts). + assert "attributeVariables" not in layer + assert "attributeAliases" not in layer diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index 12a73ef..d78129b 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -2087,7 +2087,16 @@ def add_esri_image_layer( flat_source_props["params"] = esri_params attr_key = name - if attribute_variables: + # Resolve attr_key to the ESRI service's sublayer name whenever either + # attribute_variables OR popup_options is provided. Both code paths feed + # the same React popup-render machinery, which keys alias maps by the + # ESRI service's actual sublayer name (fetched from ?f=json), NOT by the + # user-supplied display name. PR #11's outer-key normalization rewrites + # popup_options.{aliases,omit} to attr_key, so attr_key must be the + # resolved sublayer name for popup_options to land on the right key. + # Originally this block fired only `if attribute_variables:` — that + # missed the popup_options.aliases-alone case (2026-05-21 audit). + if attribute_variables or popup_options: 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 @@ -2095,10 +2104,11 @@ def add_esri_image_layer( # "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. + # sublayer name. Without this fallback, attributeVariables / + # popup_options end 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()