Skip to content

Fix TopoJson non-collection geometries - #2250

Open
KhadeerBasha1232 wants to merge 2 commits into
python-visualization:mainfrom
KhadeerBasha1232:fix-topojson-polygon
Open

Fix TopoJson non-collection geometries#2250
KhadeerBasha1232 wants to merge 2 commits into
python-visualization:mainfrom
KhadeerBasha1232:fix-topojson-polygon

Conversation

@KhadeerBasha1232

Copy link
Copy Markdown

Fixes #1816

Problem

When a TopoJSON's objects entry had a type of Polygon or
MultiPolygon (instead of GeometryCollection), folium raised:

KeyError: 'geometries'

This happened because the code assumed every object had a geometries
key and didn't handle "bare" geometry objects that store their arcs
directly.

Fix

Updated the TopoJson handling in folium/features.py so it also
supports non-collection geometry types (Polygon, MultiPolygon,
etc.), not just GeometryCollection.

Testing

Reproduced the exact example from #1816 — it rendered without the
KeyError after this change. Ran the existing test suite locally with
python -m pytest tests --ignore=tests/selenium and all tests pass.

@KhadeerBasha1232

Copy link
Copy Markdown
Author

This fixes #1816 — TopoJson raised a KeyError('geometries') when the
topology object's type was Polygon or MultiPolygon instead of
GeometryCollection, since the code assumed every object had a
geometries key.

Fix checks type first and falls back to treating the object itself
as a single-item geometry list when it's not a GeometryCollection.

Added a parametrized test covering both Polygon and MultiPolygon cases.
All existing tests + pre-commit checks pass locally. Happy to make any
changes if needed!

@adarshsm adarshsm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, @KhadeerBasha1232 — the normalize-to-a-list approach is the right shape. A few things I hit while testing it against #1816:

  1. Type-key robustness: geometry["type"] == "GeometryCollection" will itself raise KeyError: 'type' if an object omits type. Keying on the member you actually need — "geometries" in geometry — sidesteps that and reads as "does this hold sub-geometries, or is it one itself?"
  2. Test could assert styling, not just rendering: since style_data's job is to attach properties.style, passing a style_function and asserting the style lands would catch a future regression that silently drops styling.
  3. There's a second crash site this doesn't reach. style_data runs before children render, so once it's fixed, a single-geometry TopoJSON with a fields-based GeoJsonPopup/GeoJsonTooltip hits the same assumption again at features.py ~L1205 (["geometries"][0]["properties"]). Minimal repro:
    import folium
    from folium.features import GeoJsonPopup
    topo = {"type": "Topology",
            "objects": {"A": {"type": "Polygon", "arcs": [[0]], "properties": {"name": "A"}}},
            "arcs": [[[0, 0], [100, 0], [100, 100], [0, 100], [0, 0]]],
            "transform": {"scale": [1e-4, 1e-4], "translate": [9.47, 46.34]}}
    m = folium.Map()
    t = folium.TopoJson(topo, "objects.A")
    t.add_child(GeoJsonPopup(fields=["name"]))
    t.add_to(m)
    m.render()  # KeyError: 'geometries'
    I have a fix + regression tests for both sites ready — happy to push it as a follow-up PR once this lands, or you're welcome to fold the second-site fix in here. Whatever you and the maintainers prefer.

@KhadeerBasha1232

Copy link
Copy Markdown
Author

Thanks for the thorough review, @adarshsm — really appreciate you digging into the edge cases and providing the repro script.

Pushed a follow-up commit (7157e9b) that addresses all three points:

Type-key robustness — switched the check to "geometries" in geometry instead of geometry["type"] == "GeometryCollection", so it no longer breaks on objects missing a type key. Added a regression test (test_topojson_non_collection_geometry_without_type) covering that exact case.
Second crash site — extracted a shared _get_geometries() helper and reused it in GeoJsonDetail, fixing the KeyError in GeoJsonPopup/GeoJsonTooltip for non-collection TopoJSON. Verified with your repro script, and added a parametrized test (test_topojson_non_collection_geometry_detail) covering both GeoJsonPopup and GeoJsonTooltip.
Styling assertion — updated the existing test to pass a real style_function and assert the style actually lands in properties.style, rather than just checking the map renders.

All local tests pass (pytest tests/test_features.py) along with pre-commit. Let me know if you'd like anything else adjusted — happy to iterate further.

@adarshsm

Copy link
Copy Markdown

That's a clean set of changes, @KhadeerBasha1232 — the shared _get_geometries() helper reads well, and the parametrized Popup/Tooltip tests cover the second site nicely. Looks complete to me, and pre-commit.ci is green. Thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TopoJSON fails with Polygon/MultiPolygon objects

2 participants