Refactor selectlist attach feature by LionelZoubritzky-IGN · Pull Request #175 · ignfab/geocontext

LionelZoubritzky-IGN · 2026-07-23T09:43:16Z

Fix two code inconsistencies:

77570a9 merges the two existing implementations of the select: one exists for the GpfGetFeatures codepath and another for GpfGetFeatureById, while they should be identical.
d74b22b renames attachFeatureRefs into postProcessFeatureCollection to more accurately reflect that this function handles all the post-processing, and does not only attach feature refs anymore.

Pure refactors, no impact on behavior.

esgn · 2026-07-27T08:45:03Z

The default parameter geometryProperty = getGeometryProperty(featureType) is evaluated whenever the argument is not provided, even when the geometry is not needed (no select, no spatial_extras). Yet getGeometryProperty throws on a type without any geometry property.

Consequence: a bare gpf_get_feature_by_id on wfs_scot:doc_urba or wfs_scot:doc_urba_com (the 2 geometry-less types in the embedded catalog) now fails before even issuing the WFS request, whereas it worked on main. The old buildPropertyName in byId.ts only resolved the geometry when necessary — that was the invariant documented by the comment removed along with the function.

LionelZoubritzky-IGN · 2026-07-27T11:36:34Z

Thank you for catching that regression! Addressed and tested in 25871c3

Additionally, when doing a GetFeatureById with empty select and spatial_extra, the query used to have an empty propertyName, which implies fetching the entire geometry, and then discarding it (and replacing it will null). The refactor already changed that, and the latest commit tests this case. It also add tests for the case of collection without geometries, like wfs_scot:doc_urba.

esgn · 2026-07-30T13:43:42Z

Have a look at gpf_get_features_layer { typename: "wfs_scot:doc_urba" } => I assume this will cause an error on the proxy side.

LionelZoubritzky-IGN · 2026-07-30T15:25:26Z

Good catch, thanks! Fixed in 65402f6
This surfaced another issue but I'm opening a separate PR for that: #185

esgn

First review

esgn · 2026-07-30T13:18:35Z

    // Rewrite an embedded-catalog geometry-column desync into a clear diagnostic
    // (shared with the proxy path); any other error passes through unchanged.
