FiboaBaseConverter: make id mean something

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:

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()):

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)

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"]}))