Issue 479 reload cache after creating it #495
Conversation
- This will let us update the cache before setting it in FileSystemUtils
- Once we have decided that we need to create a new cache file, we scan the filesystem to update its contents - This can happen when the serialized FormCache object has a different serialUUID after an update to Briefcase - This also must happen when the user sets a new storage directory
- This can happen on restart or when the user sets the storage directory
- Items that aren't present in the filesystem anymore should be removed
- Mainly, migrate the code to use Streams and Java NIO2, and add semantics - Lose an unwanted? side effect that would delete files from the user's computer when they can't be processed by the cache updating method - Also some rename some members of the class
- That way we don't need to pass the briefcase directory every time we need to update the cache
- We cant't rely on the shutdown hook because the user might change the storage location before closing Briefcase and we would lose data - Also change the tight coupling between the ServerFetcher and the FormCache with a loose coupling using the EventBus (this will make further refactoring of both classes easier)
- We have to write new export panel ui tests anyway
- Then use it in the SettingsPanel whenever the user sets a new storage location - Aknowledge that the cache file and briefcase dir can be not present by changing their types to Optional in FormCache
- Now, when the user clears the storage location, the form cache updates its state and launches an update event accordingly - This will let the UI panels update their contents accordingly
- The panel should always show the contents of the incoming forms from the cache - We can't just replace the form list because all the FormStatus instances might have state that we would lose
|
@lognaturel, could you review this one? I know you've had problems to use the new version and this PR should solve any transition problem |
| Set<String> scannedFiles = new HashSet<>(); | ||
| list(path.resolve("forms")) | ||
| .map(this::getFormFilePath) | ||
| .peek(p -> scannedFiles.add(p.toString())) |
There was a problem hiding this comment.
What’s the benefit of using peek here?
There was a problem hiding this comment.
None whatsoever. I'll remove that
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public void setLocation(Path newBriefcaseDir) { |
There was a problem hiding this comment.
It looks like now when setting a location to an existing directory with a valid cache, that cache is rebuilt. Is that intended?
There was a problem hiding this comment.
Yes, we can't be sure that the user hasn't changed the files inside. The only time we don't want to rebuild it is on startup, which, now that you mention, I think I need to review
- This is: each time new forms are pulled and whenever the user sets/unsets the briefcase directory
| }); | ||
| hashByPath.keySet().stream().filter(p -> !scannedFiles.contains(p)).collect(toList()).forEach(hashByPath::remove); | ||
| formDefByPath.keySet().stream().filter(p -> !scannedFiles.contains(p)).collect(toList()).forEach(formDefByPath::remove); | ||
| EventBus.publish(new CacheUpdateEvent()); |
There was a problem hiding this comment.
Do you need the collect here? Here’s my rewrite, with the common predicate extracted.
Predicate<String> notScanned = p -> !scannedFiles.contains(p);
hashByPath.keySet().stream().filter(notScanned).forEach(hashByPath::remove);
formDefByPath.keySet().stream().filter(notScanned).forEach(formDefByPath::remove);
There was a problem hiding this comment.
Is this a problem?
If the map is modified while an iteration over the set is in progress (except through the iterator's own remove operation), the results of the iteration are undefined.
There was a problem hiding this comment.
Yeah, the collect is to prevent that error. Otherwise, you are removing elements while you iterate the lists
There was a problem hiding this comment.
It's not very elegant. I'm open to suggetsions ;)
There was a problem hiding this comment.
Wow, look what IDEA just suggested.
hashByPath.keySet().removeIf(notScanned);
formDefByPath.keySet().removeIf(notScanned);
I didn’t check that it removes the elements from the map behind the key set view, but I think it does.
There was a problem hiding this comment.
That's neat! I'll add that
There was a problem hiding this comment.
I checked:
Map<String, String> m = new HashMap<>();
m.put("a", "aa");
m.put("b", "bb");
System.out.println(m);
m.keySet().removeIf(k -> k.equals("a"));
System.out.println(m);
{a=aa, b=bb}
{b=bb}
There was a problem hiding this comment.
Oh my, that's very troubling... maps are super mutable.
|
My review of the code was relatively surface but nothing jumped out at me as problematic. This behavior is inline with what I expect! The build failure is just an unneeded import. |
|
Oh, and the commit messages are really helpful for understanding the rationale of each change, thank you @ggalmazor. |
|
|
||
| List<FormStatus> forms = FileSystemUtils.getODKFormList(odkDir.toFile()).stream() | ||
| .map(form -> new FormStatus(FormStatus.TransferType.GATHER, form)) | ||
| .collect(toList()); | ||
|
|
||
| TransferFromODK.pull(BriefcasePreferences.buildBriefcaseDir(Paths.get(storageDir)), odkDir, forms); | ||
| TransferFromODK.pull(briefcaseDir, odkDir, forms); | ||
| // The form cache should update and save itself now |
There was a problem hiding this comment.
Not sure this comment will age well (provide value much in the future)
| } | ||
| } | ||
| }); | ||
| // Warning: Remove map entries by mutating the key set works because the key set is a view on the map |
There was a problem hiding this comment.
Let this be a trigger warning for all you functional purists. ;-)
| formDefByPath.keySet().stream().filter(p -> !scannedFiles.contains(p)).collect(toList()).forEach(formDefByPath::remove); | ||
| // Warning: Remove map entries by mutating the key set works because the key set is a view on the map | ||
| hashByPath.keySet().removeIf(key -> !scannedFiles.contains(key)); | ||
| formDefByPath.keySet().removeIf(key -> !scannedFiles.contains(key)); |
There was a problem hiding this comment.
Somehow I knew you would reject my suggestion to factor out the common predicate. We’re getting to know each other! :-)
|
Verified on Ubuntu, MacOS and Windows |
Closes #479
What has been done to verify that this works as intended?
Written new tests that cover all the form cache's behavior.
Also tested manually:
Also tested manually to set/clear to different storage locations on my computer to see that each time I got the expected form list
Also tested manually to pull new forms and existing forms to check that the push/export tabs where updated accordingly.
Why is this the best possible solution? Were any other approaches considered?
I could have patched a smaller change into the
FormCachefile, but its code was long overdue a review and some tests.While I was writing the tests I could see how Briefcase was coupled to different parts of it and how some methods inside where doing too many things at once that were getting in the way of testing it properly.
After having a good coverage of the implementation I was able to refactor it to simplify the design of parts of Briefcase that depend on the form cache.
I also have reviewed all the event flow pulling new forms > updating the cache > updating UI elements to make it more coherent and reduce duplicated processes.
Are there any risks to merging this code? If so, what are they?
Nope.
Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.
Nope.