Modify

Opened 2 months ago

Closed 8 weeks ago

Last modified 2 weeks ago

#18566 closed defect (fixed)

[Patch] Download object: member of relations always downloaded

Reported by: skyper Owned by: GerdP
Priority: normal Milestone: 20.02
Component: Core Version:
Keywords: template_report download object relation member regression Cc: Don-vip

Description

What steps will reproduce the problem?

  1. open download object dialog
  2. choose "relation" as object type and enter id of relation
  3. make sure the second checkbox Download relations members is not checked

What is the expected result?

Only one object, the relation, is downloaded

What happens instead?

Despite the uncheck checkbox all members are downloaded

Please provide any additional information below. Attach a screenshot if possible.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-15 18:25:57 +0100 (Wed, 15 Jan 2020)
Revision:15713
Build-Date:2020-01-16 02:30:53
URL:https://josm.openstreetmap.de/svn/trunk

Attachments (7)

josm_download_object.png (22.7 KB) - added by skyper 2 months ago.
screenshot
18566-alpha.patch (11.9 KB) - added by GerdP 2 months ago.
18566-wip.patch (33.6 KB) - added by GerdP 2 months ago.
Work in progress
18566-v1.patch (36.1 KB) - added by GerdP 2 months ago.
rel-editor.PNG (26.3 KB) - added by GerdP 2 months ago.
18566.2.patch (37.1 KB) - added by GerdP 2 months ago.
overpass api behaves a bit strange. Brakets are needed for ways but not for relations.
18566.3.patch (42.5 KB) - added by GerdP 2 months ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 2 months ago by skyper

Keywords: regression added

comment:2 Changed 2 months ago by skyper

If run with remote control it even downloads the members of child relations.

comment:3 in reply to:  2 Changed 2 months ago by skyper

Replying to skyper:

If run with remote control it even downloads the members of child relations.

This happens also if you download relations as members with download (selected) members in the relation dialog

comment:4 Changed 2 months ago by Don-vip

Owner: changed from team to skyper
Status: newneedinfo

I don't reproduce. Please share object id, screenshots and .osm data layer.

comment:5 Changed 2 months ago by GerdP

Maybe "Use overpass for object downloads" is activated?

comment:6 in reply to:  5 Changed 2 months ago by skyper

Replying to GerdP:

Maybe "Use overpass for object downloads" is activated?

Yes, you are right for the general problem and the download from relation editor.

Still without overpass enabled:

  • if you download a type=route_master relation (r69202) its members are downloaded.
  • if you use remote control (69202) on a type=route_master relation its grandchildren are downloaded.

Did not test superroute.

comment:7 Changed 2 months ago by GerdP

Cc: Don-vip added

OK, I see. This is not related to the type of the relation. I think it is
The link in the 2nd example says relation_members=true so I think both problems have the same reason.
@Vincent: Maybe a regression of r10129? I wanted to try it but I can't compile the old source.
This small change seems to solve the issue:

  • src/org/openstreetmap/josm/gui/io/DownloadPrimitivesTask.java

     
    5252        super(tr("Download objects"), progressMonitor, layer);
    5353        this.ids = ids;
    5454        setZoom(true);
    55         setDownloadRelations(true, fullRelation);
     55        setDownloadRelations(false, fullRelation);
    5656    }
    5757
    5858    @Override

The javadoc for method setDownloadRelations() looks as if you planed to add something more but forgot it:

    /**
     * Sets whether .
     * @param downloadRelations {@code true} if
     * @param fullRelation {@code true} if a full download is required,
     *                     i.e., a download including the immediate children of a relation.
     * @return {@code this}
     */
    public final AbstractPrimitiveTask setDownloadRelations(boolean downloadRelations, boolean fullRelation) {
        this.downloadRelations = downloadRelations;
        this.fullRelation = fullRelation;
        return this;
    }

comment:8 Changed 2 months ago by GerdP

Probably not a regression in r10129. I tested with josm-snapshot-10123.jar and got the same result.

Changed 2 months ago by skyper

Attachment: josm_download_object.png added

screenshot

comment:9 Changed 2 months ago by skyper

Owner: changed from skyper to team
Status: needinfonew

screenshot of example:
screenshot

The behaviour is different depending on something between r69202 and r1080376.

  • without overpass r1080376 is fine but r69202 not.
  • with overpass both are treated the same with ignoring the checkbox.
