Merge pull request #807 from ggalmazor/issue_806_metadata_file_with_r…

…elative_paths Issue #806 Metadata files should have relative paths
getodk · Sep 10, 2019 · ba33949 · ba33949
2 parents 7bb6185 + 7c66042
commit ba33949
Show file tree

Hide file tree

Showing 14 changed files with 84 additions and 83 deletions.
diff --git a/src/org/opendatakit/briefcase/export/ExportForms.java b/src/org/opendatakit/briefcase/export/ExportForms.java
@@ -34,11 +34,11 @@
 public class ExportForms {
   private static final String EXPORT_DATE_PREFIX = "export_date_";
   private static final String CUSTOM_CONF_PREFIX = "custom_";
-  private final List<FormStatus> forms;
-  private ExportConfiguration defaultConfiguration;
   private final Map<String, ExportConfiguration> customConfigurations;
   private final Map<String, LocalDateTime> lastExportDateTimes;
   private final List<BiConsumer<String, LocalDateTime>> onSuccessfulExportCallbacks = new ArrayList<>();
+  private ExportConfiguration defaultConfiguration;
+  private List<FormStatus> forms;
   private Map<String, FormStatus> formsIndex = new HashMap<>();
 
   public ExportForms(List<FormStatus> forms, ExportConfiguration defaultConfiguration, Map<String, ExportConfiguration> configurations, Map<String, LocalDateTime> lastExportDateTimes) {
@@ -84,11 +84,7 @@ public static String buildCustomConfPrefix(String formId) {
   }
 
   public void merge(List<FormStatus> incomingForms) {
-    List<String> incomingFormIds = incomingForms.stream().map(ExportForms::getFormId).collect(toList());
-    List<FormStatus> formsToAdd = incomingForms.stream().filter(form -> !formsIndex.containsKey(getFormId(form))).collect(toList());
-    List<FormStatus> formsToRemove = formsIndex.values().stream().filter(form -> !incomingFormIds.contains(getFormId(form))).collect(toList());
-    forms.addAll(formsToAdd);
-    forms.removeAll(formsToRemove);
+    forms = new ArrayList<>(incomingForms);
     rebuildIndex();
   }
 

diff --git a/src/org/opendatakit/briefcase/export/ExportToCsv.java b/src/org/opendatakit/briefcase/export/ExportToCsv.java
@@ -126,7 +126,7 @@ private static ExportOutcome export(FormMetadataPort formMetadataPort, FormMetad
         .orElse(CsvLines.empty())
         .getLastLine()
         .ifPresent(line -> {
-          formMetadataPort.execute(updateLastExportedSubmission(formMetadata.getKey(), line.getInstanceId(), line.getSubmissionDate(), OffsetDateTime.now(), formStatus.getFormDir(briefcaseDir)));
+          formMetadataPort.execute(updateLastExportedSubmission(formMetadata.getKey(), line.getInstanceId(), line.getSubmissionDate(), OffsetDateTime.now(), briefcaseDir, formStatus.getFormDir(briefcaseDir)));
         });
 
     ExportOutcome exportOutcome = exportTracker.computeOutcome();

diff --git a/src/org/opendatakit/briefcase/model/form/FileSystemFormMetadataAdapter.java b/src/org/opendatakit/briefcase/model/form/FileSystemFormMetadataAdapter.java
@@ -2,6 +2,7 @@
 
 import static java.nio.file.StandardOpenOption.CREATE;
 import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toMap;
 import static org.opendatakit.briefcase.reused.UncheckedFiles.walk;
 import static org.opendatakit.briefcase.reused.UncheckedFiles.write;
@@ -13,6 +14,7 @@
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
@@ -22,7 +24,6 @@
 import org.opendatakit.briefcase.export.XmlElement;
 import org.opendatakit.briefcase.reused.BriefcaseException;
 import org.opendatakit.briefcase.reused.LegacyPrefs;
-import org.opendatakit.briefcase.reused.UncheckedFiles;
 
 public class FileSystemFormMetadataAdapter implements FormMetadataPort {
   private static final ObjectMapper MAPPER = new ObjectMapper().findAndRegisterModules();
@@ -55,19 +56,21 @@ public FormMetadataPort syncWithFilesAt(Path storageRoot) {
     Stream<Path> formFiles = candidateFormFiles.filter(path -> isAForm(XmlElement.from(path)));
 
     // Parse existing metadata.json files or build new FormMetadata from form files
-    Stream<FormMetadata> metadataFiles = formFiles.map(formFile -> {
+    // At this point, we collect the stream to avoid problems coming
+    // from actually writing files in the same folders we're reading from
+    List<FormMetadata> metadataFiles = formFiles.map(formFile -> {
       Path formDir = formFile.getParent();
       Path metadataFile = formDir.resolve("metadata.json");
-      FormMetadata formMetadata = Files.exists(metadataFile) ? deserialize(metadataFile) : FormMetadata.from(formFile);
+      FormMetadata formMetadata = Files.exists(metadataFile) ? deserialize(storageRoot, metadataFile) : FormMetadata.from(storageRoot, formFile);
       if (!formMetadata.getCursor().isEmpty())
         return formMetadata;
       // Try to recover any missing cursor from the legacy Java prefs system
       return LegacyPrefs.readCursor(formMetadata.getKey().getId())
           .map(formMetadata::withCursor)
           .orElse(formMetadata);
-    });
+    }).collect(toList());
 
-    Map<FormKey, FormMetadata> forms = metadataFiles
+    Map<FormKey, FormMetadata> forms = metadataFiles.stream()
         .peek(this::persist) // Write updated metadata.json files
         .collect(toMap(FormMetadata::getKey, metadata -> metadata));
 
@@ -84,11 +87,6 @@ private boolean isAForm(XmlElement root) {
 
   @Override
   public void flush() {
-    store.values()
-        .stream()
-        .map(FileSystemFormMetadataAdapter::getMetadataFile)
-        .filter(Files::exists)
-        .forEach(UncheckedFiles::delete);
     store.clear();
   }
 
@@ -116,9 +114,8 @@ public Stream<FormMetadata> fetchAll() {
   }
 
   // region Path <-> JSON <-> FormMetadata serialization
-  private static FormMetadata deserialize(Path metadataFile) {
-    JsonNode root = uncheckedReadTree(metadataFile);
-    return FormMetadata.from(root);
+  private static FormMetadata deserialize(Path storageRoot, Path metadataFile) {
+    return FormMetadata.from(storageRoot, uncheckedReadTree(metadataFile));
   }
 
   private static Path serialize(FormMetadata metaData) {
@@ -142,7 +139,7 @@ private static JsonNode uncheckedReadTree(Path jsonFile) {
   }
 
   private static Path getMetadataFile(FormMetadata metaData) {
-    return metaData.getStorageDirectory().resolve("metadata.json");
+    return metaData.getFormDir().resolve("metadata.json");
   }
   // endregion
 }
diff --git a/src/org/opendatakit/briefcase/model/form/FormMetadata.java b/src/org/opendatakit/briefcase/model/form/FormMetadata.java
@@ -16,27 +16,30 @@
 
 public class FormMetadata implements AsJson {
   private final FormKey key;
-  private final Path storageDirectory;
+  private final Path storageRoot;
+  private final Path formDir;
   private final boolean hasBeenPulled;
   private final Cursor cursor;
   private final Optional<SubmissionExportMetadata> lastExportedSubmission;
 
-  public FormMetadata(FormKey key, Path storageDirectory, boolean hasBeenPulled, Cursor cursor, Optional<SubmissionExportMetadata> lastExportedSubmission) {
+  public FormMetadata(FormKey key, Path storageRoot, Path formDir, boolean hasBeenPulled, Cursor cursor, Optional<SubmissionExportMetadata> lastExportedSubmission) {
     this.key = key;
-    this.storageDirectory = storageDirectory;
+    this.storageRoot = storageRoot;
+    this.formDir = formDir.isAbsolute() ? storageRoot.relativize(formDir) : formDir;
     this.hasBeenPulled = hasBeenPulled;
     this.cursor = cursor;
     this.lastExportedSubmission = lastExportedSubmission;
   }
 
-  public static FormMetadata of(FormKey key, Path storageDirectory) {
-    // Hardcoded storage directory because we want this class to decide where a
-    // form is/should be stored. Now it's based on the Briefcase storage directory,
-    // but it will change in the future to be based on a hash of the form key.
-    return new FormMetadata(key, storageDirectory, false, Cursor.empty(), Optional.empty());
+  public static FormMetadata of(FormKey key, Path storageRoot, Path formDir) {
+    return new FormMetadata(
+        key,
+        storageRoot,
+        formDir,
+        false, Cursor.empty(), Optional.empty());
   }
 
-  public static FormMetadata from(Path formFile) {
+  public static FormMetadata from(Path storageRoot, Path formFile) {
     XmlElement root = XmlElement.from(formFile);
     assert root.getName().equals("html");
     String name = root.findElements("head", "title").get(0)
@@ -49,13 +52,14 @@ public static FormMetadata from(Path formFile) {
     String id = mainInstance.childrenOf().get(0).getAttributeValue("id").orElseThrow(BriefcaseException::new);
     Optional<String> version = mainInstance.childrenOf().get(0).getAttributeValue("version");
     FormKey key = FormKey.of(name, id, version);
-    return new FormMetadata(key, formFile.getParent(), true, Cursor.empty(), Optional.empty());
+    return new FormMetadata(key, storageRoot, formFile.getParent(), true, Cursor.empty(), Optional.empty());
   }
 
-  public static FormMetadata from(JsonNode root) {
+  public static FormMetadata from(Path storageRoot, JsonNode root) {
     return new FormMetadata(
         FormKey.from(root.get("key")),
-        getJson(root, "storageDirectory").map(JsonNode::asText).map(Paths::get).orElseThrow(BriefcaseException::new),
+        storageRoot,
+        getJson(root, "formDir").map(JsonNode::asText).map(Paths::get).orElseThrow(BriefcaseException::new),
         getJson(root, "hasBeenPulled").map(JsonNode::asBoolean).orElseThrow(BriefcaseException::new),
         Cursor.from(root.get("cursor")),
         getJson(root, "lastExportedSubmission").map(SubmissionExportMetadata::from)
@@ -72,8 +76,8 @@ public FormKey getKey() {
     return key;
   }
 
-  public Path getStorageDirectory() {
-    return storageDirectory;
+  public Path getFormDir() {
+    return storageRoot.resolve(formDir);
   }
 
   public boolean hasBeenPulled() {
@@ -89,26 +93,26 @@ public Optional<SubmissionExportMetadata> getLastExportedSubmission() {
   }
 
   FormMetadata withCursor(Cursor cursor) {
-    return new FormMetadata(key, storageDirectory, hasBeenPulled, cursor, lastExportedSubmission);
+    return new FormMetadata(key, storageRoot, formDir, hasBeenPulled, cursor, lastExportedSubmission);
   }
 
   public FormMetadata withoutCursor() {
-    return new FormMetadata(key, storageDirectory, hasBeenPulled, Cursor.empty(), lastExportedSubmission);
+    return new FormMetadata(key, storageRoot, formDir, hasBeenPulled, Cursor.empty(), lastExportedSubmission);
   }
 
   FormMetadata withHasBeenPulled(boolean hasBeenPulled) {
-    return new FormMetadata(key, storageDirectory, hasBeenPulled, cursor, lastExportedSubmission);
+    return new FormMetadata(key, storageRoot, formDir, hasBeenPulled, cursor, lastExportedSubmission);
   }
 
   FormMetadata withLastExportedSubmission(String instanceId, OffsetDateTime submissionDate, OffsetDateTime exportDateTime) {
-    return new FormMetadata(key, storageDirectory, hasBeenPulled, cursor, Optional.of(new SubmissionExportMetadata(instanceId, submissionDate, exportDateTime)));
+    return new FormMetadata(key, storageRoot, formDir, hasBeenPulled, cursor, Optional.of(new SubmissionExportMetadata(instanceId, submissionDate, exportDateTime)));
   }
 
   @Override
   public ObjectNode asJson(ObjectMapper mapper) {
     ObjectNode root = mapper.createObjectNode();
     root.putObject("key").setAll(key.asJson(mapper));
-    root.put("storageDirectory", storageDirectory.toAbsolutePath().toString());
+    root.put("formDir", formDir.toString());
     root.put("hasBeenPulled", hasBeenPulled);
     root.putObject("cursor").setAll(cursor.asJson(mapper));
     lastExportedSubmission.ifPresent(o -> root.putObject("lastExportedSubmission").setAll(o.asJson(mapper)));
@@ -122,21 +126,23 @@ public boolean equals(Object o) {
     FormMetadata that = (FormMetadata) o;
     return hasBeenPulled == that.hasBeenPulled &&
         Objects.equals(key, that.key) &&
-        Objects.equals(storageDirectory, that.storageDirectory) &&
+        Objects.equals(storageRoot, that.storageRoot) &&
+        Objects.equals(formDir, that.formDir) &&
         Objects.equals(cursor, that.cursor) &&
         Objects.equals(lastExportedSubmission, that.lastExportedSubmission);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(key, storageDirectory, hasBeenPulled, cursor, lastExportedSubmission);
+    return Objects.hash(key, storageRoot, formDir, hasBeenPulled, cursor, lastExportedSubmission);
   }
 
   @Override
   public String toString() {
     return "FormMetadata{" +
         "key=" + key +
-        ", storageDirectory=" + storageDirectory +
+        ", storageRoot=" + storageRoot +
+        ", formDir=" + formDir +
         ", hasBeenPulled=" + hasBeenPulled +
         ", cursor=" + cursor +
         ", lastExportedSubmission=" + lastExportedSubmission +

diff --git a/src/org/opendatakit/briefcase/model/form/FormMetadataCommands.java b/src/org/opendatakit/briefcase/model/form/FormMetadataCommands.java
@@ -7,30 +7,30 @@
 import org.opendatakit.briefcase.pull.aggregate.Cursor;
 
 public class FormMetadataCommands {
-  public static Consumer<FormMetadataPort> updateAsPulled(FormKey key, Cursor cursor, Path storageDirectory) {
+  public static Consumer<FormMetadataPort> updateAsPulled(FormKey key, Cursor cursor, Path storageRoot, Path formDir) {
     return port -> {
       Optional<FormMetadata> fetch = port
           .fetch(key);
       FormMetadata formMetadata = fetch
-          .orElseGet(() -> FormMetadata.of(key, storageDirectory));
+          .orElseGet(() -> FormMetadata.of(key, storageRoot, formDir));
       FormMetadata formMetadata1 = formMetadata
           .withHasBeenPulled(true)
           .withCursor(cursor);
       port.persist(formMetadata1);
     };
   }
 
-  public static Consumer<FormMetadataPort> updateAsPulled(FormKey key, Path storageDirectory) {
+  public static Consumer<FormMetadataPort> updateAsPulled(FormKey key, Path storageRoot, Path formDir) {
     return port -> port.persist(port
         .fetch(key)
-        .orElseGet(() -> FormMetadata.of(key, storageDirectory))
+        .orElseGet(() -> FormMetadata.of(key, storageRoot, formDir))
         .withHasBeenPulled(true));
   }
 
-  public static Consumer<FormMetadataPort> updateLastExportedSubmission(FormKey key, String instanceId, OffsetDateTime submissionDate, OffsetDateTime exportDateTime, Path storageDirectory) {
+  public static Consumer<FormMetadataPort> updateLastExportedSubmission(FormKey key, String instanceId, OffsetDateTime submissionDate, OffsetDateTime exportDateTime, Path storageRoot, Path formDir) {
     return port -> port.persist(port
         .fetch(key)
-        .orElseGet(() -> FormMetadata.of(key, storageDirectory))
+        .orElseGet(() -> FormMetadata.of(key, storageRoot, formDir))
         .withLastExportedSubmission(instanceId, submissionDate, exportDateTime));
   }
 

diff --git a/src/org/opendatakit/briefcase/pull/aggregate/PullFromAggregate.java b/src/org/opendatakit/briefcase/pull/aggregate/PullFromAggregate.java
@@ -152,7 +152,7 @@ public Job<Void> pull(FormStatus form, Optional<Cursor> lastCursor) {
           tracker.trackEnd();
           Cursor newCursor = getLastCursor(instanceIdBatches).orElse(Cursor.empty());
 
-          formMetadataPort.execute(updateAsPulled(key, newCursor, form.getFormDir(briefcaseDir)));
+          formMetadataPort.execute(updateAsPulled(key, newCursor, briefcaseDir, form.getFormDir(briefcaseDir)));
 
           EventBus.publish(PullEvent.Success.of(form, server));
         });

diff --git a/src/org/opendatakit/briefcase/pull/central/PullFromCentral.java b/src/org/opendatakit/briefcase/pull/central/PullFromCentral.java
@@ -112,7 +112,7 @@ public Job<Void> pull(FormStatus form) {
               });
           tracker.trackEnd();
 
-          formMetadataPort.execute(updateAsPulled(key, form.getFormDir(briefcaseDir)));
+          formMetadataPort.execute(updateAsPulled(key, briefcaseDir, form.getFormDir(briefcaseDir)));
           EventBus.publish(PullEvent.Success.of(form, server));
         }));
   }

diff --git a/src/org/opendatakit/briefcase/ui/settings/SettingsPanel.java b/src/org/opendatakit/briefcase/ui/settings/SettingsPanel.java
@@ -53,13 +53,14 @@ private SettingsPanel(SettingsPanelForm form, BriefcasePreferences appPreference
     form.onStorageLocation(path -> {
       Path briefcaseDir = BriefcasePreferences.buildBriefcaseDir(path);
       UncheckedFiles.createBriefcaseDir(briefcaseDir);
+      formCache.unsetLocation();
       formCache.setLocation(briefcaseDir);
       formCache.update();
       appPreferences.setStorageDir(path);
+      formMetadataPort.flush();
       formMetadataPort.syncWithFilesAt(briefcaseDir);
     }, () -> {
       formCache.unsetLocation();
-      formCache.update();
       appPreferences.unsetStorageDir();
       formMetadataPort.flush();
     });

diff --git a/src/org/opendatakit/briefcase/util/FormCache.java b/src/org/opendatakit/briefcase/util/FormCache.java
@@ -86,6 +86,7 @@ public void setLocation(Path newBriefcaseDir) {
   }
 
   public void unsetLocation() {
+    cacheFile.ifPresent(UncheckedFiles::delete);
     briefcaseDir = Optional.empty();
     cacheFile = Optional.empty();
     hashByPath = new HashMap<>();

diff --git a/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java b/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java
@@ -169,6 +169,7 @@ void runExport(boolean overwrite, boolean exportMedia, LocalDate startDate, Loca
     FormKey formKey = FormKey.of(formDef.getFormName(), formDef.getFormId());
     FormMetadata formMetadata = new FormMetadata(
         formKey,
+        briefcaseDir,
         formDef.getFormDir().resolve(stripIllegalChars(formDef.getFormName()) + ".xml"),
         true,
         Cursor.empty(),

diff --git a/test/java/org/opendatakit/briefcase/model/form/FileSystemFormMetadataAdapterTest.java b/test/java/org/opendatakit/briefcase/model/form/FileSystemFormMetadataAdapterTest.java
@@ -5,7 +5,6 @@
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertThat;
 import static org.opendatakit.briefcase.matchers.PathMatchers.exists;
 import static org.opendatakit.briefcase.reused.UncheckedFiles.createDirectories;
@@ -60,18 +59,18 @@ public void tearDown() {
   public void persists_metadata_by_creating_a_metadata_json_file_along_with_the_form_file() throws IOException {
     FormMetadataPort port = FileSystemFormMetadataAdapter.at(storageRoot);
     Path installedForm = installForm(storageRoot, "Some form", "some-form", Optional.empty());
-    FormMetadata persistedMetadata = buildMetadataFrom(installedForm, Cursor.empty());
+    FormMetadata persistedMetadata = buildMetadataFrom(storageRoot, installedForm, Cursor.empty());
     port.persist(persistedMetadata);
 
-    Path metadataFile = persistedMetadata.getStorageDirectory().resolve("metadata.json");
+    Path metadataFile = persistedMetadata.getFormDir().resolve("metadata.json");
     assertThat(metadataFile, exists());
     // Sanity check of the json file's structure and data.
     // Further checks are make implicitly (using object equality) in other tests
     JsonNode root = MAPPER.readTree(metadataFile.toFile());
     assertThat(root.path("key").path("name").asText(), is("Some form"));
     assertThat(root.path("key").path("id").asText(), is("some-form"));
     assertThat(root.path("key").path("version").isNull(), is(true));
-    assertThat(root.path("storageDirectory").asText(), startsWith(storageRoot.resolve("forms").toString()));
+    assertThat(root.path("formDir").asText(), is("forms/Some form"));
     assertThat(root.path("hasBeenPulled").asBoolean(), is(true));
     assertThat(root.path("cursor").path("type").asText(), is("empty"));
     assertThat(root.path("cursor").path("value").isNull(), is(true));
@@ -102,7 +101,7 @@ public void persists_and_fetches_form_metadata() {
 
       // Install the form in the storage root, and get a metadata object from it
       Path formFile = formInstaller.get();
-      FormMetadata metadataToBePersisted = buildMetadataFrom(formFile, cursor);
+      FormMetadata metadataToBePersisted = buildMetadataFrom(storageRoot, formFile, cursor);
 
       // Persist metadata into the filesystem
       FormMetadataPort persistPort = FileSystemFormMetadataAdapter.at(storageRoot);
@@ -121,7 +120,7 @@ public void persists_and_fetches_form_metadata() {
   public void persists_and_fetches_collections_of_form_metadata() {
     List<FormMetadata> expectedFormMetadatas = IntStream.range(0, 10)
         .mapToObj(i -> installForm(storageRoot, "Form " + i, "form-" + i, Optional.empty()))
-        .map(formFile -> buildMetadataFrom(formFile, Cursor.empty()))
+        .map(formFile -> buildMetadataFrom(storageRoot, formFile, Cursor.empty()))
         .collect(toList());
 
     FileSystemFormMetadataAdapter.at(storageRoot).persist(expectedFormMetadatas.stream());
@@ -174,7 +173,7 @@ private static XmlElement readXml(InputStream in) {
     }
   }
 
-  private static FormMetadata buildMetadataFrom(Path formFile, Cursor cursor) {
+  private static FormMetadata buildMetadataFrom(Path storageRoot, Path formFile, Cursor cursor) {
     XmlElement root = readXml(newInputStream(formFile));
     XmlElement mainInstance = root.findElements("head", "model", "instance").stream()
         .filter(e -> !e.hasAttribute("id") // It's not a secondary instance
@@ -188,6 +187,6 @@ private static FormMetadata buildMetadataFrom(Path formFile, Cursor cursor) {
         mainInstance.getAttributeValue("id").orElseThrow(RuntimeException::new),
         mainInstance.getAttributeValue("version")
     );
-    return new FormMetadata(key, formFile.getParent(), cursor.isEmpty(), cursor, Optional.empty());
+    return new FormMetadata(key, storageRoot, formFile.getParent(), cursor.isEmpty(), cursor, Optional.empty());
   }
 }