Skip to content

Combine buildSelectList into buildPropertyName - #175

Merged
LionelZoubritzky-IGN merged 12 commits into
mainfrom
refactor-selectlist-attachFeature
Aug 4, 2026
Merged

Combine buildSelectList into buildPropertyName#175
LionelZoubritzky-IGN merged 12 commits into
mainfrom
refactor-selectlist-attachFeature

Conversation

@LionelZoubritzky-IGN

@LionelZoubritzky-IGN LionelZoubritzky-IGN commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This PR merges the two existing implementations of select: one used to exist for the GpfGetFeatures codepath and another for GpfGetFeatureById. They are now grouped into buildPropertyName, which returns a propertyName with the following convention:

  • propertyName is the empty string if and only if the caller is GpfCountFeatures.

  • The geometry can be requested by two different causes:

    • either the tool is GpfGetFeaturesLayer or GpfGetFeatureByIdLayer, in which case it is always requested.
    • either the tool is GpfGetFeatures or GpfGetFeatureById, in which case it is requested if and only if spatial_extra is not empty.

    Be it one case or the other, when the geometry is requested, its name is resolved and included in propertyName. Otherwise, propertyName does not include the name of the geometry.

  • Apart from the geometry, the rest of propertyName are the list of the selected properties. If select is undefined or empty, it is the list of all non-geometric properties of the typename in the catalog.


When the name of the geometry is required, it is resolved by checking that there is a unique geometric property in the catalog. This name is required in two situations:

  • Either the geometry is requested as part of the output of the request: this corresponds to the aforementioned cases, either a Layer tool or a non-empty spatial_extra
  • Either the geometry name only is needed in order to build a CQL filter: this occurs when there is a spatial_filter in the query.

When the geometry name has been resolved, it is stored in the geometryProperty field of the CompiledQuery. It serves as a resolved cache, and also in the case where the catalog is desynced with the actual upstream WFS service: when this occurs and the required geometric property is absent from upstream, a dedicated error is thrown to identify this problem. The geometryProperty field of the CompiledQuery is used in this error, so this field needs to be populated whenever the geometry name is required, including when it is only used as part of a CQL filter (i.e. even if the geometry itself is not requested in the output).


This PR also changes the behavior of requests on typenames that have either no or multiple geometries. Before, those used to throw an error on any request: now, they only throw an error if the geometry is required (spatial_filter / Layer tool / spatial_extra).


Finally, rename attachFeatureRefs into postProcessFeatureCollection to more accurately reflect that this function handles all the post-processing, and does not only attach feature refs anymore.

@LionelZoubritzky-IGN LionelZoubritzky-IGN added refactoring Rethink code architecture codefix Fix a small aspect of the code labels Jul 23, 2026
@LionelZoubritzky-IGN
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
LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from fc26169 to d74b22b Compare July 24, 2026 13:08
@esgn

esgn commented Jul 27, 2026

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

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

esgn commented Jul 30, 2026

Copy link
Copy Markdown
Member

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
LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from 65402f6 to d39196c Compare July 30, 2026 15:14
@LionelZoubritzky-IGN

Copy link
Copy Markdown
Contributor Author

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

@esgn esgn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review

Comment thread src/wfs/queryPreparation.ts Outdated
Comment thread src/wfs/properties.ts Outdated
Comment thread src/wfs/features.ts
Comment thread src/wfs/properties.ts Outdated

@esgn esgn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review

Comment thread src/wfs/properties.ts
Comment thread src/wfs/properties.ts
Comment thread src/wfs/properties.ts
Comment thread src/wfs/properties.ts Outdated
Comment thread src/wfs/properties.ts
Comment thread src/wfs/byId.ts Outdated
Comment thread src/proxy/execute.ts Outdated
Comment thread src/wfs/response.ts Outdated
Comment thread src/wfs/response.ts Outdated
Comment thread src/wfs/response.ts Outdated
Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>
@LionelZoubritzky-IGN
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
…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>
@LionelZoubritzky-IGN
LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch 2 times, most recently from 3ee9384 to 438656e Compare July 31, 2026 14:35
… queries)

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>
@LionelZoubritzky-IGN
LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from 68595e4 to 0c810c5 Compare July 31, 2026 14:36
@LionelZoubritzky-IGN LionelZoubritzky-IGN changed the title Refactor selectlist attach feature Combine buildSelectList into buildPropertyName Aug 3, 2026
Comment thread src/wfs/queryPreparation.ts
Comment thread test/tools/wfs/getFeaturesLayer.test.ts
Comment thread src/wfs/queryPreparation.ts
Comment thread src/wfs/queryPreparation.ts Outdated
LionelZoubritzky-IGN and others added 3 commits August 4, 2026 12:55
…re from GetFeature inputs

Emmanuel S. <5435148+esgn@users.noreply.github.com>
…atial filter uses it.

Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>
Co-authored-by: Emmanuel S. <5435148+esgn@users.noreply.github.com>
@LionelZoubritzky-IGN
LionelZoubritzky-IGN force-pushed the refactor-selectlist-attachFeature branch from a4808e0 to 34ff907 Compare August 4, 2026 12:18
@LionelZoubritzky-IGN
LionelZoubritzky-IGN merged commit 0412cef into main Aug 4, 2026
6 checks passed
@LionelZoubritzky-IGN
LionelZoubritzky-IGN deleted the refactor-selectlist-attachFeature branch August 4, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codefix Fix a small aspect of the code refactoring Rethink code architecture

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants