Themes: add extra check so that in a mapping cannot override the , fix two such instances

assets/layers/climbing_gym/climbing_gym.json

@@ -135,7 +135,6 @@
             "de": "Kletterschuhe können hier ausgeliehen werden"
           },
           "addExtraTags": [
-            "service:climbing_shoes:rental:fee=",
             "service:climbing_shoes:rental:charge="
           ]
         },

assets/themes/campersite/campersite.json

@@ -231,12 +231,7 @@
               }
             },
             {
-              "if": {
+              "if": "fee=no",
-                "and": [
-                  "fee=no",
-                  "charge="
-                ]
-              },
               "then": {
                 "en": "Can be used for free",
                 "id": "Boleh digunakan tanpa bayaran",

src/Models/ThemeConfig/Conversion/Validation.ts

@@ -1,21 +1,22 @@
-import { DesugaringStep, Each, Fuse, On } from "./Conversion"
+import {DesugaringStep, Each, Fuse, On} from "./Conversion"
-import { LayerConfigJson } from "../Json/LayerConfigJson"
+import {LayerConfigJson} from "../Json/LayerConfigJson"
 import LayerConfig from "../LayerConfig"
-import { Utils } from "../../../Utils"
+import {Utils} from "../../../Utils"
 import Constants from "../../Constants"
-import { Translation } from "../../../UI/i18n/Translation"
+import {Translation} from "../../../UI/i18n/Translation"
-import { LayoutConfigJson } from "../Json/LayoutConfigJson"
+import {LayoutConfigJson} from "../Json/LayoutConfigJson"
 import LayoutConfig from "../LayoutConfig"
-import { TagRenderingConfigJson } from "../Json/TagRenderingConfigJson"
+import {TagRenderingConfigJson} from "../Json/TagRenderingConfigJson"
-import { TagUtils } from "../../../Logic/Tags/TagUtils"
+import {TagUtils} from "../../../Logic/Tags/TagUtils"
-import { ExtractImages } from "./FixImages"
+import {ExtractImages} from "./FixImages"
-import { And } from "../../../Logic/Tags/And"
+import {And} from "../../../Logic/Tags/And"
 import Translations from "../../../UI/i18n/Translations"
 import Svg from "../../../Svg"
 import FilterConfigJson from "../Json/FilterConfigJson"
 import DeleteConfig from "../DeleteConfig"
-import { QuestionableTagRenderingConfigJson } from "../Json/QuestionableTagRenderingConfigJson"
+import {QuestionableTagRenderingConfigJson} from "../Json/QuestionableTagRenderingConfigJson"
 import Validators from "../../../UI/InputElement/Validators"
+import TagRenderingConfig from "../TagRenderingConfig";
 
 class ValidateLanguageCompleteness extends DesugaringStep<any> {
     private readonly _languages: string[]
@@ -85,7 +86,7 @@ export class DoesImageExist extends DesugaringStep<string> {
         context: string
     ): { result: string; errors?: string[]; warnings?: string[]; information?: string[] } {
         if (this._ignore?.has(image)) {
-            return { result: image }
+            return {result: image}
         }
 
         const errors = []
@@ -93,22 +94,22 @@ export class DoesImageExist extends DesugaringStep<string> {
         const information = []
         if (image.indexOf("{") >= 0) {
             information.push("Ignoring image with { in the path: " + image)
-            return { result: image }
+            return {result: image}
         }
 
         if (image === "assets/SocialImage.png") {
-            return { result: image }
+            return {result: image}
         }
         if (image.match(/[a-z]*/)) {
             if (Svg.All[image + ".svg"] !== undefined) {
                 // This is a builtin img, e.g. 'checkmark' or 'crosshair'
-                return { result: image }
+                return {result: image}
             }
         }
 
         if (image.startsWith("<") && image.endsWith(">")) {
             // This is probably HTML, you're on your own here
-            return { result: image }
+            return {result: image}
         }
 
         if (!this._knownImagePaths.has(image)) {
@@ -312,7 +313,7 @@ class OverrideShadowingCheck extends DesugaringStep<LayoutConfigJson> {
     ): { result: LayoutConfigJson; errors?: string[]; warnings?: string[] } {
         const overrideAll = json.overrideAll
         if (overrideAll === undefined) {
-            return { result: json }
+            return {result: json}
         }
 
         const errors = []
@@ -339,7 +340,7 @@ class OverrideShadowingCheck extends DesugaringStep<LayoutConfigJson> {
             }
         }
 
-        return { result: json, errors }
+        return {result: json, errors}
     }
 }
 
@@ -383,6 +384,51 @@ export class PrevalidateTheme extends Fuse<LayoutConfigJson> {
     }
 }
 
+export class DetectConflictingAddExtraTags extends DesugaringStep<TagRenderingConfigJson> {
+    constructor() {
+        super("The `if`-part in a mapping might set some keys. Those key are not allowed to be set in the `addExtraTags`, as this might result in conflicting values", [], "DetectConflictingAddExtraTags");
+    }
+
+    convert(json: TagRenderingConfigJson, context: string): {
+        result: TagRenderingConfigJson;
+        errors?: string[];
+        warnings?: string[];
+        information?: string[]
+    } {
+
+        if (!(json.mappings?.length > 0)) {
+            return {result: json}
+        }
+
+        const tagRendering = new TagRenderingConfig(json)
+
+        const errors = []
+        for (let i = 0; i < tagRendering.mappings.length; i++) {
+            const mapping = tagRendering.mappings[i];
+            if (!mapping.addExtraTags) {
+                continue
+            }
+            const keysInMapping = new Set(mapping.if.usedKeys())
+
+            const keysInAddExtraTags = mapping.addExtraTags.map(t => t.key)
+
+            const duplicateKeys = keysInAddExtraTags.filter(k => keysInMapping.has(k))
+            if (duplicateKeys.length > 0) {
+                errors.push(
+                    "At " + context + ".mappings[" + i + "]: AddExtraTags overrides a key that is set in the `if`-clause of this mapping. Selecting this answer might thus first set one value (needed to match as answer) and then override it with a different value, resulting in an unsaveable question. The offending `addExtraTags` is " + duplicateKeys.join(", ")
+                )
+            }
+        }
+
+
+        return {
+            result: json,
+            errors
+        };
+    }
+}
+
+
 export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJson> {
     private readonly _calculatedTagNames: string[]
 
@@ -449,7 +495,7 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
         const errors = []
         const warnings = []
         if (json.mappings === undefined || json.mappings.length === 0) {
-            return { result: json }
+            return {result: json}
         }
         const defaultProperties = {}
         for (const calculatedTagName of this._calculatedTagNames) {
@@ -475,7 +521,7 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
             }
             const keyValues = parsedConditions[i].asChange(defaultProperties)
             const properties = {}
-            keyValues.forEach(({ k, v }) => {
+            keyValues.forEach(({k, v}) => {
                 properties[k] = v
             })
             for (let j = 0; j < i; j++) {
@@ -564,7 +610,7 @@ export class DetectMappingsWithImages extends DesugaringStep<TagRenderingConfigJ
         const warnings: string[] = []
         const information: string[] = []
         if (json.mappings === undefined || json.mappings.length === 0) {
-            return { result: json }
+            return {result: json}
         }
         const ignoreToken = "ignore-image-in-then"
         for (let i = 0; i < json.mappings.length; i++) {
@@ -669,6 +715,7 @@ export class ValidateTagRenderings extends Fuse<TagRenderingConfigJson> {
         super(
             "Various validation on tagRenderingConfigs",
             new DetectShadowedMappings(layerConfig),
+            new DetectConflictingAddExtraTags(),
             new DetectMappingsWithImages(doesImageExist),
             new MiscTagRenderingChecks(options)
         )
@@ -865,6 +912,13 @@ export class ValidateLayer extends DesugaringStep<LayerConfigJson> {
                 }
             }
 
+            if (json.filter) {
+                const r = new On("filter", new Each( new ValidateFilter())).convert(json, context)
+                warnings.push(...(r.warnings ?? []))
+                errors.push(...(r.errors ?? []))
+                information.push(...(r.information ?? []))
+            }
+
             if (json.tagRenderings !== undefined) {
                 const r = new On(
                     "tagRenderings",
@@ -903,7 +957,7 @@ export class ValidateLayer extends DesugaringStep<LayerConfigJson> {
                     const preset = json.presets[i]
                     const tags: { k: string; v: string }[] = new And(
                         preset.tags.map((t) => TagUtils.Tag(t))
-                    ).asChange({ id: "node/-1" })
+                    ).asChange({id: "node/-1"})
                     const properties = {}
                     for (const tag of tags) {
                         properties[tag.k] = tag.v
@@ -949,9 +1003,14 @@ export class ValidateFilter extends DesugaringStep<FilterConfigJson> {
         warnings?: string[]
         information?: string[]
     } {
+        if (typeof filter === "string") {
+            // Calling another filter, we skip
+            return {result: filter}
+        }
         const errors = []
         for (const option of filter.options) {
-            for (let i = 0; i < option.fields.length; i++) {
+
+            for (let i = 0; i < option.fields?.length ?? 0; i++) {
                 const field = option.fields[i]
                 const type = field.type ?? "string"
                 if (Validators.availableTypes.find((t) => t === type) === undefined) {
@@ -962,7 +1021,7 @@ export class ValidateFilter extends DesugaringStep<FilterConfigJson> {
                 }
             }
         }
-        return { result: filter, errors }
+        return {result: filter, errors}
     }
 }
 
@@ -991,7 +1050,7 @@ export class DetectDuplicateFilters extends DesugaringStep<{
         const warnings: string[] = []
         const information: string[] = []
 
-        const { layers, themes } = json
+        const {layers, themes} = json
         const perOsmTag = new Map<
             string,
             {
@@ -1027,7 +1086,7 @@ export class DetectDuplicateFilters extends DesugaringStep<{
                 return
             }
             let msg = "Possible duplicate filter: " + key
-            for (const { filter, layer, layout } of value) {
+            for (const {filter, layer, layout} of value) {
                 let id = ""
                 if (layout !== undefined) {
                     id = layout.id + ":"