From 7001216ab70eb8b5d0a7217986f5a56cf660fd9e Mon Sep 17 00:00:00 2001 From: pietervdvn Date: Fri, 4 Feb 2022 00:44:09 +0100 Subject: [PATCH] Add more consistency checks, slight refactoring of theme conversions --- Logic/Tags/TagUtils.ts | 4 + Models/ThemeConfig/Conversion/Conversion.ts | 21 +- .../Conversion/CreateNoteImportLayer.ts | 3 + .../Conversion/LegacyJsonConvert.ts | 233 ------------ Models/ThemeConfig/Conversion/PrepareTheme.ts | 20 +- Models/ThemeConfig/Conversion/Validation.ts | 334 ++++++++++++++++++ Utils.ts | 31 +- test/LegacyThemeLoader.spec.ts | 87 +++-- 8 files changed, 457 insertions(+), 276 deletions(-) diff --git a/Logic/Tags/TagUtils.ts b/Logic/Tags/TagUtils.ts index e5e48f5b02..927205fc16 100644 --- a/Logic/Tags/TagUtils.ts +++ b/Logic/Tags/TagUtils.ts @@ -285,6 +285,10 @@ export class TagUtils { throw `Error while parsing tag '${tag}' in ${context}: no key part and value part were found` } + + if(json.and !== undefined && json.or !== undefined){ + throw `Error while parsing a TagConfig: got an object where both 'and' and 'or' are defined`} + if (json.and !== undefined) { return new And(json.and.map(t => TagUtils.Tag(t, context))); } diff --git a/Models/ThemeConfig/Conversion/Conversion.ts b/Models/ThemeConfig/Conversion/Conversion.ts index 3eee818100..589af96768 100644 --- a/Models/ThemeConfig/Conversion/Conversion.ts +++ b/Models/ThemeConfig/Conversion/Conversion.ts @@ -18,9 +18,9 @@ export abstract class Conversion { this.name = name ?? this.constructor.name } - public static strict(fixed: { errors: string[], warnings: string[], result?: T }): T { - if (fixed?.errors?.length > 0) { - throw fixed.errors.join("\n"); + public static strict(fixed: { errors?: string[], warnings?: string[], result?: T }): T { + if (fixed?.errors !== undefined && fixed?.errors?.length > 0) { + throw fixed.errors.join("\n\n"); } fixed.warnings?.forEach(w => console.warn(w)) return fixed.result; @@ -31,9 +31,12 @@ export abstract class Conversion { return DesugaringStep.strict(fixed) } - abstract convert(state: DesugaringContext, json: TIn, context: string): { result: TOut, errors: string[], warnings: string[] } + abstract convert(state: DesugaringContext, json: TIn, context: string): { result: TOut, errors?: string[], warnings?: string[] } public convertAll(state: DesugaringContext, jsons: TIn[], context: string): { result: TOut[], errors: string[], warnings: string[] } { + if(jsons === undefined){ + throw "convertAll received undefined - don't do this (at "+context+")" + } const result = [] const errors = [] const warnings = [] @@ -41,8 +44,8 @@ export abstract class Conversion { const json = jsons[i]; const r = this.convert(state, json, context + "[" + i + "]") result.push(r.result) - errors.push(...r.errors) - warnings.push(...r.warnings) + errors.push(...r.errors ?? []) + warnings.push(...r.warnings ?? []) } return { result, @@ -66,7 +69,7 @@ export class OnEvery extends DesugaringStep { this.key = key; } - convert(state: DesugaringContext, json: T, context: string): { result: T; errors: string[]; warnings: string[] } { + convert(state: DesugaringContext, json: T, context: string): { result: T; errors?: string[]; warnings?: string[] } { json = {...json} const step = this.step const key = this.key; @@ -132,8 +135,8 @@ export class Fuse extends DesugaringStep { for (let i = 0; i < this.steps.length; i++) { const step = this.steps[i]; let r = step.convert(state, json, "While running step " +step.name + ": " + context) - errors.push(...r.errors) - warnings.push(...r.warnings) + errors.push(...r.errors ?? []) + warnings.push(...r.warnings ?? []) json = r.result if (errors.length > 0) { break; diff --git a/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts b/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts index c655802857..5f244d2787 100644 --- a/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts +++ b/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts @@ -45,6 +45,9 @@ export default class CreateNoteImportLayer extends Conversion r !== null && r["location"] !== undefined); const firstRender = (pointRenderings [0]) + if(firstRender === undefined){ + throw `Layer ${layerJson.id} does not have a pointRendering: `+context + } const icon = firstRender.icon const iconBadges = [] const title = layer.presets[0].title diff --git a/Models/ThemeConfig/Conversion/LegacyJsonConvert.ts b/Models/ThemeConfig/Conversion/LegacyJsonConvert.ts index 3c101939ee..dc9201dae4 100644 --- a/Models/ThemeConfig/Conversion/LegacyJsonConvert.ts +++ b/Models/ThemeConfig/Conversion/LegacyJsonConvert.ts @@ -1,11 +1,7 @@ import {LayoutConfigJson} from "../Json/LayoutConfigJson"; -import LayerConfig from "../LayerConfig"; -import {Translation} from "../../../UI/i18n/Translation"; -import LayoutConfig from "../LayoutConfig"; import {Utils} from "../../../Utils"; import LineRenderingConfigJson from "../Json/LineRenderingConfigJson"; import {LayerConfigJson} from "../Json/LayerConfigJson"; -import Constants from "../../Constants"; import {DesugaringContext, DesugaringStep, Fuse, OnEvery} from "./Conversion"; export class UpdateLegacyLayer extends DesugaringStep { @@ -161,232 +157,3 @@ export class FixLegacyTheme extends Fuse { ); } } - -export class ValidateLayer extends DesugaringStep { - /** - * The paths where this layer is originally saved. Triggers some extra checks - * @private - */ - private readonly _path?: string; - private readonly knownImagePaths?: Set; - private readonly _isBuiltin: boolean; - - constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { - super("Doesn't change anything, but emits warnings and errors", []); - this.knownImagePaths = knownImagePaths; - this._path = path; - this._isBuiltin = isBuiltin; - } - - convert(state: DesugaringContext, json: LayerConfigJson, context: string): { result: LayerConfigJson; errors: string[]; warnings: string[] } { - const errors = [] - const warnings = [] - - if (typeof json === "string") { - errors.push(context + ": This layer hasn't been expanded: " + json) - return { - result: null, - warnings: [], - errors - } - } - - if (json["builtin"] !== undefined) { - errors.push(context + ": This layer hasn't been expanded: " + json) - return { - result: null, - warnings: [], - errors - } - } - - try { - { - // Some checks for legacy elements - - if (json["overpassTags"] !== undefined) { - errors.push("Layer " + json.id + "still uses the old 'overpassTags'-format. Please use \"source\": {\"osmTags\": }' instead of \"overpassTags\": (note: this isn't your fault, the custom theme generator still spits out the old format)") - } - const forbiddenTopLevel = ["icon", "wayHandling", "roamingRenderings", "roamingRendering", "label", "width", "color", "colour", "iconOverlays"] - for (const forbiddenKey of forbiddenTopLevel) { - if (json[forbiddenKey] !== undefined) - errors.push(context + ": layer " + json.id + " still has a forbidden key " + forbiddenKey) - } - if (json["hideUnderlayingFeaturesMinPercentage"] !== undefined) { - errors.push(context + ": layer " + json.id + " contains an old 'hideUnderlayingFeaturesMinPercentage'") - } - } - { - const layer = new LayerConfig(json, "test", true) - const images = Array.from(layer.ExtractImages()) - const remoteImages = images.filter(img => img.indexOf("http") == 0) - for (const remoteImage of remoteImages) { - errors.push("Found a remote image: " + remoteImage + " in layer " + layer.id + ", please download it. You can use the fixTheme script to automate this") - } - for (const image of images) { - if (image.indexOf("{") >= 0) { - warnings.push("Ignoring image with { in the path: ", image) - continue - } - - if (this.knownImagePaths !== undefined && !this.knownImagePaths.has(image)) { - const ctx = context === undefined ? "" : ` in a layer defined in the theme ${context}` - errors.push(`Image with path ${image} not found or not attributed; it is used in ${layer.id}${ctx}`) - } - } - - } - { - // CHeck location - const expected: string = `assets/layers/${json.id}/${json.id}.json` - if (this._path != undefined && this._path.indexOf(expected) < 0) { - errors.push("Layer is in an incorrect place. The path is " + this._path + ", but expected " + expected) - } - } - - - if (this._isBuiltin) { - if (json.tagRenderings?.some(tr => tr["id"] === "")) { - const emptyIndexes: number[] = [] - for (let i = 0; i < json.tagRenderings.length; i++) { - const tagRendering = json.tagRenderings[i]; - if (tagRendering["id"] === "") { - emptyIndexes.push(i) - } - } - errors.push(`Some tagrendering-ids are empty or have an emtpy string; this is not allowed (at ${context}.tagRenderings.[${emptyIndexes.join(",")}])`) - } - - const duplicateIds = Utils.Dupiclates((json.tagRenderings ?? [])?.map(f => f["id"]).filter(id => id !== "questions")) - if (duplicateIds.length > 0 && !Utils.runningFromConsole) { - errors.push(`Some tagRenderings have a duplicate id: ${duplicateIds} (at ${context}.tagRenderings)`) - } - - - if (json.description === undefined) { - - if (Constants.priviliged_layers.indexOf(json.id) >= 0) { - errors.push( - context + ": A priviliged layer must have a description" - ) - } else { - warnings.push( - context + ": A builtin layer should have a description" - ) - } - } - } - } catch (e) { - errors.push(e) - } - return { - result: undefined, - errors, - warnings - }; - } -} - -class ValidateLanguageCompleteness extends DesugaringStep { - private readonly _languages: string[]; - - constructor(...languages: string[]) { - super("Checks that the given object is fully translated in the specified languages", []); - this._languages = languages; - } - - convert(state: DesugaringContext, obj: any, context: string): { result: LayerConfig; errors: string[]; warnings: string[] } { - const errors = [] - const translations = Translation.ExtractAllTranslationsFrom( - obj - ) - for (const neededLanguage of this._languages) { - translations - .filter(t => t.tr.translations[neededLanguage] === undefined && t.tr.translations["*"] === undefined) - .forEach(missing => { - errors.push(context + "A theme should be translation-complete for " + neededLanguage + ", but it lacks a translation for " + missing.context + ".\n\tThe english translation is " + missing.tr.textFor('en')) - }) - } - - return { - result: obj, - warnings: [], errors - }; - } -} - -class ValidateTheme extends DesugaringStep { - /** - * The paths where this layer is originally saved. Triggers some extra checks - * @private - */ - private readonly _path?: string; - private readonly knownImagePaths: Set; - private readonly _isBuiltin: boolean; - - constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { - super("Doesn't change anything, but emits warnings and errors", []); - this.knownImagePaths = knownImagePaths; - this._path = path; - this._isBuiltin = isBuiltin; - } - - convert(state: DesugaringContext, json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors: string[]; warnings: string[] } { - const errors = [] - const warnings = [] - { - // Legacy format checks - if (this._isBuiltin) { - if (json["units"] !== undefined) { - errors.push("The theme " + json.id + " has units defined - these should be defined on the layer instead. (Hint: use overrideAll: { '+units': ... }) ") - } - if (json["roamingRenderings"] !== undefined) { - errors.push("Theme " + json.id + " contains an old 'roamingRenderings'. Use an 'overrideAll' instead") - } - } - } - - try { - const theme = new LayoutConfig(json, true, "test") - if (theme.id !== theme.id.toLowerCase()) { - errors.push("Theme ids should be in lowercase, but it is " + theme.id) - } - - const filename = this._path.substring(this._path.lastIndexOf("/") + 1, this._path.length - 5) - if (theme.id !== filename) { - errors.push("Theme ids should be the same as the name.json, but we got id: " + theme.id + " and filename " + filename + " (" + this._path + ")") - } - if (!this.knownImagePaths.has(theme.icon)) { - errors.push("The theme image " + theme.icon + " is not attributed or not saved locally") - } - const dups = Utils.Dupiclates(json.layers.map(layer => layer["id"])) - if (dups.length > 0) { - errors.push(`The theme ${json.id} defines multiple layers with id ${dups.join(", ")}`) - } - if (json["mustHaveLanguage"] !== undefined) { - const checked = new ValidateLanguageCompleteness(...json["mustHaveLanguage"]) - .convert(state, theme, theme.id) - errors.push(...checked.errors) - warnings.push(...checked.warnings) - } - - } catch (e) { - errors.push(e) - } - - return { - result: json, - errors, - warnings - }; - } -} - -export class ValidateThemeAndLayers extends Fuse { - constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { - super("Validates a theme and the contained layers", - new ValidateTheme(knownImagePaths, path, isBuiltin), - new OnEvery("layers", new ValidateLayer(knownImagePaths, undefined, false)) - ); - } -} \ No newline at end of file diff --git a/Models/ThemeConfig/Conversion/PrepareTheme.ts b/Models/ThemeConfig/Conversion/PrepareTheme.ts index a5920ebe17..afd90daf5c 100644 --- a/Models/ThemeConfig/Conversion/PrepareTheme.ts +++ b/Models/ThemeConfig/Conversion/PrepareTheme.ts @@ -10,21 +10,35 @@ import LayerConfig from "../LayerConfig"; import {TagRenderingConfigJson} from "../Json/TagRenderingConfigJson"; import {SubstitutedTranslation} from "../../../UI/SubstitutedTranslation"; import DependencyCalculator from "../DependencyCalculator"; +import {ValidateThemeAndLayers} from "./Validation"; class SubstituteLayer extends Conversion<(string | LayerConfigJson), LayerConfigJson[]> { constructor() { super("Converts the identifier of a builtin layer into the actual layer, or converts a 'builtin' syntax with override in the fully expanded form", []); } - + convert(state: DesugaringContext, json: string | LayerConfigJson, context: string): { result: LayerConfigJson[]; errors: string[]; warnings: string[] } { const errors = [] const warnings = [] + + function reportNotFound(name: string){ + const knownLayers = Array.from(state.sharedLayers.keys()) + const withDistance = knownLayers.map(lname => [lname, Utils.levenshteinDistance(name, lname)]) + withDistance.sort((a, b) => a[1] - b[1]) + const ids = withDistance.map(n => n[0]) + // Known builtin layers are "+.join(",")+"\n For more information, see " + errors.push(`${context}: The layer with name ${name} was not found as a builtin layer. Perhaps you meant ${ids[0]}, ${ids[1]} or ${ids[2]}? + For an overview of all available layers, refer to https://github.com/pietervdvn/MapComplete/blob/develop/Docs/BuiltinLayers.md`) + } + + if (typeof json === "string") { const found = state.sharedLayers.get(json) if (found === undefined) { + reportNotFound(json) return { result: null, - errors: [context + ": The layer with name " + json + " was not found as a builtin layer"], + errors, warnings } } @@ -43,7 +57,7 @@ class SubstituteLayer extends Conversion<(string | LayerConfigJson), LayerConfig for (const name of names) { const found = Utils.Clone(state.sharedLayers.get(name)) if (found === undefined) { - errors.push(context + ": The layer with name " + JSON.stringify(json) + " was not found as a builtin layer. Known builtin layers are "+Array.from(state.sharedLayers.keys()).join(",")+"\n For more information, see https://github.com/pietervdvn/MapComplete/blob/develop/Docs/BuiltinLayers.md" ) + reportNotFound(name) continue } if (json["override"]["tagRenderings"] !== undefined && (found["tagRenderings"] ?? []).length > 0) { diff --git a/Models/ThemeConfig/Conversion/Validation.ts b/Models/ThemeConfig/Conversion/Validation.ts index e69de29bb2..13385a5366 100644 --- a/Models/ThemeConfig/Conversion/Validation.ts +++ b/Models/ThemeConfig/Conversion/Validation.ts @@ -0,0 +1,334 @@ +import {DesugaringContext, DesugaringStep, Fuse, OnEvery} from "./Conversion"; +import {LayerConfigJson} from "../Json/LayerConfigJson"; +import LayerConfig from "../LayerConfig"; +import {Utils} from "../../../Utils"; +import Constants from "../../Constants"; +import {Translation} from "../../../UI/i18n/Translation"; +import {LayoutConfigJson} from "../Json/LayoutConfigJson"; +import LayoutConfig from "../LayoutConfig"; +import {TagRenderingConfigJson} from "../Json/TagRenderingConfigJson"; +import {TagUtils} from "../../../Logic/Tags/TagUtils"; +import {parseString} from "xml2js"; + + +class ValidateLanguageCompleteness extends DesugaringStep { + private readonly _languages: string[]; + + constructor(...languages: string[]) { + super("Checks that the given object is fully translated in the specified languages", []); + this._languages = languages; + } + + convert(state: DesugaringContext, obj: any, context: string): { result: LayerConfig; errors: string[]; warnings: string[] } { + const errors = [] + const translations = Translation.ExtractAllTranslationsFrom( + obj + ) + for (const neededLanguage of this._languages) { + translations + .filter(t => t.tr.translations[neededLanguage] === undefined && t.tr.translations["*"] === undefined) + .forEach(missing => { + errors.push(context + "A theme should be translation-complete for " + neededLanguage + ", but it lacks a translation for " + missing.context + ".\n\tThe english translation is " + missing.tr.textFor('en')) + }) + } + + return { + result: obj, + warnings: [], errors + }; + } +} + +class ValidateTheme extends DesugaringStep { + /** + * The paths where this layer is originally saved. Triggers some extra checks + * @private + */ + private readonly _path?: string; + private readonly knownImagePaths: Set; + private readonly _isBuiltin: boolean; + + constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { + super("Doesn't change anything, but emits warnings and errors", []); + this.knownImagePaths = knownImagePaths; + this._path = path; + this._isBuiltin = isBuiltin; + } + + convert(state: DesugaringContext, json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors: string[]; warnings: string[] } { + const errors = [] + const warnings = [] + { + // Legacy format checks + if (this._isBuiltin) { + if (json["units"] !== undefined) { + errors.push("The theme " + json.id + " has units defined - these should be defined on the layer instead. (Hint: use overrideAll: { '+units': ... }) ") + } + if (json["roamingRenderings"] !== undefined) { + errors.push("Theme " + json.id + " contains an old 'roamingRenderings'. Use an 'overrideAll' instead") + } + } + } + + try { + const theme = new LayoutConfig(json, true, "test") + if (theme.id !== theme.id.toLowerCase()) { + errors.push("Theme ids should be in lowercase, but it is " + theme.id) + } + + const filename = this._path.substring(this._path.lastIndexOf("/") + 1, this._path.length - 5) + if (theme.id !== filename) { + errors.push("Theme ids should be the same as the name.json, but we got id: " + theme.id + " and filename " + filename + " (" + this._path + ")") + } + if (!this.knownImagePaths.has(theme.icon)) { + errors.push("The theme image " + theme.icon + " is not attributed or not saved locally") + } + const dups = Utils.Dupiclates(json.layers.map(layer => layer["id"])) + if (dups.length > 0) { + errors.push(`The theme ${json.id} defines multiple layers with id ${dups.join(", ")}`) + } + if (json["mustHaveLanguage"] !== undefined) { + const checked = new ValidateLanguageCompleteness(...json["mustHaveLanguage"]) + .convert(state, theme, theme.id) + errors.push(...checked.errors) + warnings.push(...checked.warnings) + } + + } catch (e) { + errors.push(e) + } + + return { + result: json, + errors, + warnings + }; + } +} + +export class ValidateThemeAndLayers extends Fuse { + constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { + super("Validates a theme and the contained layers", + new ValidateTheme(knownImagePaths, path, isBuiltin), + new OnEvery("layers", new ValidateLayer(knownImagePaths, undefined, false)) + ); + } +} + + +class OverrideShadowingCheck extends DesugaringStep{ + + constructor() { + super("Checks that an 'overrideAll' does not override a single override"); + } + + convert(state: DesugaringContext, json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors?: string[]; warnings?: string[] } { + + const overrideAll = json.overrideAll; + if(overrideAll === undefined){ + return {result: json} + } + + const errors = [] + const withOverride = json.layers.filter(l => l["override"] !== undefined) + + for (const layer of withOverride) { + for (const key in overrideAll) { + if(layer["override"][key] !== undefined || layer["override"]["="+key] !== undefined){ + const w = "The override of layer "+JSON.stringify(layer["builtin"])+" has a shadowed property: "+key+" is overriden by overrideAll of the theme"; + errors.push(w) + } + } + } + + return {result: json, errors} + } + + + +} + +export class PrevalidateTheme extends Fuse{ + + constructor() { + super("Various consistency checks on the raw JSON", + new OverrideShadowingCheck() + ); + + } + + +} + +export class DetectShadowedMappings extends DesugaringStep{ + constructor() { + super("Checks that the mappings don't shadow each other"); + } + + convert(state: DesugaringContext, json: TagRenderingConfigJson, context: string): { result: TagRenderingConfigJson; errors?: string[]; warnings?: string[] } { + const errors = [] + if(json.mappings === undefined || json.mappings.length === 0){ + return {result: json} + } + const parsedConditions = json.mappings.map(m => TagUtils.Tag(m.if)) + for (let i = 0; i < json.mappings.length; i++){ + if(!parsedConditions[i].isUsableAsAnswer()){ + continue + } + const keyValues = parsedConditions[i].asChange({}); + const properties = [] + keyValues.forEach(({k, v}) => { + properties[k] = v + }) + for (let j = 0; j < i; j++){ + const doesMatch = parsedConditions[j].matchesProperties(properties) + if(doesMatch){ + // The current mapping is shadowed! + errors.push(`Mapping ${i} is shadowed by mapping ${j} and will thus never be shown: + The mapping ${parsedConditions[i].asHumanString(false,false, {})} is fully matched by a previous mapping, which matches: + ${parsedConditions[j].asHumanString(false, false, {})}. + + Move the mapping up to fix this problem +`) + } + } + + } + + return { + errors, + result: json + }; + } +} + +export class ValidateLayer extends DesugaringStep { + /** + * The paths where this layer is originally saved. Triggers some extra checks + * @private + */ + private readonly _path?: string; + private readonly knownImagePaths?: Set; + private readonly _isBuiltin: boolean; + + constructor(knownImagePaths: Set, path: string, isBuiltin: boolean) { + super("Doesn't change anything, but emits warnings and errors", []); + this.knownImagePaths = knownImagePaths; + this._path = path; + this._isBuiltin = isBuiltin; + } + + convert(state: DesugaringContext, json: LayerConfigJson, context: string): { result: LayerConfigJson; errors: string[]; warnings?: string[] } { + const errors = [] + const warnings = [] + + if (typeof json === "string") { + errors.push(context + ": This layer hasn't been expanded: " + json) + return { + result: null, + errors + } + } + + if (json["builtin"] !== undefined) { + errors.push(context + ": This layer hasn't been expanded: " + json) + return { + result: null, + errors + } + } + + try { + { + // Some checks for legacy elements + + if (json["overpassTags"] !== undefined) { + errors.push("Layer " + json.id + "still uses the old 'overpassTags'-format. Please use \"source\": {\"osmTags\": }' instead of \"overpassTags\": (note: this isn't your fault, the custom theme generator still spits out the old format)") + } + const forbiddenTopLevel = ["icon", "wayHandling", "roamingRenderings", "roamingRendering", "label", "width", "color", "colour", "iconOverlays"] + for (const forbiddenKey of forbiddenTopLevel) { + if (json[forbiddenKey] !== undefined) + errors.push(context + ": layer " + json.id + " still has a forbidden key " + forbiddenKey) + } + if (json["hideUnderlayingFeaturesMinPercentage"] !== undefined) { + errors.push(context + ": layer " + json.id + " contains an old 'hideUnderlayingFeaturesMinPercentage'") + } + } + { + // Check for remote images + const layer = new LayerConfig(json, "test", true) + const images = Array.from(layer.ExtractImages()) + const remoteImages = images.filter(img => img.indexOf("http") == 0) + for (const remoteImage of remoteImages) { + errors.push("Found a remote image: " + remoteImage + " in layer " + layer.id + ", please download it. You can use the fixTheme script to automate this") + } + for (const image of images) { + if (image.indexOf("{") >= 0) { + warnings.push("Ignoring image with { in the path: ", image) + continue + } + + if (this.knownImagePaths !== undefined && !this.knownImagePaths.has(image)) { + const ctx = context === undefined ? "" : ` in a layer defined in the theme ${context}` + errors.push(`Image with path ${image} not found or not attributed; it is used in ${layer.id}${ctx}`) + } + } + + } + { + // CHeck location of layer file + const expected: string = `assets/layers/${json.id}/${json.id}.json` + if (this._path != undefined && this._path.indexOf(expected) < 0) { + errors.push("Layer is in an incorrect place. The path is " + this._path + ", but expected " + expected) + } + } + if (this._isBuiltin) { + // Check for correct IDs + if (json.tagRenderings?.some(tr => tr["id"] === "")) { + const emptyIndexes: number[] = [] + for (let i = 0; i < json.tagRenderings.length; i++) { + const tagRendering = json.tagRenderings[i]; + if (tagRendering["id"] === "") { + emptyIndexes.push(i) + } + } + errors.push(`Some tagrendering-ids are empty or have an emtpy string; this is not allowed (at ${context}.tagRenderings.[${emptyIndexes.join(",")}])`) + } + + const duplicateIds = Utils.Dupiclates((json.tagRenderings ?? [])?.map(f => f["id"]).filter(id => id !== "questions")) + if (duplicateIds.length > 0 && !Utils.runningFromConsole) { + errors.push(`Some tagRenderings have a duplicate id: ${duplicateIds} (at ${context}.tagRenderings)`) + } + + + if (json.description === undefined) { + + if (Constants.priviliged_layers.indexOf(json.id) >= 0) { + errors.push( + context + ": A priviliged layer must have a description" + ) + } else { + warnings.push( + context + ": A builtin layer should have a description" + ) + } + } + } + if(json.tagRenderings !== undefined){ + new DetectShadowedMappings().convertAll(state, json.tagRenderings, context+".tagRenderings") + } + + } catch (e) { + errors.push(e) + } + + + + + return { + result: json, + errors, + warnings + }; + } +} \ No newline at end of file diff --git a/Utils.ts b/Utils.ts index e78e5f80d5..524b46835c 100644 --- a/Utils.ts +++ b/Utils.ts @@ -301,6 +301,10 @@ In the case that MapComplete is pointed to the testing grounds, the edit will be * @return the second parameter as is */ static Merge(source: any, target: any) { + if (target === null) { + return source + } + for (const key in source) { if (!source.hasOwnProperty(key)) { continue @@ -327,10 +331,7 @@ In the case that MapComplete is pointed to the testing grounds, the edit will be continue; } - const sourceV = source[key]; - if (target === null) { - return source - } + const sourceV = source[key] const targetV = target[key] if (typeof sourceV === "object") { if (sourceV === null) { @@ -668,5 +669,27 @@ In the case that MapComplete is pointed to the testing grounds, the edit will be b: parseInt(hex.substr(5, 2), 16), } } + + public static levenshteinDistance (str1: string, str2: string) { + const track = Array(str2.length + 1).fill(null).map(() => + Array(str1.length + 1).fill(null)); + for (let i = 0; i <= str1.length; i += 1) { + track[0][i] = i; + } + for (let j = 0; j <= str2.length; j += 1) { + track[j][0] = j; + } + for (let j = 1; j <= str2.length; j += 1) { + for (let i = 1; i <= str1.length; i += 1) { + const indicator = str1[i - 1] === str2[j - 1] ? 0 : 1; + track[j][i] = Math.min( + track[j][i - 1] + 1, // deletion + track[j - 1][i] + 1, // insertion + track[j - 1][i - 1] + indicator, // substitution + ); + } + } + return track[str2.length][str1.length]; + } } diff --git a/test/LegacyThemeLoader.spec.ts b/test/LegacyThemeLoader.spec.ts index 3e47d7d426..f0b09b7b45 100644 --- a/test/LegacyThemeLoader.spec.ts +++ b/test/LegacyThemeLoader.spec.ts @@ -4,6 +4,7 @@ import LayoutConfig from "../Models/ThemeConfig/LayoutConfig"; import {LayerConfigJson} from "../Models/ThemeConfig/Json/LayerConfigJson"; import {TagRenderingConfigJson} from "../Models/ThemeConfig/Json/TagRenderingConfigJson"; import {AddMiniMap} from "../Models/ThemeConfig/Conversion/PrepareTheme"; +import {DetectShadowedMappings} from "../Models/ThemeConfig/Conversion/Validation"; export default class LegacyThemeLoaderSpec extends T { @@ -235,18 +236,18 @@ export default class LegacyThemeLoaderSpec extends T { "question": { "nl": "Wat is het type inzamelpunt?" }, - "mappings":[ + "mappings": [ { - "if":"recycling_type=container", - "then":"Container of vat" + "if": "recycling_type=container", + "then": "Container of vat" }, { - "if":"recycling_type=centre", - "then":"Verwerkingsplaats of containerpark" + "if": "recycling_type=centre", + "then": "Verwerkingsplaats of containerpark" }, { - "if":"recycling_type=dump", - "then":"Composthoop" + "if": "recycling_type=dump", + "then": "Composthoop" } ], @@ -256,28 +257,28 @@ export default class LegacyThemeLoaderSpec extends T { "question": { "nl": "Wie mag hier organisch afval bezorgen?" }, - "mappings":[ + "mappings": [ { - "if":"access=yes", - "then":"Publiek toegankelijk" + "if": "access=yes", + "then": "Publiek toegankelijk" }, { - "if":"access=no", - "then":"Privaat" + "if": "access=no", + "then": "Privaat" }, { - "if":"access=permessive", - "then":"Mogelijks toegelaten tot nader order" + "if": "access=permessive", + "then": "Mogelijks toegelaten tot nader order" }, { - "if":"access=", - "then":"Waarschijnlijk publiek toegankelijk", - "hideInAnswer":true + "if": "access=", + "then": "Waarschijnlijk publiek toegankelijk", + "hideInAnswer": true }, { - "if":"access=residents", - "then":"Bewoners van gemeente", - "hideInAnswer":"recycling_type!=centre" + "if": "access=residents", + "then": "Bewoners van gemeente", + "hideInAnswer": "recycling_type!=centre" } ], @@ -294,15 +295,15 @@ export default class LegacyThemeLoaderSpec extends T { "render": { "nl": "De naam van dit compost-inzamelpunt is {name}" }, - "mappings":[ + "mappings": [ { - "if":{"and":["noname=yes","name="]}, - "then":"Heeft geen naam" + "if": {"and": ["noname=yes", "name="]}, + "then": "Heeft geen naam" }, { - "if":"name=", - "then":"Geen naam bekend", - "hideInAnswer":true + "if": "name=", + "then": "Geen naam bekend", + "hideInAnswer": true } ], "id": "Composthoekjes-name" @@ -410,7 +411,39 @@ export default class LegacyThemeLoaderSpec extends T { render: "Some random value {minimap}" }) - }] + }], + ["Shadowed mappings are detected", + () => { + const r = new DetectShadowedMappings().convert(undefined, { + mappings: [ + { + if: {or: ["key=value", "x=y"]}, + then: "Case A" + }, + { + if: "key=value", + then: "Shadowed" + } + ] + }, "test"); + T.isTrue(r.errors.length > 0, "Failing case is not detected") + + const r0 = new DetectShadowedMappings().convert(undefined, { + mappings: [ + { + if: {or: ["key=value", "x=y"]}, + then: "Case A" + }, + { + if: {and: ["key=value", "x=y"]}, + then: "Shadowed" + } + ] + }, "test"); + T.isTrue(r0.errors.length > 0, "Failing case is not detected") + } + + ] ] ); }