#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?
- open download object dialog
- choose "relation" as object type and enter id of relation
- 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)
Change History (55)
comment:1 by , 5 years ago
Keywords: | regression added |
---|
follow-up: 3 comment:2 by , 5 years ago
comment:3 by , 5 years ago
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 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → needinfo |
I don't reproduce. Please share object id, screenshots and .osm data layer.
comment:6 by , 5 years ago
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 by , 5 years ago
Cc: | 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
52 52 super(tr("Download objects"), progressMonitor, layer); 53 53 this.ids = ids; 54 54 setZoom(true); 55 setDownloadRelations( true, fullRelation);55 setDownloadRelations(false, fullRelation); 56 56 } 57 57 58 58 @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 by , 5 years ago
Probably not a regression in r10129. I tested with josm-snapshot-10123.jar and got the same result.
comment:9 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | needinfo → new |
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.
comment:11 by , 5 years ago
So we have two problems:
- relations as members are always downloaded.
- if
use overpass to download objects
is enabled the checkbox is not used.
follow-up: 17 comment:12 by , 5 years ago
comment:13 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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.
follow-up: 15 comment:14 by , 5 years ago
@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"
comment:15 by , 5 years ago
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 by , 5 years ago
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)
follow-up: 18 comment:17 by , 5 years ago
comment:18 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
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.
by , 5 years ago
Attachment: | 18566-alpha.patch added |
---|
comment:22 by , 5 years ago
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.
comment:23 by , 5 years ago
In additon to 18566-alpha.patch the new 18566-wip.patch implements
- new methods
setRecurseDownRelations()
andsetRecurseDownAppended()
inMultiFetchServerObjectReader
to give more control to the caller and allow to get equal results from Overpass api and OSM api. Also new methodappendIds()
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?
by , 5 years ago
Attachment: | 18566-v1.patch added |
---|
by , 5 years ago
Attachment: | rel-editor.PNG added |
---|
follow-up: 27 comment:24 by , 5 years ago
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.
follow-up: 40 comment:25 by , 5 years ago
Wow, never used that tab. Downloads nicely, without members, incomplete child relations .
comment:27 by , 5 years ago
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
by , 5 years ago
Attachment: | 18566.2.patch added |
---|
overpass api behaves a bit strange. Brakets are needed for ways but not for relations.
by , 5 years ago
Attachment: | 18566.3.patch added |
---|
comment:29 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
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?
follow-up: 36 comment:34 by , 5 years ago
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.
follow-up: 37 comment:35 by , 5 years ago
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).
follow-up: 38 comment:36 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
follow-up: 46 comment:39 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
What JOSM version? Do you click on the plus sign or somewhere else? Single click or double click?
comment:42 by , 5 years ago
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 by , 5 years ago
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
comment:45 by , 5 years ago
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.
comment:46 by , 5 years ago
Replying to GerdP:
Replying to 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
Yes, sounds reasonable.
With r16008 we are out of sync.
comment:48 by , 5 years ago
Replying to GerdP:
Out of sync as described in #19124?
Oh, sorry, oversaw your comment:
Similar, out of sync between the behavior of "download (incomplete) members" in relation list panel and "download (selected) members" in relation editor. Anyway, I got used to it and only use the panel for relations.
If run with remote control it even downloads the members of child relations.