[RFC PATCH] Allow end user to know what the original id of a feature was

[taylor.smock]

There are two options:

1) Set the id of the object to the "original" id
2) Add a tag with the "original" id. This would have to be a "well-known" tag to avoid other people using it (current_id probably isn't a good choice, maybe xml_current_id?).

For the attached patch, I did the latter. There are some unit tests, but one is deliberately failing until a strategy is decided upon.

[taylor.smock]

Implementation of (2) (no unit tests)

[taylor.smock]

Add generic unit tests, with a test that will fail until a strategy is decided upon (at which point it will need to be modified)

[taylor.smock]

Actually save the variable passed in the constructor...

[Don-vip]

Sorry I must have missed something, what is this ticket about?

[taylor.smock]

The MapWithAI service passes back roads with an id. This id is used for deduplication using the xml attribute orig_id (the reason I filed bug #18249).

A more complete example of what the MapWithAI service provides is as follows:

From the following split download request:

$ curl "https://www.facebook.com/maps/ml_roads?conflate_with_osm=true&theme=ml_road_vector&collaborator=josm&token=ASb3N5o9HbX8QWn8G_NtHIRQaYv3nuG2r7_f3vnGld3KhZNCxg57IsaQyssIaEw5rfRNsPpMwg4TsnrSJtIJms5m&hash=ASawRla3rBcwEjY4HIY&result_type=road_building_vector_xml&bbox=110.6367552,1.032616,110.6452774,1.0375754" > 1.osm
$ curl "https://www.facebook.com/maps/ml_roads?conflate_with_osm=true&theme=ml_road_vector&collaborator=josm&token=ASb3N5o9HbX8QWn8G_NtHIRQaYv3nuG2r7_f3vnGld3KhZNCxg57IsaQyssIaEw5rfRNsPpMwg4TsnrSJtIJms5m&hash=ASawRla3rBcwEjY4HIY&result_type=road_building_vector_xml&bbox=110.6282331,1.032616,110.6367552,1.0375754" > 2.osm
$ curl "https://www.facebook.com/maps/ml_roads?conflate_with_osm=true&theme=ml_road_vector&collaborator=josm&token=ASb3N5o9HbX8QWn8G_NtHIRQaYv3nuG2r7_f3vnGld3KhZNCxg57IsaQyssIaEw5rfRNsPpMwg4TsnrSJtIJms5m&hash=ASawRla3rBcwEjY4HIY&result_type=road_building_vector_xml&bbox=110.6367552,1.0276566,110.6452774,1.032616" > 3.osm
$ curl "https://www.facebook.com/maps/ml_roads?conflate_with_osm=true&theme=ml_road_vector&collaborator=josm&token=ASb3N5o9HbX8QWn8G_NtHIRQaYv3nuG2r7_f3vnGld3KhZNCxg57IsaQyssIaEw5rfRNsPpMwg4TsnrSJtIJms5m&hash=ASawRla3rBcwEjY4HIY&result_type=road_building_vector_xml&bbox=110.6282331,1.0276566,110.6367552,1.032616" > 4.osm

1.osm

  <way action="modify" id="-2694366360314326357" orig_id="-358790951424482" visible="true">
    <nd ref="-2310517922340745"/>
    <nd ref="-343968989585587"/>
    <more nodes.../>
    <tag k="highway" v="residential"/>
    <tag k="source" v="digitalglobe"/>
  </way>

4.osm

  <way action="modify" id="-358790951424482" visible="true">
    <more nodes.../>
    <nd ref="-343968989585587"/>
    <nd ref="-2310517922340745"/>
    <more nodes.../>
    <tag k="highway" v="residential"/>
    <tag k="source" v="digitalglobe"/>
  </way>

Right now I'm looking at overriding buildPrimitive from AbstractReader.java to do the same thing that I'm doing in this patch (adding a tag), but I thought that it might be better to have the functionality in core JOSM to have the ability to keep the "original" id of a negative id object, either by setting its id to the original id or by adding a tag with the original id.

For example, I could modify the buildPrimitive method to keep the original id like so:

    @Override
    protected OsmPrimitive buildPrimitive(PrimitiveData pd) {
        OsmPrimitive p;
        if (pd.getUniqueId() < 0) {
            p = pd.getType().newInstance(pd.getUniqueId(), true);
            if (pd.getUniqueId() < AbstractPrimitive.currentUniqueId()) {
                AbstractPrimitive.advanceUniqueId(pd.getUniqueId());
            }
        } else {
            p = pd.getType().newVersionedInstance(pd.getId(), pd.getVersion());
        }
        p.setVisible(pd.isVisible());
        p.load(pd);
        externalIdMap.put(pd.getPrimitiveId(), p);
        return p;
    }

but that duplicates some code, and I thought it might be generally useful.

Does this answer your question?

From Don-vip:

Sorry I must have missed something, what is this ticket about?

[Don-vip]

Replying to taylor.smock:

Does this answer your question?

Yes thanks, I understand better now.

I must give some thoughts to the question, right now I don't know what would be the best solution.

[simon04]

Instead of ending up with many boolean flags in OsmReader, I'd suggest to use an enum, such as:

enum Options {CONVERT_UNKNOWN_TO_TAGS, SAVE_ORIGINAL_ID}

… and using OsmReader(Options... options).

How to avoid uploading the tag current_id, or is this meant to be uploaded?

[taylor.smock]

I like your enum idea. :)

So parseDataSet(InputStream source, ProgressMonitor progressMonitor, Options... options).

For avoiding uploading current_id, that would have to be the responsibility of additional code. For example, in MapWithAI, I have some code specifically to avoid that situation (where the original id is uploaded to a service -- I don't think it is actually used right now though).

[simon04]

Replying to taylor.smock:

I like your enum idea. :)

Inspired by the functions in java.nio.file.Files

For avoiding uploading current_id, that would have to be the responsibility of additional code.

We could add the tag to org.openstreetmap.josm.data.osm.AbstractPrimitive#getDiscardableKeys

[taylor.smock]

Use an enum instead of a bunch of booleans, mark old methods with booleans as deprecated (not removed), add current_id to the list of discardable keys

[simon04]

Some comments on attachment:18258.4.patch​​ …

• List<Options> options → Collection<Options> options
• enum Options needs to be protected/public, please add Javadoc
• From my investigations, MapWithAI is the only plugin using convertUnknownToTags. If you want to, we can remove the corresponding constructor and factory function without deprecating it.
• In OsmReaderTest#testValidData the boolean flags aren't used at all?

[taylor.smock]

Replying to simon04:

Some comments on attachment:18258.4.patch​​ …

• List<Options> options → Collection<Options> options

Fair enough. I didn't do anything specific with a List. I just tend to default to List over Collection, just so I can .get(index).

• enum Options needs to be protected/public, please add Javadoc

OK.

• From my investigations, MapWithAI is the only plugin using convertUnknownToTags. If you want to, we can remove the corresponding constructor and factory function without deprecating it.

I don't mind doing this. I prefer to keep compatibility for the past month (at least) though. It just makes it a bit easier to backport fixes to the version of the plugin for JOSM stable. This is small enough and contained enough that I can (fairly) easily make the modification. I just hope we have a release in July. :)

• In OsmReaderTest#testValidData the boolean flags aren't used at all?

This probably tracks with your previous point. I didn't see the point of having a test calling an @deprecated function, when said function was in turn calling the appropriate function.

[taylor.smock]

Add javadoc, enum is public, remove methods that would otherwise be deprecated (only affects the MapWithAI plugin, as far as we know)

[taylor.smock]

@simon04: I'm assuming you prefer the current_id method over setting the id of the object to the id returned by the server (just clarifying -- if you and everyone else is ok with current_id, I need to remove the testNegativeIds test, since that is failing).

When I originally wrote the patch, I had two unit tests which were mutually exclusive. One tested that a negative id from the data source would be set to the object id, while the other tested that it was set to the current_id.

[simon04]

Thanks for updating your patch!

Replying to taylor.smock:

@simon04: I'm assuming you prefer the current_id method over setting the id of the object to the id returned by the server

Yes, a similar procedure is used when converting a GPX to an OSM dataset.

[taylor.smock]

Modify unit tests to match code expectations

[simon04]

In 16641/josm:

fix #18258 - OsmReader: Allow end user to know what the original id of a feature was (patch by taylor.smock, modified)

[simon04]

In 16642/josm:

see #18258 - OsmReaderTest: test all options combinations

[taylor.smock]

MapWithAI is fixed (so https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/lastBuild/warnings3Result/package.865753307/ isn't relevant anymore).

See ​v1.6.1.

Thanks for merging the patch!

[stoecker]

In 18646/josm:

see #18258 - add a note why current_id is here