Don't copy existing media files when updating media only #6946
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import org.odk.collect.android.formmanagement.download.FormDownloadException | |
| import org.odk.collect.android.formmanagement.download.FormDownloader | ||
| import org.odk.collect.android.instancemanagement.autosend.getLastUpdated | ||
| import org.odk.collect.android.utilities.FileUtils | ||
| import org.odk.collect.android.utilities.FormUtils | ||
| import org.odk.collect.async.OngoingWorkListener | ||
| import org.odk.collect.entities.LocalEntityUseCases | ||
| import org.odk.collect.entities.server.EntitySource | ||
|
|
@@ -14,13 +15,75 @@ import org.odk.collect.forms.Form | |
| import org.odk.collect.forms.FormSource | ||
| import org.odk.collect.forms.FormSourceException | ||
| import org.odk.collect.forms.FormsRepository | ||
| import org.odk.collect.forms.ManifestFile | ||
| import org.odk.collect.forms.MediaFile | ||
| import org.odk.collect.shared.strings.Md5.getMd5Hash | ||
| import timber.log.Timber | ||
| import java.io.File | ||
| import java.io.IOException | ||
|
|
||
| object ServerFormUseCases { | ||
|
|
||
| @JvmStatic | ||
| @Throws(FormSourceException::class) | ||
| fun fetchFormDetails( | ||
| formsRepository: FormsRepository, | ||
| formSource: FormSource | ||
| ): List<ServerFormDetails> { | ||
| val formList = formSource.fetchFormList() | ||
| return formList.map { listItem -> | ||
| val manifestFile = listItem.manifestURL?.let { | ||
| getManifestFile(formSource, it) | ||
| } | ||
|
|
||
| val forms = formsRepository.getAllNotDeletedByFormId(listItem.formID) | ||
| val thisFormAlreadyDownloaded = forms.isNotEmpty() | ||
|
|
||
| val formHash = listItem.hash | ||
| val existingForm = if (formHash != null) { | ||
| formsRepository.getOneByMd5Hash(formHash) | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| val areNewerMediaFilesAvailable = if (existingForm != null && manifestFile != null) { | ||
| areNewerMediaFilesAvailable(existingForm, manifestFile.mediaFiles) | ||
| } else { | ||
| false | ||
| } | ||
|
|
||
| val type = if (existingForm != null) { | ||
| if (existingForm.isDeleted) { | ||
| ServerFormDetails.Type.New | ||
| } else if (areNewerMediaFilesAvailable) { | ||
| ServerFormDetails.Type.UpdatedMedia | ||
| } else { | ||
| ServerFormDetails.Type.OnDevice | ||
| } | ||
| } else if (thisFormAlreadyDownloaded) { | ||
| if (listItem.hash == null) { | ||
| ServerFormDetails.Type.OnDevice | ||
|
Member
There was a problem hiding this comment. Hmm, I'm not sure about this. If
Member
Author
There was a problem hiding this comment. This isn't a change in behaviour right? Maybe I'm missing something, but this how it worked before, and it's also not something that should happen with a "well-behaved" server. It might not be quite right, but we'd need to carefully consider changing any of the behaviour here as it could break existing "badly behaved" servers. |
||
| } else if (forms.any { it.version == listItem.version }) { | ||
| ServerFormDetails.Type.UpdatedHash | ||
| } else { | ||
| ServerFormDetails.Type.UpdatedVersion | ||
| } | ||
| } else { | ||
| ServerFormDetails.Type.New | ||
| } | ||
|
|
||
| ServerFormDetails( | ||
| listItem.name, | ||
| listItem.downloadURL, | ||
| listItem.formID, | ||
| listItem.version, | ||
| listItem.hash, | ||
| manifestFile, | ||
| type | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| fun downloadForms( | ||
| forms: List<ServerFormDetails>, | ||
| formDownloader: FormDownloader, | ||
|
|
@@ -156,7 +219,9 @@ object ServerFormUseCases { | |
| val existingFileHash = existingFile.getMd5Hash() | ||
|
|
||
| if (existingFileHash.contentEquals(mediaFile.hash)) { | ||
|
Member
There was a problem hiding this comment. Can't we update
Member
Author
There was a problem hiding this comment. Ach I thought we could for a sec, but we need to check here for the case where we download a file we think has a new hash, but it doesn't below here. |
||
| if (formToDownload.type != ServerFormDetails.Type.UpdatedMedia) { | ||
|
Member
There was a problem hiding this comment. Shouldn't this be or
Member
Author
There was a problem hiding this comment. The only case we don't want to copy existing media files to the "sandbox" directory is when we're doing a media-only update as the existing media files are already where they need to be. In other update cases, the files will be needed for the new version's media directory. There's potentially an additional optimization where we copy existing media files from their original version to the new version directory (and never to the sandbox directory), but we didn't need to do that to hit our benchmark targets here. |
||
| copyFileToDirectory(existingFile, tempMediaDir) | ||
| } | ||
| } else { | ||
| downloadMediaFile( | ||
| formSource, | ||
|
|
@@ -233,6 +298,32 @@ object ServerFormUseCases { | |
| null | ||
| } | ||
| } | ||
|
|
||
| private fun getManifestFile(formSource: FormSource, manifestUrl: String): ManifestFile? { | ||
| return try { | ||
| formSource.fetchManifest(manifestUrl) | ||
| } catch (formSourceException: FormSourceException) { | ||
| Timber.w(formSourceException) | ||
| null | ||
| } | ||
| } | ||
|
|
||
| private fun areNewerMediaFilesAvailable( | ||
| existingForm: Form, | ||
| newMediaFiles: List<MediaFile> | ||
| ): Boolean { | ||
| if (newMediaFiles.isEmpty()) { | ||
| return false | ||
| } | ||
|
|
||
| val localMediaHashes = FormUtils.getMediaFiles(existingForm) | ||
| .map { it.getMd5Hash() } | ||
| .toSet() | ||
|
|
||
| return newMediaFiles.any { | ||
| !it.filename.endsWith(".zip") && it.hash !in localMediaHashes | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class EntityListUpdateException(cause: Throwable) : Exception(cause) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of it if it is only used in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 51 uses, so it'll be a real slog to change all at once and a lot of noise for the PR. I'd thought it made sense to just change incrementally.