Validations on top of #187 by esgn · Pull Request #201 · ignfab/geocontext

esgn · 2026-09-28T14:56:48Z

Description

Four commits on top of #187: settle the spatial_extras contract, move input-only checks to zod.

Related issues (if applicable)

Related to #187, #136

Motivation

Follow-up of the #187 review (null/0 contract, "requires a filter" check, by-id layer depending on
format, extras only covering the returned page).

Implementation

null when an extra cannot be computed, 0 only for a computed empty overlap; empty geometry → all null.
"requires a filter" moves to zod (invalid-tool-params); distance_to_filter + intersects_point_filter rejected.
No format read when spatial_extras is empty (by-id layer, proxy).
Descriptions and error hint: extras cover returned features only, not usable in where/order_by.

Testing

Typecheck OK, 479/480 unit tests (only the known flaky wall-clock test fails), docs regenerated.

TODOs

Before merge:

intersection_area perf with jsts prepared geometries
Flaky wall-clock test test/wfs/response.test.ts:278

After merge (issues to create):

Force srsName=EPSG:4326 on MCP requests
Extract formatDocNames in schema.ts
Validation cleanup: safety-net tests, remaining input rules to zod, dimension model, typed errors, catalog checks before network, spatial_extras registry, single spatial filter, reference geometry guard (isGeometryLike lets empty coordinates through)

Checklist

The PR is focused and of a reasonable size.
The commit history is clean.
Relevant documentation has been updated.
Relevant tests have been added or updated.

LionelZoubritzky-IGN · 2026-09-28T15:55:49Z

+/** True when `geometry` is a GeoJSON geometry holding at least one finite position. */
+function isComputableGeometry(geometry: unknown) : geometry is Geometry {
+  try {
+    return bbox(geometry as Geometry).every(Number.isFinite);
+  } catch {
+    return false;
+  }
+}
+


OK, I see this is here to guard against ill-formed geometries like { type: "Point", coordinates: [] }. Do you think we should replace isGeometryLike by this function?