Last edited 2 months ago by skyper (previous) (diff)

comment:10 Changed 2 months ago by GerdP

r69202 has relations as members, r1080376 doesn't.

comment:11 Changed 2 months ago by skyper

So we have two problems:

  • relations as members are always downloaded.
  • if use overpass to download objects is enabled the checkbox is not used.

comment:12 Changed 2 months ago by Don-vip

I guess it is intentional when I fixed #10388 in r7432. I don't remember the details.

comment:13 Changed 2 months ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

Besides the unclear code above another is that MultiFetchOverpassObjectReader.buildRequestString() always recurses down.
Also sometimes the standard api is used even when overpass api was selected.
I'll try to find a fix for these issues.

comment:14 Changed 2 months ago by GerdP

@Skyper: When you download relation 69202 "with members", you want to get the relation 69202 and its two sub relations 1080376 and 1080375, but not any node or way, right? This would be a big change, because now the checkbox "download relation members" means something like "load all until nothing is incomplete"

Last edited 2 months ago by GerdP (previous) (diff)

comment:15 in reply to:  14 Changed 2 months ago by skyper

Replying to GerdP:

@Skyper: When you download relation 69202 "with members", you want to get the relation 69202 and its two sub relations 1080376 and 1080375, but not any node or way, right?

Yes, I want to modify the tags of the relation(s), e.g. I am not interested in members at all (no members) or only in the members of the mentioned relation 69202 (download members) but no escalating down.

This would be a big change, because now the checkbox "download relation members" means something like "load all until nothing is incomplete"

The text for the check box says: Download relation members
Note the singular form of relation.

Could be a long time ago that it was changed but I think it did work differently years ago or am I off and I did always use "Download Location".

comment:16 Changed 2 months ago by GerdP

I think it should work like this (no matter if overpass or OSM api is used):
1) If you download relation r69202 with "download relation members" enabled you should get the complete relation, because "download relation members" always downloads node members and way members completely. It should also download relation members completely to be consistent. I don't understand your hint reg. "singular form". The dialog also allows to download two or more relations and members is plural.
2) If you download relation 69202 with "download relation members" disabled you should get only the incomplete relation.

I am not yet sure what I would want to get when I use update selection when a relation like r69202 is selected. I was surprised to see that lots of nodes and ways are downloaded when the selected relation is complete. I expected to see the same downloads as with 1)

comment:17 in reply to:  12 ; Changed 2 months ago by GerdP

Replying to Don-vip:

I guess it is intentional when I fixed #10388 in r7432. I don't remember the details.

Yes, likely. With snapshot r7431 I see the expected (1) behaviour, with r7435 it works like now (overpass api wasn't yet implemented)
(1) as described in comment:16

Last edited 2 months ago by GerdP (previous) (diff)

comment:18 in reply to:  17 Changed 2 months ago by skyper

Replying to GerdP:

I think it should work like this (no matter if overpass or OSM api is used):
1) If you download relation r69202 with "download relation members" enabled you should get the complete relation, because "download relation members" always downloads node members and way members completely. It should also download relation members completely to be consistent. I don't understand your hint reg. "singular form". The dialog also allows to download two or more relations and members is plural.
2) If you download relation 69202 with "download relation members" disabled you should get only the incomplete relation.

I can life with this though a new checkbox Only download 1st level members would be nice.
Do not want to get picky. There are relations with only one member. The correct plural would be: Download relations members'

Replying to GerdP:

