fix(tools): WMS attributeVariables / popup_options keys use wms_layers value, not display name - #16
Merged
Merged
Conversation
…s value, not display name Post-/ce-debug audit 2026-05-21: same Class A bug class as ESRI Image pre-PR-#12, just on a different identifier. add_wms_layer was keying attributeVariables and popup_options.aliases by the user's `name` arg (display name), but React's getImageWMSLayerAttributes (in reactapp/components/map/utilities.js) keys alias maps by the WMS LAYERS param value (e.g., "topp:states") — fetched from DescribeFeatureType?typename=<LAYERS>. Mismatch caused silent click-time lookup failures whenever the user's display name differed from the WMS LAYERS value (the common case — display names are friendly labels like "US States"; LAYERS values are workspace-prefixed like "topp:states"). This commit: - Adds `_resolve_wms_attr_key(wms_layers, fallback_name)` helper near `_resolve_esri_layer_name`. Picks the first comma-separated entry of `wms_layers` (stripped), falling back to `fallback_name` if `wms_layers` is empty/missing. Simpler than the ESRI Image case — no network call needed; the LAYERS value is already in the `wms_layers` arg. - Wires into add_wms_layer's `_apply_common_layer_options` call: `attr_key=_resolve_wms_attr_key(wms_layers, name)` instead of `attr_key=name`. Affects both attribute_variables and the popup_options outer-key normalization PR #11 added. ## Multi-layer WMS For comma-separated `wms_layers` (e.g., "topp:states,topp:counties"), the helper picks the first entry as `attr_key`. Multi-layer WMS calls are rare in practice and the React attribute-config UI is per-layer anyway; the first layer is the conservative pick. Cross-layer attribute-variable authoring should use one add_wms_layer call per layer. ## Tests 4 new in TestAddWmsLayerAttrKeyFromWmsLayers: - test_attribute_variables_outer_key_is_wms_layers_value: motivating case — outer key is "topp:states", not display name "US States" - test_comma_separated_wms_layers_picks_first: multi-layer picks first - test_popup_options_aliases_also_uses_wms_layers_value: PR #11's outer-key normalization fires against the new attr_key - test_no_attribute_variables_no_regression: when neither attribute_variables nor popup_options is provided, the regular WMS layer-add flow is unchanged (no attributeVariables / attributeAliases in the persisted layer) Plus updated 1 pre-existing test (TestAdvancedMetadata::test_advanced_dicts_accepted_as_json_strings) to assert the new correct outer-key behavior — it was previously asserting the pre-fix display-name key. Full suite: 948 passed (944 baseline + 4 new). One assertion update on an existing test reflects the new correct behavior; zero true regression. ## Audit by-product context This is the 7th PR in the 2026-05-21 debug arc. Class A surface now covered on ESRI Image (PR #12) + WMS (this PR). Other layer types verified NOT affected: ESRI Feature (caller-supplied layerName matches server-side), GeoJSON (same), KML (same), tile types (not queryable). PMTiles Vector remains low-risk / flagged for if-when-used.
romer8
added a commit
that referenced
this pull request
May 22, 2026
…tions too 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).
romer8
added a commit
that referenced
this pull request
May 22, 2026
…tions too (#17) 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).
romer8
added a commit
that referenced
this pull request
May 22, 2026
Eight PRs from the 2026-05-21 popup-modal arc, grouped into Added / Changed / Fixed / Tests / Documentation per Keep a Changelog. New tool + slash-prompt for configure_popup_modal_layer (PR #10), case-fold args normalization for popup gridItems + render_plugin + add_dynamic_map_layer (PRs #14 + #15), ESRI sublayer-ID extraction from LAYERDEFS + ESRI Image guard widening (PRs #12 + #17), WMS attr_key from wms_layers (PR #16), popup_options outer-key normalization across all 11 add_*_layer tools (PR #11), tool description tightening for case-sensitive arg_names + 100-col grid (PR #13). Suite grew 930 → 961.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-/ce-debug audit identified the same Class A bug class as ESRI Image pre-PR-#12 on
add_wms_layer. Server keyedattributeVariablesandpopup_options.aliasesby the user's display name (namearg); React'sgetImageWMSLayerAttributeskeys alias maps by the WMS LAYERS param value (e.g.,\"topp:states\"). Silent click-time lookup failures whenever the display name differed from LAYERS — which is the common case (display names like\"US States\"vs LAYERS like\"topp:states\").Root cause (verified in source)
Server:
mcp_server.py:1947→attr_key=name(display name).React:
reactapp/components/map/utilities.js getImageWMSLayerAttributes:Keys by the WMS LAYERS value. Mismatch.
Fix
Adds
_resolve_wms_attr_key(wms_layers, fallback_name)helper. Picks the first comma-separated entry ofwms_layers(stripped), falling back tofallback_nameif missing. Wired intoadd_wms_layersoattr_key=_resolve_wms_attr_key(wms_layers, name)instead ofattr_key=name.Simpler than the ESRI Image fix (PR #12) — no
?f=jsonHTTP fetch needed; the LAYERS value is already in thewms_layersarg.Multi-layer WMS
Picks the first comma-separated entry. Multi-layer WMS calls are rare; React attribute-config UI is per-layer anyway; cross-layer attribute-variable authoring should use one
add_wms_layercall per layer.Tests
4 new in
TestAddWmsLayerAttrKeyFromWmsLayers:test_attribute_variables_outer_key_is_wms_layers_value— motivating casetest_comma_separated_wms_layers_picks_first— multi-layer disambiguationtest_popup_options_aliases_also_uses_wms_layers_value— PR fix(tools): name aliases/omit keys in popup_options descriptions on all add_*_layer tools #11's outer-key normalize now fires against the right attr_keytest_no_attribute_variables_no_regression— no-op when neither field is providedPlus 1 pre-existing test updated to assert the new correct outer-key behavior (it was asserting the pre-fix display-name key).
Full suite: 948 passed (944 baseline + 4 new).
Audit completion
This is the 7th PR in the 2026-05-21 debug arc. Class A surface now covered:
Class B surface (LLM args case-fold):
add_*_layertoolsargsdict for backend pluginThe full 2026-05-21 debug arc surface is now covered (modulo the flagged secondary ESRI Image popup_options.aliases issue, deferred until observed).