Unable to filter a layer by whether a feature property is a substring of a literal string

[1ec5]

An expression that tests whether some subexpression is a substring of another string gets interpreted as a legacy filter that is incapable of evaluating to true for any feature. To reproduce, load this style and observe that nothing is visible:

colors.json

{
  "version": 8,
  "zoom": 0,
  "center": [0, 0],
  "sources": {
    "quadrants": {
      "type": "geojson",
      "data": {
        "type": "FeatureCollection",
        "features": [
          {
            "type": "Feature",
            "properties": {
              "name": "ABC",
              "color": "red"
            },
            "geometry": {
              "type": "Polygon",
              "coordinates": [[
                [-180, -90],
                [-180, 0],
                [0, 0],
                [0, -90],
                [-180, -90]
              ]]
            }
          },
          {
            "type": "Feature",
            "properties": {
              "name": "DEF",
              "color": "#00ff00"
            },
            "geometry": {
              "type": "Polygon",
              "coordinates": [[
                [-180, 90],
                [-180, 0],
                [0, 0],
                [0, 90],
                [-180, 90]
              ]]
            }
          },
          {
            "type": "Feature",
            "properties": {
              "name": "GHI",
              "color": "#0000ff"
            },
            "geometry": {
              "type": "Polygon",
              "coordinates": [[
                [180, -90],
                [180, 0],
                [0, 0],
                [0, -90],
                [180, -90]
              ]]
            }
          },
          {
            "type": "Feature",
            "properties": {
              "name": "GHI",
              "color": "#ffff00"
            },
            "geometry": {
              "type": "Polygon",
              "coordinates": [[
                [180, 90],
                [180, 0],
                [0, 0],
                [0, 90],
                [180, 90]
              ]]
            }
          }
        ]
      }
    }
  },
  "glyphs": "local://glyphs/{fontstack}/{range}.pbf",
  "layers": [
    {
      "id": "colors",
      "type": "fill",
      "source": "quadrants",
      "filter": ["in", ["get", "color"], "reddish"],
      "paint": {
        "fill-color": ["get", "color"]
      }
    }
  ]
}

Expected results: ["in", ["get", "color"], "reddish"] tests whether the color property is a substring of “reddish”.

Actual results: ["in", ["get", "color"], "reddish"] is interpreted as a legacy filter, which expects the first parameter to be a property name, not a subexpression. The following error appears in the console:

Error: layers[0].filter[1]: string expected, array found
Stack trace:
bn@https://api.mapbox.com/mapbox-gl-js/v1.8.1/mapbox-gl.js:29:128511
Oe@https://api.mapbox.com/mapbox-gl-js/v1.8.1/mapbox-gl.js:33:113929
Ze</i.prototype._load@https://api.mapbox.com/mapbox-gl-js/v1.8.1/mapbox-gl.js:33:116470
Ze</i.prototype.loadURL/this._request<@https://api.mapbox.com/mapbox-gl-js/v1.8.1/mapbox-gl.js:33:116088
bt/</r.onload@https://api.mapbox.com/mapbox-gl-js/v1.8.1/mapbox-gl.js:29:18281

isExpressionFilter() has logic specifically to avoid treating an in expression as a filter as in mapbox/mapbox-gl-native#16262. However, it only considers whether the second parameter is an array (and thus an expression):

mapbox-gl-js/src/style-spec/feature_filter/index.js

Lines 23 to 24 in 3f90780

	case 'in':
	return filter.length >= 3 && Array.isArray(filter[2]);

/cc @mapbox/gl-js @zmiao @chloekraw

[kkaefer]

This looks like it can't really be solved fully: ["in", "str", "string"] is both a valid legacy filter, and a valid expression. I implemented #9374, which errs on the side of a legacy filter if it's valid as both an expression and a legacy filter. This shouldn't be the case very often, since it only occurs when both the first and the second argument are a literal string. Using two strings with an in expression doesn't make much sense, since it's constant. If you still wanted to do it, you could convert either value to a literal expression (["literal", "value"]).

[1ec5]

Using two strings with an in expression doesn't make much sense, since it's constant. If you still wanted to do it, you could convert either value to a literal expression (["literal", "value"]).

I suppose a style could use a string-in-string expression to test whether the current library supports #4113, but I agree it is quite an edge case. It might be worth documenting the literal workaround for completeness, since the legacy filter syntax will only become more obscure over time.

[ryanhamley]

It might be worth documenting the literal workaround for completeness, since the legacy filter syntax will only become more obscure over time.

We did something similar in https://github.com/mapbox/mapbox-gl-js-docs/pull/212 after getting several issues in which people supplied an array literal as the second argument to in and were confused by the resulting error. I hadn't really considered if a string literal is the second argument, but it may make sense to document it. This seems like a docs issue rather than a bug.

[rreusser]

I've added a note to the docs regarding the workaround required for the very specific case which must be ambiguous for the sake of legacy compatibility. Since it's documented and since it's as it needs to be, I think there's not further action to be taken here.

[HansBrende]

@rreusser @ryanhamley @kkaefer fyi... I was very confused by the explanatory comment in the docs (implemented in #10833) because it said "the second and third argument" rather than the "first and second argument". In order to understand which arguments this was referring to, I had to search for the relevant github issue. My understanding was that the in keyword itself is not an argument but an operator. In addition to that, a note on what the semantics of the legacy behavior was would have been helpful (otherwise, have no clue what the semantics of ["in", string, string] are). A simple example of this edge case in the docs would be nice. To add to the confusion, all the examples linked to in the docs seem to be doing (what I only now realize to be) the legacy behavior (doing array spreading), which is not even mentioned in the docs to my knowledge, so it is extremely unclear what is going on.

[tmcw]

Seconding here: the docs for this are unhelpful. Here's what would be useful:

• An upgrade guide from the old syntax to the new. If you were using X, here's what Y is. Here's an example of a filter in the old and the new syntax. Kuan got a start on this here: http://kuanbutts.com/2019/02/18/mapbox-expressions/
• There's a lot of overlap between the old and new docs! == is an operator in both, but one, it seems, requires get to get that value whereas the other can let you just reference the key? This sort of observation shouldn't be something you have to dig up by reading the two pages intently, it should be explicitly stated to help people understand.
• The style spec validator messages are not very helpful. Lots of people get this "string expected" error and have to resort to Google. This is a common error. You could add something like "are you mixing syntaxes? (link to documentation page here)"

In short, it's clear that something's deprecated, and something's new, but not clear what.