eivindveg / HotSUploader

JavaFX-based Replay Uploader for Heroes of the Storm
Apache License 2.0
185 stars 36 forks source link

Replays are not purged on startup after deletion #115

Closed eivindveg closed 8 years ago

eivindveg commented 8 years ago

ReplayFiles are not deleted from the database when not existing anymore.

The cause of this is believed to be 463f9749bc5e390f2cfab7687a985affbd4d52e4 or 36ec68c7af60348540e972dd704fd818bfa85120

zhedar commented 8 years ago

I can check on this tonight, when I'm home again. It should happen in https://github.com/eivindveg/HotSUploader/blob/develop/src/main/java/ninja/eivind/hotsreplayuploader/repositories/OrmLiteFileRepository.java line 119:

 else if(!replayFile.getFile().exists())
     deleteReplay(replayFile);
eivindveg commented 8 years ago

Ah. That would only delete replays missing from an already known location. I uncovered this by moving my documents folder from my SSD to my HDD without symlinking(the location changed). Would it make more sense to invert the selection?

Ie instead of deleting the database entry for each file not in the database, we delete the database entry for each entry not in the file list?

EDIT: The basic idea will be the same, but the database list is assumed to be the more complete set.

zhedar commented 8 years ago

Well that should delete all replays, which aren't at the saved location anymore, so moving them should delete them as well. But I'm currently not sure, how exactly the path is saved (i.e. filename only or absolute path) in the database.

Maybe I'm missing something here, but don't we already do the 2nd and not the 1st? If the file doesn't exist in the file system, it will be removed from the db. Or what else did you mean with file list?

eivindveg commented 8 years ago

Currently it's "if the existing file is in the database and the file does not exist, delete it". I'm proposing "if the file from the database is not in the file list, delete it"

eivindveg commented 8 years ago

Actually, reviewing that first statement actually uncovers the bug; a missing file cannot be deleted as it is not part of the comparison. We need to expand the check to two different iterations: one for creating entries for new files, and one for deleting missing files.

zhedar commented 8 years ago

Currently it's "if the existing file is in the database and the file does not exist, delete it". I'm proposing "if the file from the database is not in the file list, delete it"

That could work, if we read the existing files first, maybe faster then looking it up again. But won't fix anything yet.

Actually, reviewing that first statement actually uncovers the bug; a missing file cannot be deleted as it is not part of the comparison.

But a missing file will still be in the list obtained from the db, which it should be removed from, if absent. That's how it should work.

eivindveg commented 8 years ago

I believe ReplayFile#fromDirectory, which is the iterated collection is local from disk though, which is why the current check cannot succeed.

zhedar commented 8 years ago

You're right.

public Void call() throws Exception {
    for(ReplayFile replayFile : replayFiles) {
         if(!fromDb.contains(replayFile))
              createReplay(replayFile);
         else if(!replayFile.getFile().exists())
              deleteReplay(replayFile);
    }

should really be split into two loops, the 2nd iterating over fromDb.