Only fetch newest element from API if necessary

[Helium314]

fixes #4078

I did not yet test whether the modified upload actually works, need to go for a walk first and find something to change.

Related tests are not building, and I failed at making them work, so help would be appreciated.

[matkoniecz]

Related tests are not building

Also on master?

[Helium314]

No, this is because of my changes

[Helium314]

Anyway I got it now. Needed to learn a bit more about how to use Mockito, and ran into trouble because any() apparently doesn't actually mean any... that's why there is anyLong()

[westnordost]

Hmm, I think that this approach won't work with e.g. SplitWayAction.

Note the interface of ElementEditAction:

fun createUpdates(
    originalElement: Element,
    element: Element?,
    mapDataRepository: MapDataRepository,
    idProvider: ElementIdProvider
)

It expects a MapDataRepository, MapDataApi just implements this interface. But MapDataWithEditsSource does, too. (And MapDataController could, too)

https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/split_way/SplitWayAction.kt#L45

SplitWayAction will fetch the elements it needs to perform the split from the MapDataRepository by itself. (Right now I am asking myself why even the element: Element? is passed, after all the edit action could just fetch that by itself)

My thinking is, wouldn't it be more straightforward (also in implementation) if on 1st try, the uploader would just pass a MapDataController to edit.action.createUpdates, and if that fails due to an upload conflict, pass a MapDataApi instead?

(Though, thinking about this more, this is where we would stumble upon an ugly performance workaround we made earlier - that StreetComplete discards all route relations right after download. This decision would need to be reversed or something)

[Helium314]

Could we just always use MapDataApi if edit.action is SplitWayAction?

Right now I am asking myself why even the element: Element? is passed, after all the edit action could just fetch that by itself

If the edit action fetches the element, we may need to fetch it again in the uploader on a ConflictException, to know whether the changeset is closed or the element changed. Though this was not the case before this PR...

[westnordost]

Could we just always use MapDataApi if edit.action is SplitWayAction?

You mean, go with my suggestion but make an exception for SplitWayAction in order to not revert the throwing-away-of-the-route-relations? It seems to me that in its result, that solution would then differ not that much from the current solution.

What I am thinking is whether that workaround to throw away all route relations on download (see IGNORED_RELATION_TYPES) could be revisited, especially when we can get the persisting of the relations to be done in the background thanks to your cache implementation.

[Helium314]

What I am thinking is whether that workaround to throw away all route relations on download (see IGNORED_RELATION_TYPES) could be revisited, especially when we can get the persisting of the relations to be done in the background thanks to your cache implementation.

I think persisting wasn't the problem, but rather that all elements of a relation are fetched on ... not sure, maybe getting surrounding map data for creating quests? This is then also cached, but might still be slow the first time (anyway, I'll test this).

[westnordost]

getting surrounding map data for creating quests

I remember: Mabye it was about showing the surrounding map data for certain quests? Because a really large route relation will be close to just about any element (especially with the old bbox-logic), so a huge relation would need to be fetched from the database everytime you click quests that show surrounding data. Was that it, perhaps?

[Helium314]

See https://github.com/streetcomplete/StreetComplete/issues/2803#issuecomment-827639718 and below. I had identified the the db query for getting members of (huge) relations as the slow part.

[Helium314]

I just tested how fast things are when all relations are in the database. As expected getMapDataWithGeometry is still very fast when everything is cached, but loading an area with many huge relations into cache (same area as in #2803) loading from db may even take more than 70 s.

[westnordost]

Okay, so either my approach should be taken but a hardcoded exception to this behavior (i.e. always use remote data) must be added for the SplitWayAction specifically (and any other future actions that may edit route etc. relations).

Or, we stay with your solution. However, as stated before, I find it not so clean because always a mapDataApi is passed to the action. So any other action that actually accesses the mapDataRepository (which is currently always a MapDataApi) to create the changes won't work with that solution (currently, excluding SplitWayAction, that's just DeletePoiNodeAction).