-    rethrowIdentifiedCatalogDesyncError(error, compiled.geometryProperty.name, input.typename);
+    if (compiled.geometryProperty) {


compileQueryParts now only populates compiled.geometryProperty inside if (spatialFilter), but buildPropertyName still forces the geometry column into propertyName whenever spatial_extras is non-empty resolving it locally (properties.ts:142) without handing it back. So for a query with spatial_extras and no spatial filter, the column is in the request while this field is undefined, and the rewrite is skipped.

The two names have drifted apart:

compiled.geometryProperty now means "column used by the spatial filter"

catalogDesync needs "column injected into the request"

Fixed and tested in 1842373

esgn · 2026-07-30T14:10:19Z

+  if (includeGeometry) {
+    // ensure that the geometric property exists
+    geometryProperty ?? getGeometryProperty(featureType);
+    return ""; // return all properties


So, there, if select is not there and includeGeometry is true we return "" so no propertyName will appear in the WFS query and every column will be returned. If we were to omit columns from the catalogue we will still return all the WFS columns.

0c810c5

This commit makes it so that all properties are now explicitly returned and the resulting propertyName is never empty

esgn

Second review

esgn · 2026-07-30T20:00:56Z

-) {
-  const shouldIncludeGeometry = (input.spatial_extras ?? []).length > 0;
+  select?: string[],
+  spatial_extras?: string[],


spatial_extras is never read in the body, its only uses are this declaration, the JSDoc, and the
default value of includeGeometry.
buildPropertyNameWithGeometry(ft, select, spatial_extras)
silently ignores its 3rd argument, since it forces includeGeometry: true. Both real callers pass only (featureType, select).

Maybe drop spatial_extras from both signatures and let each caller compute includeGeometry
explicitly at the call site (it already did before this PR).

I prefer the current design, it avoids duplicating the logic to detect whether or not includeGeometry should be set. That was actually one of the main points of this refactor.

However, I agree that spatial_extras is irrelevant for buildPropertyNameWithGeometry so I removed the spatial_extras argument from buildPropertyNameWithGeometry in 0c810c5

esgn · 2026-07-30T20:03:44Z

+
+  if (includeGeometry) {
+    // ensure that the geometric property exists
+    geometryProperty ?? getGeometryProperty(featureType);


geometryProperty ?? getGeometryProperty(featureType); is an expression statement whose result is
discarded. It only exists to throw. And the only caller with includeGeometry: true always passes
geometryProperty, so the right-hand side never runs.

Yes, the result is discarded, it only exists to throw. But no, the right-hand side can run, because includeGeometry can be set on the other caller if spatial_extra is not empty. That would be the case of a GetFeatures call with empty select but non-empty spatial_extra on a geometry-less collection: this is tested in the new tests of this PR at https://github.com/ignfab/geocontext/pull/175/changes#diff-188df1952a07ac03771df78672fa7af090aa35892cbc19c19840fa99b6e3f970R437-R459

esgn · 2026-07-30T20:05:58Z

This comment is now false: it promises appending the geometry column "only when spatial_extras needs it", but the return "" above exits before reaching here. A reader concludes main's behavior is preserved.

The comment that carried the why (// Include geometry when spatial_extras needs it to derive bbox/centroid/...) was dropped with no replacement.

Probably a good idea to restore the why on the branch that actually appends (:141), and cut this one down to what the code below really does.

0c810c5

I move the comment above and slightly reword it.

esgn · 2026-07-30T20:07:14Z

 * @param geometryProperty Geometry property already resolved for the feature type.
- * @param input Normalized tool input.
+ * @param includeGeometry Override boolean to force including the geometry in the return query.
 * @returns The list of property names to expose in the WFS `propertyName` parameter.


Will have to be updated based on decisions deriving from other comments

Nothing to do so far, but keeping this open while we don't have a common conclusion on the issues above.

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

…sInput Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

…rty is part of the request Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

… queries) Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

LionelZoubritzky-IGN added this to the v0.10.x MCP and WFS Stabilization to improve reliability milestone Jul 23, 2026

LionelZoubritzky-IGN requested a review from esgn July 23, 2026 09:43

LionelZoubritzky-IGN added refactoring Rethink code architecture codefix Fix a small aspect of the code labels Jul 23, 2026

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from e55fafe to fc26169 Compare July 23, 2026 09:45

Base automatically changed from 160/stateless-proxy to main July 24, 2026 12:37

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from fc26169 to d74b22b Compare July 24, 2026 13:08

LionelZoubritzky-IGN added 3 commits July 30, 2026 11:39

refactor: merge buildSelectList into buildPropertyName

7e158de

refactor: attachFeatureRefs renamed to postProcessFeatureCollection

e8458e2

fix: buildPropertyName should not eagerly fetch geometryProperty

2b9817e

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from 25871c3 to 2b9817e Compare July 30, 2026 09:40

fix: eagerly fail when requesting layer on a collection without geometry

d39196c

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from 65402f6 to d39196c Compare July 30, 2026 15:14

esgn requested changes Jul 30, 2026

View reviewed changes

chore: misc and doc update

9c906c7

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from a6773c0 to 9c906c7 Compare July 31, 2026 12:09

LionelZoubritzky-IGN and others added 3 commits July 31, 2026 16:26

chore: rename GpfGetFeatureCoreInput into FeatureCollectionPostProces…

d03f863

…sInput Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

fix: ensure geometryProperty is populated whenever the geometry prope…

1842373

…rty is part of the request Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

chore: use CompiledQuery for buildMainRequest "compiled" input

d058a7d

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch 2 times, most recently from 3ee9384 to 438656e Compare July 31, 2026 14:35

fix: always return a non-empty propertyName (except for CountFeatures…

0c810c5

… queries) Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>

LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from 68595e4 to 0c810c5 Compare July 31, 2026 14:36

Uh oh!

Conversation

LionelZoubritzky-IGN commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

LionelZoubritzky-IGN commented Jul 27, 2026

Uh oh!

esgn commented Jul 30, 2026

Uh oh!

LionelZoubritzky-IGN commented Jul 30, 2026

Uh oh!

esgn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

esgn left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LionelZoubritzky-IGN commented Jul 23, 2026 •

edited

Loading

esgn commented Jul 27, 2026 •

edited

Loading