Simplify handling of id rewrites by centralizing them to the changesethandler (which improves testability too), fixes a part of #564

Logic/ElementStorage.ts

@@ -49,6 +49,29 @@ export class ElementStorage {
         return this._elements.has(id);
     }
 
+    addAlias(oldId: string, newId: string){
+        if (newId === undefined) {
+            // We removed the node/way/relation with type 'type' and id 'oldId' on openstreetmap!
+            const element = this.getEventSourceById(oldId);
+            element.data._deleted = "yes"
+            element.ping();
+            return;
+        }
+        
+        if (oldId == newId) {
+            return undefined;
+        }
+        const element = this.getEventSourceById( oldId);
+        if (element === undefined) {
+            // Element to rewrite not found, probably a node or relation that is not rendered
+            return undefined
+        }
+        element.data.id = newId;
+        this.addElementById(newId, element);
+        this.ContainingFeatures.set(newId, this.ContainingFeatures.get( oldId))
+        element.ping();
+    }
+    
     private addOrGetById(elementId: string, newProperties: any): UIEventSource<any> {
         if (!this._elements.has(elementId)) {
             const eventSource = new UIEventSource<any>(newProperties, "tags of " + elementId);

Logic/Osm/Actions/ChangeDescription.ts

@@ -71,6 +71,110 @@ export interface ChangeDescription {
 
 export class ChangeDescriptionTools {
 
+    /**
+     * Rewrites all the ids in a changeDescription
+     *
+     * // should rewrite the id of the changed object
+     * const change = <ChangeDescription> {
+     *             id: -1234,
+     *             type: "node",
+     *             meta:{
+     *                 theme:"test",
+     *                 changeType: "answer"
+     *             },
+     *             tags:[
+     *                 {
+     *                     k: "key",
+     *                     v: "value"
+     *                 }
+     *             ]
+     *         }
+     * }
+     * const mapping = new Map<string, string>([["node/-1234", "node/42"]])
+     * const rewritten = ChangeDescriptionTools.rewriteIds(change, mapping)
+     * rewritten.id // => 42
+     *
+     * // should rewrite ids in nodes of a way
+     * const change = <ChangeDescription> {
+     *     type: "way",
+     *     id: 789,
+     *     changes: {
+     *         nodes: [-1, -2, -3, 68453],
+     *         coordinates: []
+     *     },
+     *     meta:{
+     *         theme:"test",
+     *         changeType: "create"
+     *     }
+     * }
+     * const mapping = new Map<string, string>([["node/-1", "node/42"],["node/-2", "node/43"],["node/-3", "node/44"]])
+     * const rewritten = ChangeDescriptionTools.rewriteIds(change, mapping)
+     * rewritten.id // => 789
+     * rewritten.changes["nodes"] // => [42,43,44, 68453]
+     * 
+     * // should rewrite ids in relationship members
+     * const change = <ChangeDescription> {
+     *     type: "way",
+     *     id: 789,
+     *     changes: {
+     *         members: [{type: "way", ref: -1, role: "outer"},{type: "way", ref: 48, role: "outer"}],
+     *     },
+     *     meta:{
+     *         theme:"test",
+     *         changeType: "create"
+     *     }
+     * }
+     * const mapping = new Map<string, string>([["way/-1", "way/42"],["node/-2", "node/43"],["node/-3", "node/44"]])
+     * const rewritten = ChangeDescriptionTools.rewriteIds(change, mapping)
+     * rewritten.id // => 789
+     * rewritten.changes["members"] // => [{type: "way", ref: 42, role: "outer"},{type: "way", ref: 48, role: "outer"}]
+     *
+     */
+    public static rewriteIds(change: ChangeDescription, mappings: Map<string, string>): ChangeDescription {
+        const key = change.type + "/" + change.id
+
+        const wayHasChangedNode = ((change.changes ?? {})["nodes"] ?? []).some(id => mappings.has("node/" + id));
+        const relationHasChangedMembers = ((change.changes ?? {})["members"] ?? [])
+            .some((obj:{type: string, ref: number}) => mappings.has(obj.type+"/" + obj.ref));
+
+        const hasSomeChange = mappings.has(key)
+            || wayHasChangedNode || relationHasChangedMembers
+        if(hasSomeChange){
+            change = {...change}
+        }
+        
+        if (mappings.has(key)) {
+            const [_, newId] = mappings.get(key).split("/")
+            change.id = Number.parseInt(newId)
+        }
+        if(wayHasChangedNode){
+            change.changes = {...change.changes}
+            change.changes["nodes"] = change.changes["nodes"].map(id => {
+                const key = "node/"+id
+                if(!mappings.has(key)){
+                    return id
+                }
+                const [_, newId] = mappings.get(key).split("/")
+                return Number.parseInt(newId)
+            })
+        }
+        if(relationHasChangedMembers){
+            change.changes = {...change.changes}
+            change.changes["members"] = change.changes["members"].map(
+                (obj:{type: string, ref: number}) => {
+                    const key = obj.type+"/"+obj.ref;
+                    if(!mappings.has(key)){
+                        return obj
+                    }
+                    const [_, newId] = mappings.get(key).split("/")
+                    return {...obj, ref: Number.parseInt(newId)}
+                }
+            )
+        }
+
+        return change
+    }
+
     public static getGeojsonGeometry(change: ChangeDescription): any {
         switch (change.type) {
             case "node":

Logic/Osm/Actions/CreateNewNodeAction.ts

@@ -51,22 +51,6 @@ export default class CreateNewNodeAction extends OsmCreateAction {
         }
     }
 
-    public static registerIdRewrites(mappings: Map<string, string>) {
-        const toAdd: [string, number][] = []
-
-        this.previouslyCreatedPoints.forEach((oldId, key) => {
-            if (!mappings.has("node/" + oldId)) {
-                return;
-            }
-
-            const newId = Number(mappings.get("node/" + oldId).substr("node/".length))
-            toAdd.push([key, newId])
-        })
-        for (const [key, newId] of toAdd) {
-            CreateNewNodeAction.previouslyCreatedPoints.set(key, newId)
-        }
-    }
-
     async CreateChangeDescriptions(changes: Changes): Promise<ChangeDescription[]> {
 
         if (this._reusePreviouslyCreatedPoint) {

Logic/Osm/Changes.ts

@@ -2,7 +2,7 @@ import {OsmNode, OsmObject, OsmRelation, OsmWay} from "./OsmObject";
 import {UIEventSource} from "../UIEventSource";
 import Constants from "../../Models/Constants";
 import OsmChangeAction from "./Actions/OsmChangeAction";
-import {ChangeDescription} from "./Actions/ChangeDescription";
+import {ChangeDescription, ChangeDescriptionTools} from "./Actions/ChangeDescription";
 import {Utils} from "../../Utils";
 import {LocalStorageSource} from "../Web/LocalStorageSource";
 import SimpleMetaTagger from "../SimpleMetaTagger";
@@ -142,11 +142,7 @@ export class Changes {
         this.allChanges.data.push(...changes)
         this.allChanges.ping()
     }
-
+    
-    public registerIdRewrites(mappings: Map<string, string>): void {
-        CreateNewNodeAction.registerIdRewrites(mappings)
-    }
-
     private calculateDistanceToChanges(change: OsmChangeAction, changeDescriptions: ChangeDescription[]) {
 
         const locations = this.historicalUserLocations?.features?.data
@@ -226,17 +222,6 @@ export class Changes {
         }
 
         console.log("Got the fresh objects!", osmObjects, "pending: ", pending)
-        const changes: {
-            newObjects: OsmObject[],
-            modifiedObjects: OsmObject[]
-            deletedObjects: OsmObject[]
-        } = self.CreateChangesetObjects(pending, osmObjects)
-        if (changes.newObjects.length + changes.deletedObjects.length + changes.modifiedObjects.length === 0) {
-            console.log("No changes to be made")
-            return true
-        }
-
-
         const perType = Array.from(
             Utils.Hist(pending.filter(descr => descr.meta.changeType !== undefined && descr.meta.changeType !== null)
                 .map(descr => descr.meta.changeType)), ([key, count]) => (
@@ -303,7 +288,19 @@ export class Changes {
         ]
 
         await this._changesetHandler.UploadChangeset(
-            (csId) => Changes.createChangesetFor("" + csId, changes),
+            (csId, remappings) =>{
+                if(remappings.size > 0){
+                    console.log("Rewriting pending changes from", pending, "with", remappings)
+                    pending = pending.map(ch => ChangeDescriptionTools.rewriteIds(ch, remappings))
+                    console.log("Result is", pending)
+                }
+                const changes: {
+                    newObjects: OsmObject[],
+                    modifiedObjects: OsmObject[]
+                    deletedObjects: OsmObject[]
+                } = self.CreateChangesetObjects(pending, osmObjects)
+               return Changes.createChangesetFor("" + csId, changes)
+            },
             metatags,
             openChangeset
         )

Logic/Osm/ChangesetHandler.ts

@@ -23,6 +23,14 @@ export class ChangesetHandler {
     private readonly auth: any;
     private readonly backend: string;
 
+
+    /**
+     * Contains previously rewritten IDs
+     * @private
+     */
+    private readonly _remappings = new Map<string, string>()
+
+
     /**
      * Use 'osmConnection.CreateChangesetHandler' instead
      * @param dryRun
@@ -50,6 +58,7 @@ export class ChangesetHandler {
 
     }
 
+    
     /**
      * Creates a new list which contains every key at most once
      * 
@@ -104,7 +113,7 @@ export class ChangesetHandler {
      *
      */
     public async UploadChangeset(
-        generateChangeXML: (csid: number) => string,
+        generateChangeXML: (csid: number, remappings: Map<string, string>) => string,
         extraMetaTags: ChangesetTag[],
         openChangeset: UIEventSource<number>): Promise<void> {
 
@@ -120,7 +129,7 @@ export class ChangesetHandler {
             this.userDetails.ping();
         }
         if (this._dryRun.data) {
-            const changesetXML = generateChangeXML(123456);
+            const changesetXML = generateChangeXML(123456, this._remappings);
             console.log("Metatags are", extraMetaTags)
             console.log(changesetXML);
             return;
@@ -131,7 +140,7 @@ export class ChangesetHandler {
             try {
                 const csId = await this.OpenChangeset(extraMetaTags)
                 openChangeset.setData(csId);
-                const changeset = generateChangeXML(csId);
+                const changeset = generateChangeXML(csId, this._remappings);
                 console.trace("Opened a new changeset (openChangeset.data is undefined):", changeset);
                 const changes = await this.UploadChange(csId, changeset)
                 const hasSpecialMotivationChanges = ChangesetHandler.rewriteMetaTags(extraMetaTags, changes)
@@ -162,7 +171,7 @@ export class ChangesetHandler {
 
                 const rewritings = await this.UploadChange(
                     csId,
-                    generateChangeXML(csId))
+                    generateChangeXML(csId, this._remappings))
 
                 const rewrittenTags = this.RewriteTagsOf(extraMetaTags, rewritings, oldChangesetMeta)
                 await this.UpdateTags(csId, rewrittenTags)
@@ -239,57 +248,67 @@ export class ChangesetHandler {
     
     }
 
-    private handleIdRewrite(node: any, type: string): [string, string] {
+    /**
+     * Updates the id in the AllElements store, returns the new ID
+     * @param node: the XML-element, e.g.  <node old_id="-1" new_id="9650458521" new_version="1"/>
+     * @param type
+     * @private
+     */
+    private static parseIdRewrite(node: any, type: string): [string, string] {
         const oldId = parseInt(node.attributes.old_id.value);
         if (node.attributes.new_id === undefined) {
-            // We just removed this point!
+            return [type+"/"+oldId, undefined];
-            const element = this.allElements.getEventSourceById("node/" + oldId);
-            element.data._deleted = "yes"
-            element.ping();
-            return;
         }
 
         const newId = parseInt(node.attributes.new_id.value);
+        // The actual mapping
         const result: [string, string] = [type + "/" + oldId, type + "/" + newId]
-        if (!(oldId !== undefined && newId !== undefined &&
+        if(oldId === newId){
-            !isNaN(oldId) && !isNaN(newId))) {
             return undefined;
         }
-        if (oldId == newId) {
-            return undefined;
-        }
-        const element = this.allElements.getEventSourceById("node/" + oldId);
-        if (element === undefined) {
-            // Element to rewrite not found, probably a node or relation that is not rendered
-            return undefined
-        }
-        element.data.id = type + "/" + newId;
-        this.allElements.addElementById(type + "/" + newId, element);
-        this.allElements.ContainingFeatures.set(type + "/" + newId, this.allElements.ContainingFeatures.get(type + "/" + oldId))
-        element.ping();
         return result;
     }
 
+    /**
+     * Given a diff-result XML of the form 
+     * <diffResult version="0.6">
+     *  <node old_id="-1" new_id="9650458521" new_version="1"/>
+     *  <way old_id="-2" new_id="1050127772" new_version="1"/>
+     * </diffResult>,
+     * will:
+     * 
+     * - create a mapping `{'node/-1' --> "node/9650458521", 'way/-2' --> "way/9650458521"}
+     * - Call this.changes.registerIdRewrites
+     * - Call handleIdRewrites as needed
+     * @param response
+     * @private
+     */
     private parseUploadChangesetResponse(response: XMLDocument): Map<string, string> {
         const nodes = response.getElementsByTagName("node");
-        const mappings = new Map<string, string>()
+        const mappings : [string, string][]= []
         
         for (const node of Array.from(nodes)) {
-            const mapping = this.handleIdRewrite(node, "node")
+            const mapping = ChangesetHandler.parseIdRewrite(node, "node")
             if (mapping !== undefined) {
-                mappings.set(mapping[0], mapping[1])
+                mappings.push(mapping)
             }
         }
 
         const ways = response.getElementsByTagName("way");
         for (const way of Array.from(ways)) {
-            const mapping = this.handleIdRewrite(way, "way")
+            const mapping = ChangesetHandler.parseIdRewrite(way, "way")
             if (mapping !== undefined) {
-                mappings.set(mapping[0], mapping[1])
+                mappings.push(mapping)
             }
         }
-        this.changes.registerIdRewrites(mappings)
+        for (const mapping of mappings) {
-        return mappings
+            const [oldId, newId] = mapping
+            this.allElements.addAlias(oldId, newId);
+            if(newId !== undefined) {
+                this._remappings.set(mapping[0], mapping[1])
+            }
+        }
+        return new Map<string, string>(mappings)
 
     }
 
@@ -335,7 +354,6 @@ export class ChangesetHandler {
         tags: ChangesetTag[]) {
         tags = ChangesetHandler.removeDuplicateMetaTags(tags)
 
-        console.trace("Updating tags of " + csId)
         const self = this;
         return new Promise<string>(function (resolve, reject) {
 
@@ -351,7 +369,7 @@ export class ChangesetHandler {
                     `</changeset></osm>`].join("")
             }, function (err, response) {
                 if (response === undefined) {
-                    console.log("err", err);
+                    console.error("Updating the tags of changeset "+csId+" failed:", err);
                     reject(err)
                 } else {
                     resolve(response);
@@ -397,7 +415,7 @@ export class ChangesetHandler {
                     `</changeset></osm>`].join("")
             }, function (err, response) {
                 if (response === undefined) {
-                    console.log("err", err);
+                    console.error("Opening a changeset failed:", err);
                     reject(err)
                 } else {
                     resolve(Number(response));
@@ -421,7 +439,7 @@ export class ChangesetHandler {
                 content: changesetXML
             }, function (err, response) {
                 if (response == null) {
-                    console.log("err", err);
+                    console.error("Uploading an actual change failed", err);
                     reject(err);
                 }
                 const changes = self.parseUploadChangesetResponse(response);

Logic/Tags/TagUtils.ts

@@ -11,7 +11,7 @@ import {isRegExp} from "util";
 import * as key_counts from "../../assets/key_totals.json"
 
 export class TagUtils {
-    private static keyCounts : {keys: any, tags: any} = key_counts["default"] ?? key_counts
+    private static keyCounts: { keys: any, tags: any } = key_counts["default"] ?? key_counts
     private static comparators
         : [string, (a: number, b: number) => boolean][]
         = [
@@ -169,23 +169,23 @@ export class TagUtils {
 
     /**
      * Returns wether or not a keys is (probably) a valid key.
-     * 
+     *
      * // should accept common keys
      * TagUtils.isValidKey("name") // => true
      * TagUtils.isValidKey("image:0") // => true
      * TagUtils.isValidKey("alt_name") // => true
-     * 
+     *
      * // should refuse short keys
      * TagUtils.isValidKey("x") // => false
      * TagUtils.isValidKey("xy") // => false
-     * 
+     *
      * // should refuse a string with >255 characters
      * let a255 = ""
      * for(let i = 0; i < 255; i++) { a255 += "a"; }
      * a255.length // => 255
      * TagUtils.isValidKey(a255) // => true
      * TagUtils.isValidKey("a"+a255) // => false
-     * 
+     *
      * // Should refuse unexpected characters
      * TagUtils.isValidKey("with space") // => false
      * TagUtils.isValidKey("some$type") // => false
@@ -194,10 +194,10 @@ export class TagUtils {
     public static isValidKey(key: string): boolean {
         return key.match(/^[a-z][a-z0-9:_]{2,253}[a-z0-9]$/) !== null
     }
-    
+
     /**
      * Parses a tag configuration (a json) into a TagsFilter
-     * 
+     *
      * TagUtils.Tag("key=value") // => new Tag("key", "value")
      * TagUtils.Tag("key=") // => new Tag("key", "")
      * TagUtils.Tag("key!=") // => new RegexTag("key", "^..*$")
@@ -210,6 +210,9 @@ export class TagUtils {
      * TagUtils.Tag("xyz!~\\[\\]") // => new RegexTag("xyz", /^\[\]$/, true)
      * TagUtils.Tag("tags~(^|.*;)amenity=public_bookcase($|;.*)") // => new RegexTag("tags", /(^|.*;)amenity=public_bookcase($|;.*)/)
      * TagUtils.Tag("service:bicycle:.*~~*") // => new RegexTag(/^service:bicycle:.*$/, /^..*$/)
+     *
+     * TagUtils.Tag("xyz<5").matchesProperties({xyz: 4}) // => true
+     * TagUtils.Tag("xyz<5").matchesProperties({xyz: 5}) // => false
      */
     public static Tag(json: AndOrTagConfigJson | string, context: string = ""): TagsFilter {
         try {
@@ -224,7 +227,7 @@ export class TagUtils {
      * INLINE sort of the given list
      */
     public static sortFilters(filters: TagsFilter [], usePopularity: boolean): void {
-        filters.sort((a,b) => TagUtils.order(a, b, usePopularity))
+        filters.sort((a, b) => TagUtils.order(a, b, usePopularity))
     }
 
     public static toString(f: TagsFilter, toplevel = true): string {
@@ -236,10 +239,10 @@ export class TagUtils {
         } else {
             r = f.asHumanString(false, false, {})
         }
-        if(toplevel){
+        if (toplevel) {
             r = r.trim()
         }
-        
+
         return r
     }
 
@@ -260,8 +263,8 @@ export class TagUtils {
             }
             throw "At " + context + ": unrecognized tag"
         }
-        
+
-        
+
         const tag = json as string;
         for (const [operator, comparator] of TagUtils.comparators) {
             if (tag.indexOf(operator) >= 0) {
@@ -272,12 +275,19 @@ export class TagUtils {
                     val = new Date(split[1].trim()).getTime()
                 }
 
-                const f = (value: string | undefined) => {
+                const f = (value: string | number | undefined) => {
                     if (value === undefined) {
                         return false;
                     }
-                    let b = Number(value?.trim())
+                    let b: number
-                    if (isNaN(b)) {
+                    if (typeof value === "number") {
+                        b = value
+                    } else if (typeof b === "string") {
+                        b = Number(value?.trim())
+                    } else {
+                        b = Number(value)
+                    }
+                    if (isNaN(b) && typeof value === "string") {
                         b = Utils.ParseDate(value).getTime()
                         if (isNaN(b)) {
                             return false
@@ -306,8 +316,8 @@ export class TagUtils {
                 split[1] = "..*"
             }
             return new RegexTag(
-                new RegExp("^"+split[0]+"$"),
+                new RegExp("^" + split[0] + "$"),
-                new RegExp("^"+ split[1]+"$")
+                new RegExp("^" + split[1] + "$")
             );
         }
         if (tag.indexOf("!:=") >= 0) {
@@ -371,32 +381,32 @@ export class TagUtils {
     }
 
     private static GetCount(key: string, value?: string) {
-        if(key === undefined) {
+        if (key === undefined) {
             return undefined
         }
         const tag = TagUtils.keyCounts.tags[key]
-        if(tag !== undefined && tag[value] !== undefined) {
+        if (tag !== undefined && tag[value] !== undefined) {
             return tag[value]
         }
         return TagUtils.keyCounts.keys[key]
     }
-    
+
     private static order(a: TagsFilter, b: TagsFilter, usePopularity: boolean): number {
         const rta = a instanceof RegexTag
         const rtb = b instanceof RegexTag
-        if(rta !== rtb) {
+        if (rta !== rtb) {
             // Regex tags should always go at the end: these use a lot of computation at the overpass side, avoiding it is better
-            if(rta) {
+            if (rta) {
                 return 1 // b < a
-            }else {
+            } else {
                 return -1
             }
         }
         if (a["key"] !== undefined && b["key"] !== undefined) {
-            if(usePopularity) {
+            if (usePopularity) {
                 const countA = TagUtils.GetCount(a["key"], a["value"])
                 const countB = TagUtils.GetCount(b["key"], b["value"])
-                if(countA !== undefined && countB !== undefined) {
+                if (countA !== undefined && countB !== undefined) {
                     return countA - countB
                 }
             }
@@ -420,5 +430,5 @@ export class TagUtils {
         }
         return " (" + joined + ") "
     }
-    
+
 }

Models/Constants.ts

@@ -2,7 +2,7 @@ import {Utils} from "../Utils";
 
 export default class Constants {
 
-    public static vNumber = "0.18.0-alpha-2";
+    public static vNumber = "0.18.0-alpha-3";
     
     public static ImgurApiKey = '7070e7167f0a25a'
     public static readonly mapillary_client_token_v4 = "MLY|4441509239301885|b40ad2d3ea105435bd40c7e76993ae85"