Modify

Opened 9 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#18258 closed enhancement (fixed)

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

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 20.06
Component: Core Version:
Keywords: split way, original id Cc:

Description (last modified by 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.

Attachments (6)

18258.patch (3.3 KB) - added by taylor.smock 9 months ago.
Implementation of (2) (no unit tests)
18258.1.patch (9.8 KB) - added by taylor.smock 9 months ago.
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)
18258.2.patch (9.9 KB) - added by taylor.smock 9 months ago.
Actually save the variable passed in the constructor…
18258.4.patch (13.2 KB) - added by taylor.smock 4 weeks ago.
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
18258.5.patch (12.9 KB) - added by taylor.smock 4 weeks ago.
Add javadoc, enum is public, remove methods that would otherwise be deprecated (only affects the MapWithAI plugin, as far as we know)
18258.6.patch (10.5 KB) - added by taylor.smock 4 weeks ago.
Modify unit tests to match code expectations

Download all attachments as: .zip

Change History (20)

Changed 9 months ago by taylor.smock

Attachment: 18258.patch added

Implementation of (2) (no unit tests)

Changed 9 months ago by taylor.smock

Attachment: 18258.1.patch added

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)

comment:1 Changed 9 months ago by taylor.smock

Description: modified (diff)

Changed 9 months ago by taylor.smock

Attachment: 18258.2.patch added

Actually save the variable passed in the constructor...

comment:2 Changed 9 months ago by Don-vip

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

comment:3 in reply to:  2 ; Changed 9 months ago by 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?

comment:4 in reply to:  3 Changed 9 months ago by 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.

comment:5 Changed 5 weeks ago by simon04

Keywords: split way original id added
Milestone: 20.06

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?

comment:6 Changed 5 weeks ago by 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).

comment:7 in reply to:  6 Changed 5 weeks ago by 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

Last edited 5 weeks ago by simon04 (previous) (diff)

Changed 4 weeks ago by taylor.smock

Attachment: 18258.4.patch added

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

comment:8 Changed 4 weeks ago by simon04

Some comments on attachment:18258.4.patch​ …

  • List<Options> optionsCollection<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?

comment:9 in reply to:  8 Changed 4 weeks ago by taylor.smock

Replying to simon04:

Some comments on attachment:18258.4.patch​ …

  • List<Options> optionsCollection<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.

EDIT: I do have to wait for the next JOSM-latest before I can rebuild the plugin, so there will be some delay between the new JOSM-latest and me fixing the plugin.

Last edited 4 weeks ago by taylor.smock (previous) (diff)

Changed 4 weeks ago by taylor.smock

Attachment: 18258.5.patch added

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

comment:10 Changed 4 weeks ago by 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.

comment:11 in reply to:  10 Changed 4 weeks ago by 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.

Changed 4 weeks ago by taylor.smock

Attachment: 18258.6.patch added

Modify unit tests to match code expectations

comment:12 Changed 4 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 16641/josm:

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

comment:13 Changed 4 weeks ago by simon04

In 16642/josm:

see #18258 - OsmReaderTest: test all options combinations

comment:14 Changed 3 weeks ago by 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!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.