Yes, likely. With snapshot r7431 I see the expected (1) behaviour, with r7435 it works like now (overpass api wasn't yet implemented)
(1) as described in comment:16

Overpass was a plugin before getting into core. Think the option to use overpass to download objects existed already, five years ago, with the plugin.

comment:19 Changed 2 months ago by GerdP

BTW: For me, "Use overpass for object downloads" is always much slower compared to OSM api, so I only use it for testing. What's your reason to use it?

comment:20 Changed 2 months ago by skyper

Did not check the speed, lately. There was a time when it was faster to download from overpass and when the OSM-api had some hick ups. Still, you can save resources on the OSM-api server.

comment:21 Changed 2 months ago by GerdP

I fear I cannot solve this problem without a complex rewrite of the existing classes. We have lots of different classes which download a list of primitives. Some are aware of the Overpass api, some are not.
For example, "Download parent ways/relations" always uses the OSM api to find the parents but then uses Overpass to download the details.
With Overpass api I would expect a single call with something like this for node 26165886:

(
  // node with parent ways/relations
  node(id:26165886) -> .n;
  .n;way(bn);>;	
  .n;rel(bn);>>;
);
out meta;

to retrieve the node, its complete parent ways and is complete parent relations in one api call.
Instead I see multiple calls, some using Overpass, some OSM api.

Overpass query: node(id:26165886);out meta;
https://api.openstreetmap.org/api/0.6/node/26165886/ways
https://api.openstreetmap.org/api/0.6/node/26165886/relations 
https://api.openstreetmap.org/api/0.6/way/314042995/full
https://api.openstreetmap.org/api/0.6/way/208055465/full
https://api.openstreetmap.org/api/0.6/way/30503471/full
https://api.openstreetmap.org/api/0.6/way/30503468/full
Overpass query: node(id:1703187559,3861801992,1701375246,30893809,1150742558,3861801990,3861801991,3200768061,3861801989,26291274,4538502920,1150742825);out meta;

One big problem with the existing Overpass api integration is that Overpass doesn't like frequent calls from the same internet address, so if we use it we should probably prefer to create one complex query instead of many rather simple ones.

Last edited 2 months ago by GerdP (previous) (diff)

Changed 2 months ago by GerdP

Attachment: 18566-alpha.patch added

comment:22 Changed 2 months ago by GerdP

The attached patch implements the following changes:
1) When downloading relations with "Download Object" the settings for "Download relation members" are applied.
"Download relation members" means that the complete relation is wanted, including complete sub relations.
2) When updating a selection only the selected objects are updated, not their children. With the unpatched version all known members are also updated, including sub relations and their complete members down to all nodes.
Not sure if the old behaviour is wanted.
3) MultiFetchOverpassObjectReader now uses "Recurse down relations (>>)" to fetch complete sub relations. A new method setRecurseDownRelations() allows to switch off the "recurse down" for relations. The generated overpass query was optimized based on the hint https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#By_element_id

Performance hint: Try to bundle multiple id queries into one statement, i.e. instead of writing (way(3998029);way(3998240);way(4065354);); use way(id:3998029,3998240,4065354);  

4) Optimization: When overpass api is used with a selection of ways and nodes, only those nodes are downloaded which are not referenced by the downloaded ways as those are already complete. (we always recurse down when downloading ways with the overpass api)

Other problems:

  • "download members" doesn't use overpass api, it always use the OSM api with the /full suffix like this:
    https://api.openstreetmap.org/api/0.6/relation/69202/full
    
  • "download incomplete members" is typically much slower than "download members", maybe both should use the faster api call to retrieve the complete relation and later filter what's really wanted.

Edit: Forgot to mention that "https://api.openstreetmap.org/api/0.6/relation/69202/full" doens't download the members of the sub relations.

Last edited 2 months ago by GerdP (previous) (diff)

Changed 2 months ago by GerdP

Attachment: 18566-wip.patch added

Work in progress

comment:23 Changed 2 months ago by GerdP

In additon to 18566-alpha.patch the new 18566-wip.patch implements

  • new methods setRecurseDownRelations()and setRecurseDownAppended() in MultiFetchServerObjectReader to give more control to the caller and allow to get equal results from Overpass api and OSM api. Also new method appendIds() was added.
  • use of MultiFetchServerObjectReader in more places
  • fixes an EDT violation in class ChildRelationBrowser
  • "Update sellection" completes the updated elements for both APIs in the same way. Maybe I should add a new action like "Update and complete selection" so that user can decide what they want?

Changed 2 months ago by GerdP

Attachment: 18566-v1.patch added

Changed 2 months ago by GerdP

Attachment: rel-editor.PNG added

comment:24 Changed 2 months ago by GerdP

Milestone: 20.02
Summary: Download object: member of relations always downloaded[Patch] Download object: member of relations always downloaded

Various changes to improve performance and reliability of download actions. I am not yet sure what to do with the download ... buttons in the relation editer dialog:

Should the button "download button" work like "Update selected" or like "Download Object..."? In the first case all known objects would be downloaded, else just the complete Relation. The result would be the same unless the relation was changed on the server, e.g. if a member was deleted.

