fix: mixed legacy+expression filter handling in maplibre-style-spec (#1544)#1545
fix: mixed legacy+expression filter handling in maplibre-style-spec (#1544)#1545eddy-geek wants to merge 2 commits into
maplibre-style-spec (#1544)#1545Conversation
…maplibre#1544) Filters like this one were misclassified as legacy filters and then sent through legacy conversion, silently mangling expression-only operators like 'case' and 'global-state', and causing filters to fail without any error. ```ts ['all', ['==', '$type', 'Point'], ['case', ...]] ``` The fix aligns runtime behavior with the deprecation docs: legacy filter syntax and expression syntax cannot be mixed in one filter definition. Instead of attempting broad recursive normalization, mixed filters now fail explicitly with clearer diagnostics and rewrite suggestions where the legacy subfilter has an obvious expression equivalent. - Updated [isExpressionFilter()] so mixed `all` / `any` / `none` trees are classified as expressions when they contain expression children, preventing accidental fallback into legacy conversion. - Preserved pure legacy behavior, including legacy `none`, by keeping legacy-only filters on the [convertFilter()] path. - Added shared mixed-filter diagnostics in [src/feature_filter/index.ts]: - detect legacy subfilters inside expression contexts - produce explicit error messages - suggest expression rewrites for cases like: - `["==", "$type", "Point"]` - `["in", "$type", "Point", "LineString"]` - `["==", "class", "primary"]` - Reused the same mixed-filter detection/message path in [src/validate/validate_filter.ts] so validation and runtime report consistent errors. - Expanded integration coverage in [filters.input.json] for mixed cases, including: - plain property filters mixed with expressions - `$type` equality mixed with expressions - mixed `none` - `$type in` - nested mixed syntax inside `case` conditions - Rebalanced [feature_filter.test.ts] so it keeps runtime-specific coverage only: - the original classification regression shape - mixed `none` - unsupported legacy special-key operator errors We considered broader recursive reuse of [validateFilter()] / [validateNonExpressionFilter()] or permissive recursive conversion of mixed trees, but dropped that approach. Mixed syntax is context-sensitive: only boolean/filter positions are sensible places to detect or reason about legacy-looking subtrees. General recursive conversion would risk implying support in positions where conversion is unsafe or misleading, such as non-boolean expression arguments or result branches. So this change prefers: - broad coverage in validation fixtures - narrow, explicit runtime behavior - clear failures instead of magical fixups Mixed legacy + expression filters now fail cleanly with actionable messages, for example: - `Mixing deprecated filter syntax with expression syntax is not supported. Replace ["==","$type","Point"] with ["==",["geometry-type"],"Point"].` Unsupported legacy special-key combinations still fail explicitly, e.g.: - `"$type" cannot be use with operator ">"`
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
==========================================
- Coverage 93.79% 93.67% -0.12%
==========================================
Files 112 112
Lines 4670 4773 +103
Branches 1572 1621 +49
==========================================
+ Hits 4380 4471 +91
- Misses 290 302 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for taking the time to open this PR! |
|
fair point. in short: the result was always semantically wrong and surprising vs what you would expect - e.g. the modern branch is silently ignored. according to AI this would happen: // mixed example
["all", ["==", "$type", "Point"], ["case", [">=", ["zoom"], 10], true, false]]
// converted silently to
["all", ["filter-type-==", "Point"], true]I would really think that even if people ended up keeping such broken filters, the new clear failure diagnostics will improve their life. the change to make is also clearly spelled out. I'm aware 'bug-for-bug compatibility' does exist in software but I don't believe it's the best choice here :) more AI-generated details# What the mangling looked likeAssuming the mixed subtree was misclassified as legacy and sent through [convertFilter()], there were two main failure modes:
So yes: sometimes the result was still valid, but wrong. Other times it became an invalid internal filter expression and failed later. Case 1Input: ["all", ["==", "$type", "Point"], ["case", [">=", ["zoom"], 10], true, false]]If this goes through legacy conversion:
So the whole thing becomes: ["all", ["filter-type-==", "Point"], true]Which is effectively just: ["filter-type-==", "Point"]Meaning
So in this case: yes, the mangled result was valid, just weaker than intended. Case 2Input: ["case", ["all", [">=", ["zoom"], 10], ["==", "$type", "Point"]], true, false]If the inner
So the inner condition becomes: ["all", ["filter->=", ["zoom"], 10], ["filter-type-==", "Point"]]And the outer ["case", ["all", ["filter->=", ["zoom"], 10], ["filter-type-==", "Point"]], true, false]MeaningThis one is different:
So this is not a meaningful converted filter. It is not a sensible runtime condition anymore. Likely outcomeThis would typically fail expression compilation/validation later, because the internal operator gets the wrong argument shape. So in this case the mangled result was not usefully valid. Why
|
|
Thanks. |
|
Yes I will add a couple tests. |
|
I've added this PR to the v6 breaking changes tracking issue, I hope to start the process of releasing a breaking change version in the next few weeks. |
|
@eddy-geek version 6 has started the pre-release process, so we can consider merging this now and releasing a new version. |
| export function validateFilter(options) { | ||
| if (isExpressionFilter(deepUnbundle(options.value))) { | ||
| const value = deepUnbundle(options.value); | ||
| if (isExpressionFilter(value)) { |
There was a problem hiding this comment.
Can this if be negated and reduce indentation in this method?
There was a problem hiding this comment.
Also add types to options object.
| const value = deepUnbundle(options.value); | ||
| if (isExpressionFilter(value)) { | ||
| const mixedLegacyDiagnostic = findMixedLegacyFilter(value); | ||
| if (mixedLegacyDiagnostic) { |
| isExpressionFilter | ||
| } from '../feature_filter'; | ||
|
|
||
| function getValueAtPath(value, path) { |
| return null; | ||
| } | ||
|
|
||
| const checkChild = (index: number) => { |
There was a problem hiding this comment.
Move this out to a separate method.
| return null; | ||
| } | ||
|
|
||
| export function validateNoMixedExpressionFilter(filter: unknown) { |
There was a problem hiding this comment.
Isn't filter should be of type FilterSpecification or something like that?
|
|
||
| if (!isExpressionFilter(filter)) { | ||
| if (Array.isArray(filter) && filter[0] === 'none' && isExpressionFilter(filter)) { | ||
| validateNoMixedExpressionFilter(filter); |
There was a problem hiding this comment.
I would expect the validate to be before the initail if and maybe move the logic of Array.isArray(filter) && filter[0] === 'none' inside this method.
|
What's the performance impact of this change? I would assume filtering is probably a hot path in maplibre... |
Filter VALIDATION is unlikely to be in the hot path. Expression eval is also unlikely to be in our hot path according to the evals that I have done, unless you are doing something severly cursed. |
|
Fair point. |
4 participants
Filters like this one were misclassified as legacy filters and then sent through legacy conversion, silently mangling expression-only operators like 'case' and 'global-state', and causing filters to fail without any error.
The fix aligns runtime behavior with the deprecation docs: legacy filter syntax and expression syntax cannot be mixed in one filter definition. Instead of attempting broad recursive normalization, mixed filters now fail explicitly with clearer diagnostics and rewrite suggestions where the legacy subfilter has an obvious expression equivalent.
all/any/nonetrees are classified as expressions when they contain expression children, preventing accidental fallback into legacy conversion.none, by keeping legacy-only filters on the [convertFilter()] path.["==", "$type", "Point"]["in", "$type", "Point", "LineString"]["==", "class", "primary"]$typeequality mixed with expressionsnone$type incaseconditionsnoneWe considered broader recursive reuse of [validateFilter()] / [validateNonExpressionFilter()] or permissive recursive conversion of mixed trees, but dropped that approach.
Mixed syntax is context-sensitive: only boolean/filter positions are sensible places to detect or reason about legacy-looking subtrees. General recursive conversion would risk implying support in positions where conversion is unsafe or misleading, such as non-boolean expression arguments or result branches.
So this change prefers:
Mixed legacy + expression filters now fail cleanly with actionable messages, for example:
Mixing deprecated filter syntax with expression syntax is not supported. Replace ["==","$type","Point"] with ["==",["geometry-type"],"Point"].Unsupported legacy special-key combinations still fail explicitly, e.g.:
"$type" cannot be use with operator ">"Launch Checklist
CHANGELOG.mdunder the## mainsection.