From 90916cdd32d084dcb0d075eb5414205de453e397 Mon Sep 17 00:00:00 2001 From: Pieter Vander Vennet Date: Sat, 24 Aug 2024 12:01:46 +0200 Subject: [PATCH] Cleanup of changeset handler, prep for #2082 --- scripts/handleErrors.ts | 2 +- scripts/importscripts/cleanRepair.ts | 2 +- src/Logic/Osm/Changes.ts | 92 +++++++------- src/Logic/Osm/ChangesetHandler.ts | 177 ++++++++++++++------------- src/Models/ThemeViewState.ts | 26 ++-- 5 files changed, 161 insertions(+), 138 deletions(-) diff --git a/scripts/handleErrors.ts b/scripts/handleErrors.ts index ac9b2c8f0..aff45af19 100644 --- a/scripts/handleErrors.ts +++ b/scripts/handleErrors.ts @@ -106,7 +106,7 @@ class HandleErrors extends Script { deletedObjects: OsmObject[] } = changesObj.CreateChangesetObjects(toUpload, objects) - const changeset = Changes.createChangesetFor("", changes) + const changeset = Changes.buildChangesetXML("", changes) const path = "error_changeset_" + parsed.index + "_" + e.layout + "_" + e.username + ".osc" if ( diff --git a/scripts/importscripts/cleanRepair.ts b/scripts/importscripts/cleanRepair.ts index 412b0673e..3549fb9dc 100644 --- a/scripts/importscripts/cleanRepair.ts +++ b/scripts/importscripts/cleanRepair.ts @@ -313,7 +313,7 @@ export default class CleanRepair extends Script { const changedObjects = changes.CreateChangesetObjects(changesToMake, objects) - const osc = Changes.createChangesetFor("", changedObjects) + const osc = Changes.buildChangesetXML("", changedObjects) writeFileSync("Cleanup.osc", osc, "utf8") } diff --git a/src/Logic/Osm/Changes.ts b/src/Logic/Osm/Changes.ts index a13d1e03e..14fe5980a 100644 --- a/src/Logic/Osm/Changes.ts +++ b/src/Logic/Osm/Changes.ts @@ -41,7 +41,7 @@ export class Changes { private readonly previouslyCreated: OsmObject[] = [] private readonly _leftRightSensitive: boolean public readonly _changesetHandler: ChangesetHandler - private readonly _reportError?: (string: string | Error) => void + private readonly _reportError?: (string: string | Error, extramessage?: string) => void constructor( state: { @@ -53,7 +53,7 @@ export class Changes { featureSwitches?: FeatureSwitchState }, leftRightSensitive: boolean = false, - reportError?: (string: string | Error) => void + reportError?: (string: string | Error, extramessage?: string) => void ) { this._leftRightSensitive = leftRightSensitive // We keep track of all changes just as well @@ -68,7 +68,7 @@ export class Changes { state.osmConnection, state.featurePropertiesStore, this, - (e) => this._reportError(e) + (e, extramessage: string) => this._reportError(e, extramessage) ) this.historicalUserLocations = state.historicalUserLocations @@ -76,7 +76,7 @@ export class Changes { // This doesn't matter however, as the '-1' is per piecewise upload, not global per changeset } - static createChangesetFor( + static buildChangesetXML( csId: string, allChanges: { modifiedObjects: OsmObject[] @@ -618,14 +618,15 @@ export class Changes { openChangeset: UIEventSource ): Promise { const neededIds = Changes.GetNeededIds(pending) - // We _do not_ pass in the Changes object itself - we want the data from OSM directly in order to apply the changes + /* Download the latest version of the OSM-objects + * We _do not_ pass in the Changes object itself - we want the data from OSM directly in order to apply the changes + */ const downloader = new OsmObjectDownloader(this.backend, undefined) - let osmObjects = await Promise.all<{ id: string; osmObj: OsmObject | "deleted" }>( + const osmObjects = Utils.NoNull(await Promise.all<{ id: string; osmObj: OsmObject | "deleted" }>( neededIds.map((id) => this.getOsmObject(id, downloader)) - ) - - osmObjects = Utils.NoNull(osmObjects) + )) + // Drop changes to deleted items for (const { osmObj, id } of osmObjects) { if (osmObj === "deleted") { pending = pending.filter((ch) => ch.type + "/" + ch.id !== id) @@ -645,20 +646,56 @@ export class Changes { return undefined } + const metatags = this.buildChangesetTags(pending) + + let { toUpload, refused } = this.fragmentChanges(pending, objects) + + if (toUpload.length === 0) { + return refused + } + await this._changesetHandler.UploadChangeset( + (csId, remappings) => { + if (remappings.size > 0) { + toUpload = toUpload.map((ch) => + ChangeDescriptionTools.rewriteIds(ch, remappings) + ) + } + + const changes: { + newObjects: OsmObject[] + modifiedObjects: OsmObject[] + deletedObjects: OsmObject[] + } = this.CreateChangesetObjects(toUpload, objects) + + return Changes.buildChangesetXML("" + csId, changes) + }, + metatags, + openChangeset + ) + + console.log("Upload successful! Refused changes are", refused) + return refused + } + + /** + * Builds all the changeset tags, such as `theme=cyclofix; answer=42; add-image: 5`, ... + */ + private buildChangesetTags(pending: ChangeDescription[]) { + // Build statistics for the changeset tags const perType = Array.from( Utils.Hist( pending .filter( (descr) => - descr.meta.changeType !== undefined && descr.meta.changeType !== null + descr.meta.changeType !== undefined && descr.meta.changeType !== null, ) - .map((descr) => descr.meta.changeType) + .map((descr) => descr.meta.changeType), ), ([key, count]) => ({ key: key, value: count, aggregate: true, - }) + }), ) const motivations = pending .filter((descr) => descr.meta.specialMotivation !== undefined) @@ -697,7 +734,7 @@ export class Changes { value: count, aggregate: true, } - }) + }), ) // This method is only called with changedescriptions for this theme @@ -720,34 +757,7 @@ export class Changes { ...motivations, ...perBinMessage, ] - - let { toUpload, refused } = this.fragmentChanges(pending, objects) - - if (toUpload.length === 0) { - return refused - } - await this._changesetHandler.UploadChangeset( - (csId, remappings) => { - if (remappings.size > 0) { - toUpload = toUpload.map((ch) => - ChangeDescriptionTools.rewriteIds(ch, remappings) - ) - } - - const changes: { - newObjects: OsmObject[] - modifiedObjects: OsmObject[] - deletedObjects: OsmObject[] - } = this.CreateChangesetObjects(toUpload, objects) - - return Changes.createChangesetFor("" + csId, changes) - }, - metatags, - openChangeset - ) - - console.log("Upload successful! Refused changes are", refused) - return refused + return metatags } private async flushChangesAsync(): Promise { diff --git a/src/Logic/Osm/ChangesetHandler.ts b/src/Logic/Osm/ChangesetHandler.ts index 8868b7932..92bc4bba6 100644 --- a/src/Logic/Osm/ChangesetHandler.ts +++ b/src/Logic/Osm/ChangesetHandler.ts @@ -13,6 +13,19 @@ export interface ChangesetTag { aggregate?: boolean } +export type ChangesetMetadata = { + type: "changeset" + id: number + created_at: string + open: boolean + uid: number + user: string + changes_count: number + tags: Record, + minlat: number, minlon: number, maxlat: number, maxlon: number + comments_count: number +} + export class ChangesetHandler { private readonly allElements: FeaturePropertiesStore private osmConnection: OsmConnection @@ -26,7 +39,7 @@ export class ChangesetHandler { * @private */ public readonly _remappings = new Map() - private readonly _reportError: (e: string | Error) => void + private readonly _reportError: (e: string | Error, extramsg: string) => void constructor( dryRun: Store, @@ -36,7 +49,7 @@ export class ChangesetHandler { | { addAlias: (id0: string, id1: string) => void } | undefined, changes: Changes, - reportError: (e: string | Error) => void + reportError: (e: string | Error, extramessage: string) => void, ) { this.osmConnection = osmConnection this._reportError = reportError @@ -94,6 +107,27 @@ export class ChangesetHandler { return hasChange } + private async UploadWithNew(generateChangeXML: (csid: number, remappings: Map) => string, openChangeset: UIEventSource, extraMetaTags: ChangesetTag[]) { + const csId = await this.OpenChangeset(extraMetaTags) + openChangeset.setData(csId) + const changeset = generateChangeXML(csId, this._remappings) + console.log( + "Opened a new changeset (openChangeset.data is undefined):", + changeset, + extraMetaTags, + ) + const changes = await this.UploadChange(csId, changeset) + const hasSpecialMotivationChanges = ChangesetHandler.rewriteMetaTags( + extraMetaTags, + changes, + ) + if (hasSpecialMotivationChanges) { + // At this point, 'extraMetaTags' will have changed - we need to set the tags again + await this.UpdateTags(csId, extraMetaTags) + } + } + + /** * The full logic to upload a change to one or more elements. * @@ -107,7 +141,7 @@ export class ChangesetHandler { public async UploadChangeset( generateChangeXML: (csid: number, remappings: Map) => string, extraMetaTags: ChangesetTag[], - openChangeset: UIEventSource + openChangeset: UIEventSource, ): Promise { if ( !extraMetaTags.some((tag) => tag.key === "comment") || @@ -130,83 +164,58 @@ export class ChangesetHandler { return } - if (openChangeset.data === undefined) { - // We have to open a new changeset - try { - const csId = await this.OpenChangeset(extraMetaTags) - openChangeset.setData(csId) - const changeset = generateChangeXML(csId, this._remappings) - console.log( - "Opened a new changeset (openChangeset.data is undefined):", - changeset, - extraMetaTags - ) - const changes = await this.UploadChange(csId, changeset) - const hasSpecialMotivationChanges = ChangesetHandler.rewriteMetaTags( - extraMetaTags, - changes - ) - if (hasSpecialMotivationChanges) { - // At this point, 'extraMetaTags' will have changed - we need to set the tags again - await this.UpdateTags(csId, extraMetaTags) - } - } catch (e) { - if (this._reportError) { - this._reportError(e) - } - if ((e).status === 400) { - // This request is invalid. We simply drop the changes and hope that someone will analyze what went wrong with it in the upload; we pretend everything went fine - return - } - console.warn( - "Could not open/upload changeset due to ", - e, - "trying again with a another fresh changeset " - ) - openChangeset.setData(undefined) - - throw e - } - } else { - // There still exists an open changeset (or at least we hope so) - // Let's check! - const csId = openChangeset.data + if (openChangeset.data) { try { + const csId = openChangeset.data const oldChangesetMeta = await this.GetChangesetMeta(csId) - if (!oldChangesetMeta.open) { - // Mark the CS as closed... - console.log("Could not fetch the metadata from the already open changeset") - openChangeset.setData(undefined) - // ... and try again. As the cs is closed, no recursive loop can exist - await this.UploadChangeset(generateChangeXML, extraMetaTags, openChangeset) - return + if (oldChangesetMeta.open) { + // We can hopefully reuse the changeset + + try { + + const rewritings = await this.UploadChange( + csId, + generateChangeXML(csId, this._remappings), + ) + + const rewrittenTags = this.RewriteTagsOf( + extraMetaTags, + rewritings, + oldChangesetMeta, + ) + await this.UpdateTags(csId, rewrittenTags) + return // We are done! + } catch (e) { + this._reportError(e, "While reusing a changeset " + openChangeset.data) + } + } - - const rewritings = await this.UploadChange( - csId, - generateChangeXML(csId, this._remappings) - ) - - const rewrittenTags = this.RewriteTagsOf( - extraMetaTags, - rewritings, - oldChangesetMeta - ) - await this.UpdateTags(csId, rewrittenTags) } catch (e) { - if (this._reportError) { - this._reportError( - "Could not reuse changeset " + - csId + - ", might be closed: " + - (e.stacktrace ?? e.status ?? "" + e) - ) - } - console.warn("Could not upload, changeset is probably closed: ", e) - openChangeset.setData(undefined) - throw e + this._reportError(e, "While getting metadata from a changeset " + openChangeset.data) } } + + + // We have to open a new changeset + try { + return await this.UploadWithNew(generateChangeXML, openChangeset, extraMetaTags) + } catch (e) { + if (this._reportError) { + this._reportError(e, "While opening a new changeset") + } + if ((e).status === 400) { + // This request is invalid. We simply drop the changes and hope that someone will analyze what went wrong with it in the upload; we pretend everything went fine + return + } + console.warn( + "Could not open/upload changeset due to ", + e, + "trying again with a another fresh changeset ", + ) + openChangeset.setData(undefined) + + throw e + } } /** @@ -227,7 +236,7 @@ export class ChangesetHandler { uid: number // User ID changes_count: number tags: any - } + }, ): ChangesetTag[] { // Note: extraMetaTags is where all the tags are collected into @@ -346,15 +355,9 @@ export class ChangesetHandler { console.log("Closed changeset ", changesetId) } - private async GetChangesetMeta(csId: number): Promise<{ - id: number - open: boolean - uid: number - changes_count: number - tags: any - }> { + private async GetChangesetMeta(csId: number): Promise { const url = `${this.backend}/api/0.6/changeset/${csId}` - const csData = await Utils.downloadJson(url) + const csData = await Utils.downloadJson<{ elements: ChangesetMetadata[] }>(url) return csData.elements[0] } @@ -370,7 +373,7 @@ export class ChangesetHandler { tag.key !== undefined && tag.value !== undefined && tag.key !== "" && - tag.value !== "" + tag.value !== "", ) const metadata = tags.map((kv) => ``) const content = [``, metadata, ``].join("") @@ -410,7 +413,7 @@ export class ChangesetHandler { const csId = await this.osmConnection.put( "changeset/create", [``, metadata, ``].join(""), - { "Content-Type": "text/xml" } + { "Content-Type": "text/xml" }, ) return Number(csId) } @@ -420,12 +423,12 @@ export class ChangesetHandler { */ private async UploadChange( changesetId: number, - changesetXML: string + changesetXML: string, ): Promise> { const response = await this.osmConnection.post( "changeset/" + changesetId + "/upload", changesetXML, - { "Content-Type": "text/xml" } + { "Content-Type": "text/xml" }, ) const changes = this.parseUploadChangesetResponse(response) console.log("Uploaded changeset ", changesetId) diff --git a/src/Models/ThemeViewState.ts b/src/Models/ThemeViewState.ts index 1bfcd52df..9abc22d81 100644 --- a/src/Models/ThemeViewState.ts +++ b/src/Models/ThemeViewState.ts @@ -278,7 +278,7 @@ export default class ThemeViewState implements SpecialVisualizationState { featureSwitches: this.featureSwitches, }, layout?.isLeftRightSensitive() ?? false, - (e) => this.reportError(e), + (e, extraMsg) => this.reportError(e, extraMsg), ) this.historicalUserLocations = this.geolocation.historicalUserLocations this.newFeatures = new NewGeometryFromChangesFeatureSource( @@ -650,9 +650,9 @@ export default class ThemeViewState implements SpecialVisualizationState { available, category, current.data, - skipLayers + skipLayers, ) - if(!best){ + if (!best) { return } console.log("Best layer for category", category, "is", best?.properties?.id) @@ -680,19 +680,19 @@ export default class ThemeViewState implements SpecialVisualizationState { Hotkeys.RegisterHotkey( { shift: "O" }, Translations.t.hotkeyDocumentation.selectOsmbasedmap, - () => setLayerCategory("osmbasedmap",2), + () => setLayerCategory("osmbasedmap", 2), ) Hotkeys.RegisterHotkey( { shift: "M" }, Translations.t.hotkeyDocumentation.selectMap, - () => setLayerCategory("map",2), + () => setLayerCategory("map", 2), ) Hotkeys.RegisterHotkey( { shift: "P" }, Translations.t.hotkeyDocumentation.selectAerial, - () => setLayerCategory("photo",2), + () => setLayerCategory("photo", 2), ) Hotkeys.RegisterHotkey( { nomod: "L" }, @@ -907,7 +907,7 @@ export default class ThemeViewState implements SpecialVisualizationState { this.selectedElement.setData(this.currentView.features?.data?.[0]) } - public async reportError(message: string | Error | XMLHttpRequest) { + public async reportError(message: string | Error | XMLHttpRequest, extramessage: string = "") { const isTesting = this.featureSwitchIsTesting.data console.log( isTesting @@ -922,7 +922,17 @@ export default class ThemeViewState implements SpecialVisualizationState { if ("" + message === "[object XMLHttpRequest]") { const req = message - message = "XMLHttpRequest with status code " + req.status + ", " + req.statusText + let body = "" + try { + body = req.responseText + } catch (e) { + // pass + } + message = "XMLHttpRequest with status code " + req.status + ", " + req.statusText + ", received: " + body + } + + if (extramessage) { + message += "(" + extramessage + ")" } const stacktrace: string = new Error().stack