Featureset validation and QRF behavior alignment with gl-native (inte…

…rnal-2077)
mapbox · Dec 23, 2024 · bf77519 · bf77519
1 parent 6689a84
commit bf77519
Show file tree

Hide file tree

Showing 20 changed files with 140 additions and 206 deletions.
diff --git a/src/data/evaluation_feature.ts b/src/data/evaluation_feature.ts
@@ -6,7 +6,7 @@ import type {VectorTileFeature} from '@mapbox/vector-tile';
 export type EvaluationFeature = {
     readonly type: 0 | 1 | 2 | 3 | 'Unknown' | 'Point' | 'LineString' | 'Polygon';
     readonly id?: any;
-    readonly properties: {
+    properties: {
         [_: string]: any;
     };
     readonly patterns?: {

diff --git a/src/data/feature_index.ts b/src/data/feature_index.ts
@@ -241,8 +241,10 @@ class FeatureIndex {
             // Iterave over all targets to check if the feature should be included and add feature variants if necessary
             let shouldInclude = false;
             for (const target of targets) {
+                this.updateFeatureProperties(geojsonFeature, target);
                 const {filter} = target;
                 if (filter) {
+                    feature.properties = geojsonFeature.properties;
                     if (filter.needGeometry) {
                         const evaluationFeature = toEvaluationFeature(feature, true);
                         if (!filter.filter(new EvaluationParameters(this.tileID.overscaledZ), evaluationFeature, this.tileID.canonical)) {
@@ -322,8 +324,10 @@ class FeatureIndex {
         // Iterave over all targets to check if the feature should be included and add feature variants if necessary
         let shouldInclude = false;
         for (const target of targets) {
+            this.updateFeatureProperties(geojsonFeature, target);
             const {filter} = target;
             if (filter) {
+                feature.properties = geojsonFeature.properties;
                 if (filter.needGeometry) {
                     if (!filter.filter(new EvaluationParameters(this.tileID.overscaledZ), feature, this.tileID.canonical)) {
                         continue;
@@ -347,19 +351,7 @@ class FeatureIndex {
         }
     }
 
-    /**
-     * Create a feature variant for a query target and add it to the original feature.
-     *
-     * @param {Feature} feature The original feature.
-     * @param {QrfTarget} target The target to derive the feature for.
-     * @returns {Feature} The derived feature.
-     */
-    addFeatureVariant(feature: Feature, target: QrfTarget, availableImages?: Array<string>) {
-        const variant: FeatureVariant = {
-            target: target.target,
-            namespace: target.namespace,
-        };
-
+    updateFeatureProperties(feature: Feature, target: QrfTarget, availableImages?: Array<string>) {
         if (target.properties) {
             const transformedProperties = {};
             for (const name in target.properties) {
@@ -371,11 +363,27 @@ class FeatureIndex {
                     feature.tile,
                     availableImages
                 );
-
                 if (value != null) transformedProperties[name] = value;
             }
+            feature.properties = transformedProperties;
+        }
+    }
 
-            variant.properties = transformedProperties;
+    /**
+     * Create a feature variant for a query target and add it to the original feature.
+     *
+     * @param {Feature} feature The original feature.
+     * @param {QrfTarget} target The target to derive the feature for.
+     * @returns {Feature} The derived feature.
+     */
+    addFeatureVariant(feature: Feature, target: QrfTarget, availableImages?: Array<string>) {
+        const variant: FeatureVariant = {
+            target: target.target,
+            namespace: target.namespace,
+        };
+
+        if (target.properties) {
+            variant.properties = feature.properties;
         }
 
         feature.variants = feature.variants || {};

diff --git a/src/style/style.ts b/src/style/style.ts
@@ -2118,11 +2118,28 @@ class Style extends Evented<MapEvents> {
     setFeaturesetSelectors(featuresets?: FeaturesetsSpecification) {
         if (!featuresets) return;
 
+        const sourceInfoMap: { [sourceInfo: string]: string } = {};
+        // Helper to create consistent keys
+        const createKey = (sourceId: string, sourcelayerId: string = '') => `${sourceId}::${sourcelayerId}`;
+
         this.featuresetSelectors = {};
         for (const featuresetId in featuresets) {
             const featuresetSelectors: FeaturesetSelector[] = this.featuresetSelectors[featuresetId] = [];
             for (const selector of featuresets[featuresetId].selectors) {
-
+                if (selector.featureNamespace) {
+                    const layer = this.getOwnLayer(selector.layer);
+                    if (!layer) {
+                        warnOnce(`Layer is undefined for selector: ${selector.layer}`);
+                        continue;
+                    }
+                    const sourceKey = createKey(layer.source, layer.sourceLayer);
+                    // Based on spec, "If the underlying source is the same for multiple selectors within a featureset, the same featureNamespace should be used across those selectors."
+                    if (sourceKey in sourceInfoMap && sourceInfoMap[sourceKey] !== selector.featureNamespace)  {
+                        warnOnce(`"featureNamespace ${selector.featureNamespace} of featureset ${featuresetId}'s selector is not associated to the same source, skip this selector`);
+                        continue;
+                    }
+                    sourceInfoMap[sourceKey] = selector.featureNamespace;
+                }
                 let properties;
                 if (selector.properties) {
                     for (const name in selector.properties) {

diff --git a/src/util/vectortile_to_geojson.ts b/src/util/vectortile_to_geojson.ts
@@ -107,12 +107,12 @@ export type FeatureVariant = {
  * `TargetFeature` is a [GeoJSON](http://geojson.org/) [Feature object](https://tools.ietf.org/html/rfc7946#section-3.2) representing a feature
  * associated with a specific query target in {@link Map#queryRenderedFeatures}. For featuresets in imports, `TargetFeature` includes a `target` reference as a {@link TargetDescriptor}
  * and may also include a `namespace` property to prevent feature ID collisions when layers defined in the query target reference multiple sources.
- * Unlike features returned for root style featuresets, `TargetFeature` omits the `layer`, `source`, and `sourceLayer` properties.
+ * Unlike features returned for root style featuresets, `TargetFeature` omits the `layer`, `source`, and `sourceLayer` properties if the feature belongs to import style.
  */
 export class TargetFeature extends Feature {
-    override layer: never;
-    override source: never;
-    override sourceLayer: never;
+    override layer;
+    override source;
+    override sourceLayer;
     override variants: never;
 
     /**
@@ -135,6 +135,12 @@ export class TargetFeature extends Feature {
         this.target = variant.target;
         this.namespace = variant.namespace;
         if (variant.properties) this.properties = variant.properties;
+
+        if (this.target && (('featuresetId' in this.target && !this.target.importId) || ('layerId' in this.target))) {
+            this.source = feature.source;
+            this.sourceLayer = feature.sourceLayer;
+            this.layer = feature.layer;
+        }
     }
 
     override toJSON(): GeoJSON.Feature & FeatureVariant {

diff --git a/test/integration/query-tests/featuresets/import-featureset-feature-state/expected.json b/test/integration/query-tests/featuresets/import-featureset-feature-state/expected.json
@@ -4,88 +4,33 @@
       "type": "LineString",
       "coordinates": [
         [
-          13.406662344932556,
-          52.49845542419487
+          13.415004014968872,
+          52.49808639114306
         ],
         [
-          13.406715989112854,
-          52.49853706825692
+          13.414907455444336,
+          52.49794269601955
         ],
         [
-          13.407037854194641,
-          52.499007335102704
+          13.41582477092743,
+          52.49771082335269
         ],
         [
-          13.40782642364502,
-          52.50002296369735
-        ],
-        [
-          13.409215807914734,
-          52.50175045812034
+          13.416152000427246,
+          52.49817456746362
         ]
       ]
     },
     "type": "Feature",
     "properties": {
-      "class": "main",
-      "type": "secondary"
+      "class": "path",
+      "type": "footway"
     },
-    "id": 4612696,
+    "id": 289597991,
     "target": {"featuresetId": "roads", "importId": "basemap"},
     "state": {
       "stateA": 1,
       "stateB": "feature_id as number"
     }
-  },
-  {
-    "geometry": {
-      "type": "LineString",
-      "coordinates": [
-        [
-          13.404956459999084,
-          52.50075446300568
-        ],
-        [
-          13.405857682228088,
-          52.500525870779285
-        ],
-        [
-          13.40782642364502,
-          52.50002296369735
-        ],
-        [
-          13.41029942035675,
-          52.49939268890719
-        ],
-        [
-          13.410347700119019,
-          52.49937962612168
-        ],
-        [
-          13.410476446151733,
-          52.49934370344147
-        ],
-        [
-          13.410674929618835,
-          52.499291452217875
-        ],
-        [
-          13.4122896194458,
-          52.498840782836766
-        ]
-      ]
-    },
-    "type": "Feature",
-    "properties": {
-      "class": "street",
-      "type": "residential"
-    },
-    "id": 4612752,
-    "target": {"featuresetId": "roads", "importId": "basemap"},
-    "state": {
-      "stateB": {
-        "stateKey": [4, 2]
-      }
-    }
   }
-]
+]
diff --git a/test/integration/query-tests/featuresets/import-featureset-feature-state/style.json b/test/integration/query-tests/featuresets/import-featureset-feature-state/style.json
@@ -3,12 +3,13 @@
   "metadata": {
     "test": {
       "height": 256,
+      "width": 500,
       "operations": [
-        ["setFeatureState", {"target": {"featuresetId": "roads", "importId": "basemap"}, "id": "4612696"}, {"stateA": 1}],
-        ["setFeatureState", {"target": {"featuresetId": "roads", "importId": "basemap"}, "id": 4612696}, {"stateB": "feature_id as number"}],
+        ["setFeatureState", {"target": {"featuresetId": "roads", "importId": "basemap"}, "id": "289597991"}, {"stateA": 1}],
+        ["setFeatureState", {"target": {"featuresetId": "roads", "importId": "basemap"}, "id": 289597991}, {"stateB": "feature_id as number"}],
         ["setFeatureState", {"target": {"featuresetId": "roads", "importId": "basemap"}, "id": "4612752"}, {"stateB": {"stateKey" : [4, 2]}}]
       ],
-      "queryGeometry": [10, 100],
+      "queryGeometry": [199, 184],
       "queryOptions": {
         "target": {"featuresetId": "roads", "importId": "basemap"}
       }
@@ -43,6 +44,16 @@
           }
         },
         "layers": [
+          {
+            "id": "line",
+            "type": "line",
+            "source": "mapbox",
+            "source-layer": "road",
+            "paint": {
+              "line-color": "red"
+            },
+            "interactive": true
+          },
           {
             "id": "road",
             "type": "circle",

diff --git a/test/integration/query-tests/featuresets/import-featureset-filter-false/style.json b/test/integration/query-tests/featuresets/import-featureset-filter-false/style.json
@@ -7,7 +7,7 @@
       "queryGeometry": [32, 32],
       "queryOptions": {
         "target": {"featuresetId": "circle", "importId": "basemap"},
-        "filter": ["==", "key", "nope"]
+        "filter": ["==", "key", "value"]
       }
     }
   },

diff --git a/test/integration/query-tests/featuresets/import-featureset-filter-true/style.json b/test/integration/query-tests/featuresets/import-featureset-filter-true/style.json
@@ -7,7 +7,7 @@
       "queryGeometry": [32, 32],
       "queryOptions": {
         "target": {"featuresetId": "circle", "importId": "basemap"},
-        "filter": ["==", "key", "value"]
+        "filter": ["==", "a-property-value", "value"]
       }
     }
   },

diff --git a/test/integration/query-tests/featuresets/import-featureset-multiple-layers/expected.json b/test/integration/query-tests/featuresets/import-featureset-multiple-layers/expected.json
@@ -1,18 +1,4 @@
 [
-  {
-    "type": "Feature",
-    "state": {},
-    "geometry": {
-      "type": "Point",
-      "coordinates": [0, 0]
-    },
-    "properties": {
-      "b-property-value": "value",
-      "b-constant-value": "constant-b"
-    },
-    "target": {"featuresetId": "featureset", "importId": "basemap"},
-    "namespace": "b"
-  },
   {
     "type": "Feature",
     "state": {},

diff --git a/test/integration/query-tests/featuresets/import-featureset-multiple-layers/style.json b/test/integration/query-tests/featuresets/import-featureset-multiple-layers/style.json
@@ -26,14 +26,6 @@
                   "a-property-value": ["get", "key"],
                   "a-constant-value": "constant-a"
                 }
-              },
-              {
-                "layer": "b",
-                "featureNamespace": "b",
-                "properties": {
-                  "b-property-value": ["get", "key"],
-                  "b-constant-value": "constant-b"
-                }
               }
             ]
           }
@@ -58,11 +50,6 @@
             "id": "a",
             "type": "circle",
             "source": "geojson"
-          },
-          {
-            "id": "b",
-            "type": "circle",
-            "source": "geojson"
           }
         ]
       }