Replacing it would lose the shape check: isComputableGeometry only asks for one finite position (it also accepts a Feature or a GeometryCollection, which geometryToEwkt can't write). But you're right that isGeometryLike is too weak: {type: "Polygon", coordinates: [] } passes, becomes POLYGON() and the WFS silently returns 0 features. So I'd use both.
It predates #187, so I've added it to the post-merge "validation cleanup" item in the PR description.

LionelZoubritzky-IGN · 2026-09-28T16:11:01Z

+  distance_to_filter: {
+    incompatibleFilter: "intersects_point_filter",
+    incompatibleReason: "vaut toujours 0 avec `intersects_point_filter`, puisque chaque objet renvoyé contient le point : pour classer des objets selon leur distance à un point, utilisez plutôt `dwithin_point_filter`",
+  },


The design is problematic: what if a spatial_extra has multiple incompatible filters? This is not only theoretic, there is actually one forgotten case here:

distance_to_filter: { incompatibleFilter: "intersects_feature_filter", incompatibleReason: "vaut toujours 0 avec `intersects_feature_filter`, puisque chaque objet renvoyé doit croiser le filtre : pour classer des objets selon leur distance à un point, utilisez plutôt `dwithin_point_filter`". }

so I think FILTER_DEPENDENT_EXTRA_RULES should be a Record<..., Array{ {...} }> instead.

In theory bbox_filter should also be part of it, except there is #196 currently

Not with the contract we settled in #136 (#136 (comment)): the distance goes from the filter center (the reference's centroid for intersects_feature_filter) to the closest point of the feature, hence the rename to distance_to_filter_center.
So it isn't always 0: among communes crossing a département, only the one containing its centroid gets 0. intersects_point_filter stays the only trivial case for both extras, so one entry per extra is enough; we can switch to an array if a second incompatible filter ever shows up.

The design is still not very flexible, but agreed: we can decide to fix that later if ever need be.

LionelZoubritzky-IGN · 2026-09-28T16:21:05Z

      "`distance_to_filter` est la distance (en m) entre la géométrie de l'objet renvoyé et le centroïde du filtre spatial (le point de départ dans le cas de `travel_time_filter`).\n"+
-      "`intersection_area` est l'aire (en m²) de la partie de l'objet renvoyé située dans le filtre spatial. L'objet et le filtre doivent être surfaciques, sinon la valeur est `null` ; `0` signifie que l'objet ne recouvre pas le filtre."
+      "`intersection_area` est l'aire (en m²) de la partie de l'objet renvoyé située dans le filtre spatial. L'objet et le filtre doivent être surfaciques, sinon la valeur est `null` ; `0` signifie que l'objet ne recouvre pas le filtre.\n"+
+      "`distance_to_filter` et `intersection_area` exigent un filtre spatial autre que `intersects_point_filter`."


et `distance_to_filter` un filtre spatial autre que `intersects_feature_filter`.

But I think it's even better to omit this sentence from the description altogether, because it's useless. Either the LLM is stupid and it's not helpful because it may do the stupid thing we are telling it not to do anyway, or it's not stupid, so it won't attempt to do it. It will get a nice error that explains what's going wrong anyway, so I think we should avoid being extra-verbose in the description.

Same answer as in #201 (comment): with the contract in #136, the distance goes from the reference's centroid, so intersects_feature_filter doesn't make it trivial and needs no mention. Agreed that the intersects_point_filter exception is redundant with the error, so I'd drop it but keep "exigent un filtre spatial", which tells the LLM what the call needs before it fails.

LionelZoubritzky-IGN · 2026-09-28T16:25:36Z

+      for (const filter of [
+        { bbox_filter: { west: 2.1, south: 48.7, east: 2.5, north: 48.9 } },
+        { dwithin_point_filter: { lon: 2.3, lat: 48.8, distance_m: 500 } },
+        { intersects_feature_filter: { typename: "ADMINEXPRESS-COG.LATEST:departement", feature_id: "departement.1" } },


Should fail for distance_to_filter

Not with the contract in #136: the distance goes from the reference's centroid, not from its geometry, so it isn't always 0 with intersects_feature_filter (see #201 (comment)). The test stays as is.

…erlap - intersection_area: null when the feature or the filter is not areal, or when the filter geometry could not be prepared; 0 only when both are areal and do not overlap - every extra is null on a missing or empty geometry - length sums the linear parts of a GeometryCollection; area of an empty Polygon is null - measures sum the parts of a GeometryCollection, overlaps included (JTS) - state the contract in deriveFromGeometry and in the LLM-facing descriptions - regenerate docs/mcp-tools.md

…hema - move "requires a spatial filter" from compileQueryParts to the zod refinement: invalid-tool-params instead of execution-error, raised before any catalog or network call - reject distance_to_filter with intersects_point_filter (always 0) - list only the compatible filters in the error message - state the rule in the spatial_extras description, regenerate docs/mcp-tools.md - move the related tests from compileQueryParts to the tool

buildPropertyNameWithGeometry always passes an empty spatial_extras list, and the `!spatial_extras` guard let it through: the by-id layer tool and the proxy by-id resolve then read the geometry format and threw on an unlisted or missing one. Return early on an empty list, so already minted by-id layer URLs do not depend on the catalog format values.

…turned features - tool and spatial_extras descriptions: extras are computed after the query, on the returned features only, and cannot be used in where/order_by; a ranking or a sum needs numberReturned == numberMatched - "property does not exist" error: explain when the name is a spatial extra (message only, a real catalog column with that name is still accepted)

esgn requested a review from LionelZoubritzky-IGN September 28, 2026 14:58

esgn changed the title ~~Esgn/187 validations~~ Validations on top of #187 Sep 28, 2026

LionelZoubritzky-IGN reviewed Sep 28, 2026

View reviewed changes

esgn added 3 commits September 29, 2026 18:00

LionelZoubritzky-IGN force-pushed the esgn/187-validations branch from 92d4127 to fe9006a Compare September 29, 2026 16:00

esgn force-pushed the esgn/187-validations branch from fe9006a to 66bb1f0 Compare September 30, 2026 08:23

LionelZoubritzky-IGN approved these changes Sep 30, 2026

View reviewed changes

esgn merged commit 7b95493 into 136/more-spatial_extras Sep 30, 2026

esgn mentioned this pull request Sep 30, 2026

Add geometry metadata in gpf_wfs_get_features response #136

Open

4 tasks

Navigation Menu

Validations on top of #187 - #201

Validations on top of #187#201
esgn merged 4 commits into
136/more-spatial_extrasfrom
esgn/187-validations

esgn commented Sep 28, 2026 •

edited

Loading

Uh oh!

LionelZoubritzky-IGN Sep 28, 2026

Uh oh!

esgn Sep 28, 2026

Uh oh!

LionelZoubritzky-IGN Sep 28, 2026

Uh oh!

LionelZoubritzky-IGN Sep 28, 2026

Uh oh!

esgn Sep 30, 2026

Uh oh!

LionelZoubritzky-IGN Sep 30, 2026

Uh oh!

LionelZoubritzky-IGN Sep 28, 2026

Uh oh!

esgn Sep 30, 2026

Uh oh!

LionelZoubritzky-IGN Sep 28, 2026

Uh oh!

esgn Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

esgn commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues (if applicable)

Motivation

Implementation

Testing

TODOs

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

esgn commented Sep 28, 2026 •

edited

Loading