comment:25 Changed 2 months ago by skyper

Wow, never used that tab. Downloads nicely, without members, incomplete child relations .

Last edited 2 months ago by skyper (previous) (diff)

comment:26 Changed 2 months ago by GerdP

I also never used it. Thought you would like it ;)

comment:27 in reply to:  24 Changed 2 months ago by skyper

Replying to GerdP:

Should the button "download button" work like "Update selected" or like "Download Object..."?

Do not know the internals, but it should be the same as download members from tag/membership, relation and selection toggle dialogs' context menus

comment:28 Changed 2 months ago by GerdP

Yes, sounds reasonable.

Changed 2 months ago by GerdP

Attachment: 18566.2.patch added

overpass api behaves a bit strange. Brakets are needed for ways but not for relations.

Changed 2 months ago by GerdP

Attachment: 18566.3.patch added

comment:29 Changed 2 months ago by GerdP

18566.3.patch improves multifetch with overpass api:

  • generate a single query for all objects
  • detect missing primitives (as overpass doesn't return invisible (deleted) objects and download them with the OSM api if needed

This performs much better than a bunch of rather simple overpass queries. It uses "POST" instead of "GET" and thus avoids to hit some quotas.

comment:30 Changed 8 weeks ago by GerdP

I think there is still one big problem: When you try to upload data and a conflict is found the current code might use Overpass api to "synchronize with the server". I think this is not a good idea, it is likely that the Overpass server is not up to date in this case.

comment:31 Changed 8 weeks ago by GerdP

I need help here: I think it would be best to temporarily disallow usage of overpass API when an upload task is running. What would be the best way to do this?
My idea would be to store the current content of property OverpassDownloadReader.FOR_MULTI_FETCH, make sure it is set to false, execute the upload task and finally reset the OverpassDownloadReader.FOR_MULTI_FETCH in case it was true before.
Is that an acceptable way to do this?

comment:32 Changed 8 weeks ago by GerdP

Resolution: fixed
Status: assignedclosed

In 15811/josm:

fix #18566: Download object: member of relations always downloaded (18566.3.patch)

  • return same result when downloading objects, no matter if overpass api is enabled or not.
  • fix an EDT violation in class ChildRelationBrowser
  • let MultiFetchOverpassObjectReader create a single overpass query, no matter how many objects are requested

(gives much better reaction times).

  • use multi-fetch in ChildRelationBrowser to reduce duplicated code. Small disadvantage: When downloading multiple members of a relation the dialog doesn't update after each member. Should not matter much as the overall time for multiple members is shorter

I tried to keep the code backward compatible. Plugins reverter and undelete should continue to work.

comment:33 Changed 8 weeks ago by GerdP

In 15812/josm:

see #18566: fix findbugs UWF_UNWRITTEN_FIELD introduced with r15811

To be honest, I don't understand the synchronized (this) blocks in the code. Looks like an incomplete approach to implement a "download in background". Hope this change doesn't block anything.

comment:34 Changed 4 weeks ago by anonymous

See also #18835 (unwanted full recursive download of children relations causes JOSM to download millions nodes, and exhaust the VM memory, and finally hang JOSM: we'll see the VM reaches the maximum VM size, uses 100% CPU trying to free some memory in the garbage collector, and there will be an all blank window with just a title "Error"; there's no other choice than just killing JOSM and loosing all edits and partly solved conflicts; relauching JOSM will relaad some data in inconsistant state that will generate new edit conflicts because what was submitted successfully was not stored in the autosave file).

There's NO useful and safe case where we want a full recursive download of children relations (espacielly for country relations where it will load all subareas, or for large route relations); this blocks resolving local-only edit conflicts caused in some parent relation, that the conflict resolver will attempt to fully recursively download.

The iD editor NEVER performs any full recursion on children, it only recurses down for 1 level at most (users have to manually load subrelations with a manual request by clicking an icon in the list of members). This is the ONLY safe behavior that JOSM should have preserved.

comment:35 Changed 4 weeks ago by verdyp@…

And in the screenshot above, the button "Download All Children" should be removed.
We just need one button "Download Selected children" (the user needs to select all the listed ones if needed).
In all cases, this should just download the tags and member list of these selected children, but still NOT recurse into them.
Even if they have a "+" icon to expand their list, selecting a child with a "+" icon should only download the visible ones that are explicitly selected. If someonone wants to load more, the user has to click the "+" to expand the list (which may have incomplete members), and select these visible children explicitly. Selecting the parent relation alone should not select their children for download, even if the parent is selected: the user should also select separately these children (here again there will be no forced recursion each selected relation will be loaded separately with JUST their tags and the full list of complete or incomplete members).

comment:36 in reply to:  34 ; Changed 4 weeks ago by GerdP

Replying to anonym:

There's NO useful and safe case where we want a full recursive download of children relations (espacielly for country relations where it will load all subareas, or for large route relations); this blocks resolving local-only edit conflicts caused in some parent relation, that the conflict resolver will attempt to fully recursively download.

I think there are only a few relations which cause trouble.

The iD editor NEVER performs any full recursion on children, it only recurses down for 1 level at most (users have to manually load subrelations with a manual request by clicking an icon in the list of members). This is the ONLY safe behavior that JOSM should have preserved.

Well, I don't mind to change this. Before r15811 the full recursive download was only performed with overpass api enabled.
I can change this so that both APIs recurse only one level of down for sub relations.

comment:37 in reply to:  35 Changed 4 weeks ago by GerdP

Replying to verdyp@…:

And in the screenshot above, the button "Download All Children" should be removed.
We just need one button "Download Selected children" (the user needs to select all the listed ones if needed).
In all cases, this should just download the tags and member list of these selected children, but still NOT recurse into them.
Even if they have a "+" icon to expand their list, selecting a child with a "+" icon should only download the visible ones that are explicitly selected. If someonone wants to load more, the user has to click the "+" to expand the list (which may have incomplete members), and select these visible children explicitly. Selecting the parent relation alone should not select their children for download, even if the parent is selected: the user should also select separately these children (here again there will be no forced recursion each selected relation will be loaded separately with JUST their tags and the full list of complete or incomplete members).

I think the changes in r15811 did not change the behaviour of the dialog. Please open a new ticket to suggest improvements for it.

comment:38 in reply to:  36 Changed 4 weeks ago by skyper

Replying to GerdP:

Well, I don't mind to change this. Before r15811 the full recursive download was only performed with overpass api enabled.
I can change this so that both APIs recurse only one level of down for sub relations.

+1

comment:39 Changed 4 weeks ago by GerdP

In r16008:

This is a partly revert of r15811 which changed the meaning of "download members" to "download complete members". Now - as before r15811 - only way and node members are completed. This also applies when overpass api is used. For some relations, e.g. 1311341 (the boundary of Spain), the full recursion has to download thousands of sub relations, which is likely to fail.
Cancel button did not work while relation members were downloaded.

comment:40 in reply to:  25 Changed 3 weeks ago by skyper

Replying to skyper:

Wow, never used that tab. Downloads nicely, without members, incomplete child relations .

Sadly, the feature got lost. In child tab, clicking at a incomplete relation did only download the relation but now it is the same as clicking on the "download selected children" button below and all children are downloaded.

comment:41 Changed 3 weeks ago by GerdP

What JOSM version? Do you click on the plus sign or somewhere else? Single click or double click?

comment:42 Changed 3 weeks ago by skyper

latest, click on the plus sign to extend. Single click on name does nothing except selecting and double click is the same as single click on plus, downloading all members.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-03-10 00:04:08 +0100 (Tue, 10 Mar 2020)
Revision:16106
Build-Date:2020-03-10 02:30:53
URL:https://josm.openstreetmap.de/svn/trunk

comment:43 Changed 3 weeks ago by GerdP

I just tried with r16107 and relation 7159297 and for me it downloads only one level. The generated API call for relation 69178 is

GET https://api.openstreetmap.org/api/0.6/relation/69178/full
Last edited 3 weeks ago by GerdP (previous) (diff)

comment:44 Changed 3 weeks ago by GerdP

I get the same result with r15810.

comment:45 in reply to:  44 Changed 2 weeks ago by skyper

Replying to GerdP:

I get the same result with r15810.

Strange. I had in mind that two month ago clicking on the plus to unfold did only download the relation without members but probably I had one more level of nesting and the relation had only a few relations as members which were downloaded.
As selecting in "Tags/Members" tab and "downloading selected" does the job, I am fine. Sorry, for the noise.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.