That there could be other action types in the future may seem abstract, but just look at the ElementEditActions that already exist - it is really easy to add other types of actions. Even the super-complex splitting of ways is just 300 lines of code. For example, there could be a MovePoiNodeAction (#3502), a AddEntrancesToBuildingOutlineAction (or more generic) etc.. It would be great if this PR was future proof in the way that such possible extensions to the functionality of this app were covered.

[Helium314]

I agree that passing mapDataApi and the local element is not good.

How about:

• don't pass any element
• remove element from ElementEditAction.createUpdates and use mapDataRepository.get<Node/Way/Relation>
• have a list of actions that are allowed to use local elements (or put it in a boolean for each action)

And do something like:

val localChanges by lazy { edit.action.createUpdates(edit.originalElement, mapDataController, idProvider) }
val remoteChanges by lazy { edit.action.createUpdates(edit.originalElement, mapDataApi, idProvider) }
val changes = if (edit.action.isAllowedToUseLocalChanges())
        localChanges
    else
        remoteChanges

return try {
    uploadChanges(edit, changes, false)
} catch (e: ConflictException) {
    // either changeset was closed, or element modified, or local element was cleaned from db
    if (changes === localChanges) {
        // anything of the 3 may be the problem, try again with remote changes
        try {
            uploadChanges(edit, remoteChanges, false)
        } catch (e: ConflictException) {
            // probably changeset closed
            uploadChanges(edit, remoteChanges, true)
        }
    } else {
        // probably changeset closed
        uploadChanges(edit, remoteChanges, true)
    }
}

[westnordost]

don't pass any element remove element from ElementEditAction.createUpdates and use mapDataRepository.get<Node/Way/Relation>

Yes, this seems reasonable. I wonder if there was a specific reason why I did not do it like this from the start. So, if there was no reason, then I am all for it. Makes it simpler.

have a list of actions that are allowed to use local elements (or put it in a boolean for each action)

No, because whether or not an action is allowed to use local data is not a property of that action. The action shouldn't (need to) know that due to some performance consideration, StreetComplete as a whole drops certain relations on download before even persisting them. Ideally, which action types should not be allowed to use local changes (I'd like to turn that around) would be defined at the same place where it is defined to throw away certain relation types - and be it just as a public constant.

[Helium314]

I wonder if there was a specific reason why I did not do it like this from the start.

I did't find any reason in other actions, or in the history... but actually this change isn't really necessary, I can also keep the element in there and fetch it in the uploader.

So I will put some list next to IGNORED_RELATION_TYPES. My motivation for using a list of allowed actions was to be on the safe side in case this list is not considered when adding a new action. But I don't really care, so I'll use the list of not allowed actions.

[Helium314]

So, if there was no reason, then I am all for it. Makes it simpler.

I did not (yet) change this. If you're ok with the way it is now, I will use/test it for a while.

Only after some testing I'll remove passing the element / fetching it in the action and test / use it for a while again.

[Helium314]

I will use/test it for a while

And I did, including some logging.

• Normal upload without conflicts: works as expected
• Conflict when uploading a SplitWayAction (way split in other editor): upload using remote changes fails, is tried again with new changeset, fails again and edit is dropped (no change from old behavior, as it's using changes on remote element)
• Conflict when uploading UpdateElementsTagsAction (added node to the way in other editor): upload using local changes fails, is tried again using remote changes, and works
• Conflict when uploading DeletePoiNodeAction (deleted node in other editor): upload using local changes fails, is tried again using remote changes, then tried again with new changeset, and then edit is dropped because "element deleted"
• Conflict when creating local changes (local element was deleted, db cleared): upload using remote changes works

[westnordost]

Did the upload feel faster by the way? For the changelog it would be nice to know how much of an improvement this really is (roughly)

[Helium314]

I didn't really notice the difference... though I did some logging, and time for uploading was reduced by somthing like 80-100 ms on average.

But I usually have auto-upload off, so time between consecutive uploads is mostly limited by rebuildLocalChanges, which is slower than upload if there are more than ~20 edits waiting (depends on phone performance).