REST converters: page by id windows

[ivorbosloper]

Paging by sortBy + attribute > last_id sorts the whole layer per page: ~100 s per request on Cantabria's joined layers, and Galicia is 15.8M recintos per edition. Half-open id windows remove the sort, and pages cached by the old scheme are reused where their ids prove they cover a window.

Three bugs found on the way, none visible in the output:

layer ids repeat across services	every SIXPAC_<year> has Recintos at id 2, so one year's cache served another
an Esri error body is HTTP 200	it was written to the cache and read back forever
joined layers qualify field names	RECINTOS.OBJECTID, so the paging filter matched nothing and returned everything

A second commit adds three things the Balearic services need, each of which a converter had a copy of: a joined layer's table prefixes are stripped for every converter (so es_ib and es_cb lose theirs), a variant whose value is a URL names the service that edition lives in, and the id-bound query — the one sorted query left — is retried, because that proxy answers two in three with a 502.

Not a copy: main's rest_format and where-clause work is kept. tests/test_converter_rest.py is new; mixin coverage 19% → 94%.

[Copilot]

🟡 Changes recommended

Cache provenance remains unsafe, and the Balearic server-side filter still uses an unqualified joined-layer field.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Reworks Esri REST paging for faster, safer conversion and expands Balearic/Cantabrian edition support.

Changes:

• Pages REST layers using ID windows with service-aware caching and error cleanup.
• Handles qualified joined-layer fields and URL-backed variants.
• Adds extensive REST and ES-IB conversion coverage.

File summaries

File	Description
fiboa_cli/conversion/converter_rest.py	Implements window paging, cache handling, retries, and field normalization.
fiboa_cli/datasets/es_ib.py	Adds current/historic services and snapshot-date handling.
fiboa_cli/datasets/es_cb.py	Updates editions, licensing, and determination dates.
tests/test_converter_rest.py	Covers REST paging and caching behavior.
tests/test_converters.py	Updates REST parameter assertions.
tests/test_convert.py	Tests current and historic ES-IB variants.
tests/data-files/convert/es_ib/es_ib_2024.geojson	Provides historic joined-layer test data.
CHANGELOG.md	Documents converter changes.

Review details

Suppressed comments (1)

fiboa_cli/conversion/converter_rest.py:135

• The new cache key still omits the effective filter/variant. For example, DESTConverter changes rest_params['where'] per edition while using the same service and layer (fiboa_cli/datasets/de_st.py:54-61), so a matching ID window cached for one edition can be returned for another. Include a stable fingerprint of the effective query (at least the variant and rest_params) in the cache key.

                    cache_file = os.path.join(
                        cache_folder,
                        f"{self.id}_{service}_{layer['id']}_r{lo}.{self.rest_format}",
                    )

• Files reviewed: 7/9 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ivorbosloper]

The mechanism is right: the legacy name is {id}_{layer}_{key}, so it carries the dataset and the layer but not the service or the variant, and the check only proves the id range. Two services that number their layers the same way would be indistinguishable — which is exactly the case this PR's own comment cites, every SIXPAC_<year> MapServer serving Recintos at layer 2.

What saves it today is an accident of the one dataset that has such pages. On the machine that builds the catalog there are 9,306 legacy pages, all es_cb, and Cantabria gives every edition its own layer id — 3, 7, 11, 15, 19 … 71 — so a page belongs to exactly one edition. es_ib, es_ga and es_cm have none at all.

An accident is not a guarantee, and the saving is bounded: those 9,306 pages are one dataset's, against 210,053 already cached under the new key. I would rather drop the reuse than keep a path that cannot prove where its data came from. Alternatively it could be restricted to converters without variants, where the ambiguity cannot arise — say which you prefer and I will make the change.

[ivorbosloper]

The server resolves the unqualified name. Checked against the published editions, counting rows carrying any of the twelve excluded codes:

2022: 478,041 rows, 0 with an excluded use code
2026: 349,087 rows, 0 with an excluded use code

If the predicate were rejected the request would fail, and if it were ignored those codes would be there — AG, ED and ZU are common SIGPAC uses, so zero out of 349,087 is not chance. ArcGIS accepts the bare field name in where on these joined layers even though it answers with the qualified one.

[ivorbosloper]

Right about the mechanism, wrong about the consequence. With A.USO and B.USO and no bare USO, the first is renamed and the second is skipped because the suffix is already a value in renames, so columns.duplicated() sees two different names and keeps both — B.USO does stay qualified.

It does not reach the output, because a column no columns entry maps is dropped before the file is written. So the documented behaviour holds — the first table wins — and the leftover is discarded rather than published with its prefix.

Happy to make it explicit by dropping the remaining qualified columns instead of leaving them to be swept up later, if you would rather the function finish its own job.