Fixed the update process by also complete places before updating

[hendrikmoree]

Currently updates aren't correctly processed. Places are incomplete, and a lot warning are given because a place can be deleted and also tried to updateOrCreate afterwards.

This fixes both issues.

[hendrikmoree]

https://github.com/komoot/photon/pull/369/files#diff-e9d2005193ee859b90ddc8de7035194eR53 fixes #337

[lonvia]

Looking at this again, the different cases need a bit more consideration.

For 'case 100' (object was deleted), export.getByPlaceId() shouldn't even be called because we already know that the placeID won't exist anymore. (That was wrong before as well but is worth fixing while changing this code.)

More importantly, for the 'case 2', the code does not catch when a housenumber gets deleted from the batch of housenumbers for that placeID because the code only updates the ones that exist after the update. The most simple solution here is to always call a delete with a subsequent create for case 2. I don't know what the performance impact of such a solution would be.

In any case, the delete() in 'case 2' should be moved out of the loop. isUsefulForIndex() evaluates always to the same value for the elements of updatesDocs and calling delete() multiple times for the same placeID is inefficient, if not harmful.

[hendrikmoree]

I've fixed the case '100' issue.

I'm not really sure how to fix the other case '2' issue. We're already running some months with this code, so it doesn't seems to be harmful. This PR looks quite important to me, because the update process is really broken now, if anyone now how to fix it, it would be great.

[simonpoole]

@hendrikmoree I think @lonvia was thinking about something along

                    final List<PhotonDoc> updatedDocs = exporter.getByPlaceId(place.getPlaceId());
                    if (indexdStatus == 2 && updatedDocs.size() > 1) {
                        updater.delete(placeId);
                    }
                    boolean wasUseful = false;
                    for (PhotonDoc updatedDoc : updatedDocs) {
                        switch (indexdStatus) {
                        case 1:
                            if (updatedDoc.isUsefulForIndex()) {
                                updater.create(updatedDoc);
                            }
                            break;
                        case 2:
                            if (updatedDoc.isUsefulForIndex()) {
                                updater.updateOrCreate(updatedDoc);
                                wasUseful = true;
                            }
                            break;
                        default:
                            LOGGER.error(String.format("Unknown index status %d", indexdStatus));
                            break;
                        }
                    }
                    if (indexdStatus == 2 && !wasUseful) {
                        updater.delete(placeId);
                    }

This is better, but still has a couple of cases that are not covered, for example that the object now only has 1 house number and used to have multiple.

To make it really correct you would need to remove the updatedDocs.size() > 1 test, which means that every update would first delete all documents with the referenced place id,

[simonpoole]

@lonvia @karussell my question here would be: do we really need the added complexity caused by expanding Nominatim objects with multiple house numbers in to multiple documents on import in the first place?

[lonvia]

They are needed to support address interpolations.

[simonpoole]

There are two issues that are not addressed by my rough code suggestion

• interpolations were not being updated at all (this is simply an omission in the current code)
• you can't call ...updateOrCreate after calling delete as, updateOrCreate checks if the documents exist to determine if to call update or create when submitting the action for batch execution, and as a result update will incorrectly be called instead of create.

I now have (I believe :-)) working code for this that addresses both issues. I'll refine it a bit more (are there any tests for the updater?) and then create a new PR if nobody disagrees.

[lonvia]

Superseded by #382. Closing here.

Thanks @hendrikmoree for the initial version.