Don't copy existing media files when updating media only #6946
Conversation
seadowg
force-pushed
the
form-download-types
branch
3 times, most recently
from
95899e8 to
b09632e
Compare
seadowg
changed the title
ServerFormDetails
| } | ||
| } | ||
|
|
||
| public interface ServerFormsDetailsFetcher { |
There was a problem hiding this comment.
We could simplify this by converting to Kotlin, but I wanted to try and limit the change set here!
collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } else if (thisFormAlreadyDownloaded) { | ||
| if (listItem.hash == null) { | ||
| ServerFormDetails.Type.OnDevice |
There was a problem hiding this comment.
Hmm, I'm not sure about this. If existingForm == null so that we end up here in the else block, that means the form is not on the device. If hash is null we should rather assume it's a new one.
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.
|
|
||
| if (existingFileHash.contentEquals(mediaFile.hash)) { | ||
| copyFileToDirectory(existingFile, tempMediaDir) | ||
| if (formToDownload.type != ServerFormDetails.Type.UpdatedMedia) { |
There was a problem hiding this comment.
Shouldn't this be or != Type.OnDevice?
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.
| if (existingFile != null) { | ||
| val existingFileHash = existingFile.getMd5Hash() | ||
|
|
||
| if (existingFileHash.contentEquals(mediaFile.hash)) { |
There was a problem hiding this comment.
Can't we update searchForExistingMediaFile to check the hash as well so that existingFile is null if the file exists but has been updated? That would simplify the structure here a little bit.
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.
| val type: Type | ||
| ) : Serializable { | ||
|
|
||
| @Deprecated(message = "Use primary constructor instead") |
There was a problem hiding this comment.
Can we get rid of it if it is only used in tests?
There was a problem hiding this comment.
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.
collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDetails.kt
Outdated
Show resolved
Hide resolved
...c/test/java/org/odk/collect/android/formmanagement/FetchFormDetailsServerFormUseCasesTest.kt
Show resolved
Hide resolved
These test file names are pretty long, but they allow IntelliJ to navigate between the file and the test.
seadowg
force-pushed
the
form-download-types
branch
from
b09632e to
f9db6d5
Compare
2 participants
Closes #6843
Why is this the best possible solution? Were any other approaches considered?
To make things (hopefully) simpler to understand in the download code, I've introduced a new
Typeenum toServerFormDetailsthat replaces the existing booleans forisNotOnDeviceandisUpdated. This allows our download code to make optimizations based on the download scenario explicitly rather than having to infer it from several pieces of information.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
There should be no change in behaviour here - the only thing that should change is the performance of updating an entity list for a form that has many media files (and none have changed).
Do we need any specific form for testing your changes? If so, please attach one.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest