Skip to content

Commit

Permalink
Merge pull request #5439 from seadowg/hash-fix
Browse files Browse the repository at this point in the history
Stop showing "Updated at" after redownloading identical media files
  • Loading branch information
seadowg committed Feb 15, 2023
2 parents 569e3d2 + be9ad72 commit faf9891
Show file tree
Hide file tree
Showing 12 changed files with 225 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@
import org.junit.rules.RuleChain;
import org.junit.runner.RunWith;
import org.odk.collect.android.R;
import org.odk.collect.android.support.rules.CollectTestRule;
import org.odk.collect.android.support.TestDependencies;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.android.support.TranslatedStringBuilder;
import org.odk.collect.android.support.pages.MainMenuPage;
import org.odk.collect.android.support.rules.CollectTestRule;
import org.odk.collect.android.support.rules.TestRuleChain;

import java.util.Arrays;

@RunWith(AndroidJUnit4.class)
public class BadServerTest {


private final CollectTestRule rule = new CollectTestRule(false);
private final TestDependencies testDependencies = new TestDependencies();

Expand Down Expand Up @@ -63,4 +63,26 @@ public void whenMediaFileHasMissingPrefix_showsAsUpdated() {
.clickGetBlankForm()
.assertText(R.string.newer_version_of_a_form_info);
}

@Test
/*
A server that doesn't return hashes based on the md5 of the file for media files
would fool Collect into thinking there was a new one each time. Collect should still redownload
the file in this case (there's nothing else it can do), but it should only identify the form
as being updated if the file actually changed.
*/
public void whenMediaFileHasUnstableHash_butIsIdentical_doesNotShowAsUpdatedAfterRedownload() {
testDependencies.server.returnRandomMediaFileHash();
testDependencies.server.addForm("One Question", "one_question", "1", "one-question.xml", Arrays.asList("fruits.csv"));

rule.withProject(testDependencies.server.getURL())
.copyForm("one-question.xml", Arrays.asList("fruits.csv"), false, testDependencies.server.getHostName())
.clickGetBlankForm()
.assertText(R.string.newer_version_of_a_form_info)
.clickGetSelected()
.clickOKOnDialog(new MainMenuPage())
.clickFillBlankForm()
.assertTextThatContainsDoesNoExist("Updated on")
.assertTextThatContainsExists("Added on");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.odk.collect.android.openrosa.OpenRosaConstants;
import org.odk.collect.android.openrosa.OpenRosaHttpInterface;
import org.odk.collect.shared.strings.Md5;
import org.odk.collect.shared.strings.RandomString;

import java.io.ByteArrayInputStream;
import java.io.File;
Expand All @@ -43,6 +44,7 @@ public class StubOpenRosaServer implements OpenRosaHttpInterface {
private boolean fetchingFormsError;
private boolean noHashInFormList;
private boolean noHashPrefixInMediaFiles;
private boolean randomHash;

@NonNull
@Override
Expand Down Expand Up @@ -150,6 +152,10 @@ public void removeMediaFileHashPrefix() {
noHashPrefixInMediaFiles = true;
}

public void returnRandomMediaFileHash() {
randomHash = true;
}

public String getURL() {
return "https://" + HOST;
}
Expand Down Expand Up @@ -233,7 +239,13 @@ private InputStream getManifestResponse(@NonNull URI uri) throws IOException {

for (String mediaFile : xformItem.getMediaFiles()) {
AssetManager assetManager = InstrumentationRegistry.getInstrumentation().getContext().getAssets();
String mediaFileHash = Md5.getMd5Hash(assetManager.open("media/" + mediaFile));
String mediaFileHash;

if (randomHash) {
mediaFileHash = RandomString.randomString(8);
} else {
mediaFileHash = Md5.getMd5Hash(assetManager.open("media/" + mediaFile));
}

stringBuilder
.append("<mediaFile>")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ abstract class Page<T : Page<T>> {
return this as T
}

@JvmOverloads
fun assertTextThatContainsExists(text: String, index: Int = 0): T {
onView(
withIndex(
Expand All @@ -123,6 +124,21 @@ abstract class Page<T : Page<T>> {
return this as T
}

@JvmOverloads
fun assertTextThatContainsDoesNoExist(text: String, index: Int = 0): T {
onView(
withIndex(
withText(
containsString(
text
)
),
index
)
).check(doesNotExist())
return this as T
}

fun checkIsTranslationDisplayed(vararg text: String?): T {
for (s in text) {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,40 @@ class FormMediaDownloader(
@Throws(IOException::class, FormSourceException::class, InterruptedException::class)
fun download(
formToDownload: ServerFormDetails,
files: List<MediaFile>,
tempMediaPath: String,
tempDir: File,
stateListener: OngoingWorkListener
): Boolean {
var atLeastOneNewMediaFileDetected = false
val tempMediaDir = File(tempMediaPath).also { it.mkdir() }

files.forEachIndexed { i, mediaFile ->
formToDownload.manifest!!.mediaFiles.forEachIndexed { i, mediaFile ->
stateListener.progressUpdate(i + 1)

val tempMediaFile = File(tempMediaDir, mediaFile.filename)

searchForExistingMediaFile(formToDownload, mediaFile).let {
val existingFile = searchForExistingMediaFile(formToDownload, mediaFile)
existingFile.let {
if (it != null) {
copyFile(it, tempMediaFile)
if (getMd5Hash(it).contentEquals(mediaFile.hash)) {
copyFile(it, tempMediaFile)
} else {
val existingFileHash = getMd5Hash(it)
val file = formSource.fetchMediaFile(mediaFile.downloadUrl)
interuptablyWriteFile(file, tempMediaFile, tempDir, stateListener)

if (!getMd5Hash(tempMediaFile).contentEquals(existingFileHash)) {
atLeastOneNewMediaFileDetected = true
}
}
} else {
atLeastOneNewMediaFileDetected = true
val file = formSource.fetchMediaFile(mediaFile.downloadUrl)
interuptablyWriteFile(file, tempMediaFile, tempDir, stateListener)
atLeastOneNewMediaFileDetected = true
}
}
}

return atLeastOneNewMediaFileDetected
}

Expand All @@ -51,12 +62,12 @@ class FormMediaDownloader(
mediaFile: MediaFile
): File? {
val allFormVersions = formsRepository.getAllByFormId(formToDownload.formId)
return allFormVersions.map { form: Form ->
return allFormVersions.sortedByDescending {
it.date
}.map { form: Form ->
File(form.formMediaPath, mediaFile.filename)
}.firstOrNull { file: File ->
val currentFileHash = getMd5Hash(file)
val downloadFileHash = mediaFile.hash
file.exists() && currentFileHash.contentEquals(downloadFileHash)
file.exists()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe
// download media files if there are any
if (fd.getManifest() != null && !fd.getManifest().getMediaFiles().isEmpty()) {
FormMediaDownloader mediaDownloader = new FormMediaDownloader(formsRepository, formSource);
newAttachmentsDetected = mediaDownloader.download(fd, fd.getManifest().getMediaFiles(), tempMediaPath, tempDir, stateListener);
newAttachmentsDetected = mediaDownloader.download(fd, tempMediaPath, tempDir, stateListener);
}
} catch (FormDownloadException.DownloadingInterrupted | InterruptedException e) {
Timber.i(e);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package org.odk.collect.android.formmanagement

import org.hamcrest.MatcherAssert.assertThat
import org.hamcrest.Matchers.equalTo
import org.junit.Test
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.odk.collect.forms.FormSource
import org.odk.collect.forms.ManifestFile
import org.odk.collect.forms.MediaFile
import org.odk.collect.formstest.FormFixtures
import org.odk.collect.formstest.InMemFormsRepository
import org.odk.collect.shared.TempFiles
import org.odk.collect.shared.strings.Md5
import java.io.File

class FormMediaDownloaderTest {

@Test
fun `returns false when there is an existing copy of a media file and an older one`() {
// Save forms
val formsRepository = InMemFormsRepository()
val form1 = FormFixtures.form(
version = "1",
date = 1,
mediaFiles = listOf(Pair("file", "old"))
)
formsRepository.save(form1)

val form2 = FormFixtures.form(
version = "2",
date = 2,
mediaFiles = listOf(Pair("file", "existing"))
)
formsRepository.save(form2)

// Set up same media file on server
val existingMediaFileHash = Md5.getMd5Hash(File(form2.formMediaPath, "file"))!!
val mediaFile = MediaFile("file", existingMediaFileHash, "downloadUrl")
val manifestFile = ManifestFile(null, listOf(mediaFile))
val serverFormDetails =
ServerFormDetails(null, null, "formId", "3", null, false, true, manifestFile)
val formSource = mock<FormSource> {
on { fetchMediaFile(mediaFile.downloadUrl) } doReturn "existing".toByteArray()
.inputStream()
}

val formMediaDownloader = FormMediaDownloader(formsRepository, formSource)
val result = formMediaDownloader.download(
serverFormDetails,
File(TempFiles.createTempDir(), "temp").absolutePath,
TempFiles.createTempDir(),
mock()
)

assertThat(result, equalTo(false))
}

@Test
fun `returns false when there is an existing copy of a media file and an older one and server hash doesn't match existing copy`() {
// Save forms
val formsRepository = InMemFormsRepository()
val form1 = FormFixtures.form(
version = "1",
date = 1,
mediaFiles = listOf(Pair("file", "old"))
)
formsRepository.save(form1)

val form2 = FormFixtures.form(
version = "2",
date = 2,
mediaFiles = listOf(Pair("file", "existing"))
)
formsRepository.save(form2)

// Set up same media file on server
val mediaFile = MediaFile("file", "somethingElse", "downloadUrl")
val manifestFile = ManifestFile(null, listOf(mediaFile))
val serverFormDetails =
ServerFormDetails(null, null, "formId", "3", null, false, true, manifestFile)
val formSource = mock<FormSource> {
on { fetchMediaFile(mediaFile.downloadUrl) } doReturn "existing".toByteArray()
.inputStream()
}

val formMediaDownloader = FormMediaDownloader(formsRepository, formSource)
val result = formMediaDownloader.download(
serverFormDetails,
File(TempFiles.createTempDir(), "temp").absolutePath,
TempFiles.createTempDir(),
mock()
)

assertThat(result, equalTo(false))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.odk.collect.android.R
import org.odk.collect.android.support.MockFormEntryPromptBuilder
import org.odk.collect.android.widgets.support.FormFixtures.selectChoice
import org.odk.collect.android.widgets.support.FormFixtures.treeElement
import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice
import org.odk.collect.android.widgets.support.FormElementFixtures.treeElement
import org.odk.collect.androidtest.getOrAwaitValue
import org.odk.collect.geo.selection.MappableSelectItem.IconifiedText
import org.odk.collect.maps.MapPoint
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ import org.odk.collect.android.support.MockFormEntryPromptBuilder
import org.odk.collect.android.utilities.Appearances
import org.odk.collect.android.widgets.items.SelectOneFromMapDialogFragment.Companion.ARG_FORM_INDEX
import org.odk.collect.android.widgets.items.SelectOneFromMapDialogFragment.Companion.ARG_SELECTED_INDEX
import org.odk.collect.android.widgets.support.FormFixtures.selectChoice
import org.odk.collect.android.widgets.support.FormFixtures.treeElement
import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice
import org.odk.collect.android.widgets.support.FormElementFixtures.treeElement
import org.odk.collect.android.widgets.support.NoOpMapFragment
import org.odk.collect.async.Scheduler
import org.odk.collect.fragmentstest.FragmentScenarioLauncherRule
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import org.odk.collect.android.preferences.GuidanceHint
import org.odk.collect.android.support.CollectHelpers
import org.odk.collect.android.support.MockFormEntryPromptBuilder
import org.odk.collect.android.support.WidgetTestActivity
import org.odk.collect.android.widgets.support.FormFixtures.selectChoice
import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice
import org.odk.collect.android.widgets.support.NoOpMapFragment
import org.odk.collect.android.widgets.support.QuestionWidgetHelpers.mockValueChangedListener
import org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithAnswer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import org.javarosa.core.model.SelectChoice
import org.javarosa.core.model.data.StringData
import org.javarosa.core.model.instance.TreeElement

object FormFixtures {
object FormElementFixtures {

fun selectChoice(
value: String = "foo",
Expand Down
30 changes: 30 additions & 0 deletions formstest/src/main/java/org/odk/collect/formstest/FormFixtures.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.odk.collect.formstest

import org.odk.collect.forms.Form
import org.odk.collect.shared.TempFiles
import java.io.File

object FormFixtures {
fun form(
formId: String = "formId",
version: String = "1",
date: Long = 1,
mediaFiles: List<Pair<String, String>> = emptyList()
): Form {
val formFilesPath = TempFiles.createTempDir().absolutePath
val mediaFilePath = TempFiles.createTempDir().absolutePath

mediaFiles.forEach { (name, contents) ->
File(mediaFilePath, name).also { it.writeBytes(contents.toByteArray()) }
}

return Form.Builder()
.displayName("Test Form")
.formId(formId)
.version(version)
.formFilePath(FormUtils.createFormFixtureFile(formId, version, formFilesPath))
.formMediaPath(mediaFilePath)
.date(date)
.build()
}
}
Loading

0 comments on commit faf9891

Please sign in to comment.