diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d67a81b..cb6dbc36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - ES-MD: find RECINTO.shp wherever the archive puts it - DE-NDS: give the collection an id (the row index), which it was published without - DE-BB: ref_ident holds the FLIK (field block reference), not a farmer, and the shapefile is cp1252 +- Converters must map something to `id`, that column must be in the source, and its values must be unique — checked before a conversion starts and before geometries are exploded +- A converter may no longer declare both `sources` and `variants`, where `sources` silently won and every `--variant` converted the same file - Converters drop rows that cannot validate — no id, no crop:code, no geometry — up to 1% of a file, and fail rather than drop beyond that - Every schema a conversion needs is fetched upfront with retries, so a transient outage cannot kill a long run at the last step - Update vecorel-cli to v0.2.17: diff --git a/fiboa_cli/conversion/fiboa_converter.py b/fiboa_cli/conversion/fiboa_converter.py index 337dc67f..12c9ed48 100644 --- a/fiboa_cli/conversion/fiboa_converter.py +++ b/fiboa_cli/conversion/fiboa_converter.py @@ -25,9 +25,96 @@ def __init__(self, *args, **kwargs): self.columns = {**self.columns, "determination:datetime": "determination:datetime"} def convert(self, *args, **kwargs): + self._require_id_mapping() + self._require_one_source_of_urls() self._prewarm_schemas() return super().convert(*args, **kwargs) + def _require_one_source_of_urls(self): + """Fail when both `sources` and `variants` are declared. + + The base converter takes `sources` when it is set and ignores the + variants entirely, so `--variant 2011` silently converts whatever + `sources` points at. hr declared both and would have published thirteen + copies of the current file under thirteen different years. A converter + that inherits variants it does not want says so with `variants = {}`. + """ + if self.sources and self.variants: + raise ValueError( + f"{type(self).__name__} declares both sources and variants; sources wins " + "and every --variant would convert the same file. Drop sources, or set " + "variants = {} when the inherited ones do not apply." + ) + + def _require_unique_ids(self, gdf): + """Fail when the column that becomes `id` does not identify a field. + + fiboa asks for one identifier per field, and the catalog documents `id` + as unique within an edition, but nothing measured it: es_cl published + 9,109,136 fields whose id was the string "0", us_usda_cropland mapped a + group id shared by thousands of fields, and every converter that reads + several files and sets `index_as_id` repeated the same index once per + file. This runs before geometries are exploded, so it judges what the + converter assigned rather than the split parts of one source feature. + + A missing id is a different failure, dropped under a bounded rule a few + lines below, so it is not counted here: es_cl's C_REFREC identifies all + 13,022,051 recintos except the 20 that carry none, and reading those as + repeats rejected a perfectly good identifier. + """ + sources = [ + k + for k, v in self.columns.items() + if "id" in (v if isinstance(v, (list, tuple)) else [v]) + ] + column = next( + (c for c in sources if c in gdf.columns), "id" if "id" in gdf.columns else None + ) + if column is None: + # The mapping exists (convert() checks that) but the data does not + # carry it, and the unlisted-column drop then writes a file with no + # id that validates — si's 2019 campaign names the field POLJINA_ID. + raise ValueError( + f"{type(self).__name__}: none of the columns mapped to 'id' " + f"({', '.join(sources) or 'none'}) is in this source; it has " + f"{', '.join(sorted(gdf.columns)[:12])}" + ) + ids = gdf[column].dropna() + if ids.is_unique: + return + counts = ids.value_counts() + duplicated = int(len(ids) - len(counts)) + worst = int(counts.iloc[0]) + raise ValueError( + f"{type(self).__name__}: '{column}' is not unique — {duplicated:,} of {len(ids):,} " + f"rows repeat an id (one appears {worst:,} times), so it cannot be `id`. Map a column " + "that identifies a field, build one from the source's key columns, or use the row " + "index (index_as_id) only when the conversion reads a single file." + ) + + def _require_id_mapping(self): + """Fail before converting when nothing will end up as `id`. + + Every collection needs the identifier, and nothing downstream enforces + it: the base converter drops columns no mapping names, so a converter + without one simply writes a file without `id` and validates. That is + how de_bb and sk reached the catalog without it, and sk shows the + subtler half — `index_as_id = True` fills the column and the same drop + step removes it again, because `columns` never named it. A converter + with no natural key sets both `index_as_id` and `"id": "id"`. + """ + targets = set() + for value in list(self.columns.values()) + list(self.column_additions or {}): + targets.update(value if isinstance(value, (list, tuple)) else [value]) + if "id" not in targets: + hint = ( + ' — `index_as_id = True` is set, so add \'"id": "id"\' to columns' + if getattr(self, "index_as_id", False) + else " — map a unique source column to it, or set index_as_id = True" + ' and add \'"id": "id"\' to columns' + ) + raise ValueError(f"{type(self).__name__} maps no column to 'id'{hint}") + def _prewarm_schemas(self): """Fetch every schema this conversion will need before doing any real work. @@ -78,6 +165,7 @@ def _required_properties(self) -> set[str]: def post_migrate(self, gdf): gdf = super().post_migrate(gdf) + self._require_unique_ids(gdf) # post_migrate runs before columns are renamed, so look up the source column for key in sorted(self._required_properties()): diff --git a/fiboa_cli/datasets/ec_be_vlg.py b/fiboa_cli/datasets/ec_be_vlg.py index 5f1c048c..c7794317 100644 --- a/fiboa_cli/datasets/ec_be_vlg.py +++ b/fiboa_cli/datasets/ec_be_vlg.py @@ -9,6 +9,10 @@ class ECConverter(EuroCropsConverterMixin, BEVLGBaseConverter): "BE_VLG_2021/BE_VLG_2021_EC21.shp" ] } + # This is the single 2021 EuroCrops release, not the yearly agpa downloads + # the Flemish parent declares; say so rather than leaning on `sources` + # taking precedence over inherited variants. + variants = {} def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/tests/test_converters.py b/tests/test_converters.py index 680d94ff..7d04e378 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -1,11 +1,14 @@ from urllib.parse import parse_qs, urlparse import geopandas as gpd +import pandas as pd +import pytest import spdx_license_list from shapely.geometry import Point from vecorel_cli.vecorel.schemas import VecorelSchema from vecorel_cli.vecorel.util import load_file +from fiboa_cli.conversion.fiboa_converter import FiboaBaseConverter from fiboa_cli.converters import Converters from fiboa_cli.fiboa.version import get_fiboa_uri @@ -104,3 +107,57 @@ def test_overriden_base_properties(): assert s == converter_properties[property], ( "Converter {converter} overrides schema for base property {property}" ) + + +def test_every_converter_maps_an_id(): + """ + Nothing downstream enforces `id`: columns without a mapping are dropped, so a + converter that never names one writes a valid file without the identifier + every collection needs. de_bb and sk were published that way. + """ + c = Converters() + for _id in Converters().list_ids(): + c.load(_id)._require_id_mapping() + + +def test_no_converter_declares_both_sources_and_variants(): + """ + `sources` wins over `variants` in the base converter, so a converter with + both converts the same file whatever --variant asks for — silently. + """ + c = Converters() + for _id in Converters().list_ids(): + c.load(_id)._require_one_source_of_urls() + + +def test_unique_id_check_ignores_missing_ids(): + """ + A row without an id is dropped downstream under a bounded rule, so it is not + a repeat: counting nulls as repeats rejected es_cl's C_REFREC, which + identifies every one of the 13,022,051 recintos that carries it. + """ + + class Converter(FiboaBaseConverter): + id = "test_unique_ids" + columns = {"geometry": "geometry", "REF": "id"} + + converter = Converter() + converter._require_unique_ids(pd.DataFrame({"REF": ["a", "b", None, None]})) + + with pytest.raises(ValueError, match="2 of 3 rows repeat an id"): + converter._require_unique_ids(pd.DataFrame({"REF": ["a", "a", "a", None]})) + + +def test_unique_id_check_needs_the_column_in_the_source(): + """ + The mapping existing is not enough: si's 2019 campaign names the field + POLJINA_ID where every later one names it ID, and the unlisted-column drop + then wrote 820,151 fields with no id at all — which validated. + """ + + class Converter(FiboaBaseConverter): + id = "test_missing_id_column" + columns = {"geometry": "geometry", "ID": "id"} + + with pytest.raises(ValueError, match="none of the columns mapped to 'id'"): + Converter()._require_unique_ids(pd.DataFrame({"POLJINA_ID": ["a", "b"]}))