getodk / briefcase

ODK Briefcase is a Java application for fetching and pushing forms and their contents. It helps make billions of data points from ODK portable. Contribute and make the world a better place! ✨💼✨

https://docs.getodk.org/briefcase-intro

Other

60 stars 156 forks source link

Resolve all jr:// URIs to media directory #879

Closed lognaturel closed 3 years ago

lognaturel commented 3 years ago

Needed to build a FormDef for files with external secondary instances.

Closes #878

What has been done to verify that this works as intended?

Wrote a test that fails with the user-reported error on master. Tried downloading a real form with external secondary instances and then exporting it. @getodk/testers I think it would be good to try with some of the other ones that you have with both xml and csv external instances and a few submissions.

Why is this the best possible solution? Were any other approaches considered?

The core of the solution is setting up a ReferenceFactory for the singleton ReferenceManager. I don't see any alternatives to that. The decision points I saw were around where to actually build the media directory paths and how much of the ReferenceFactory to implement. It felt reasonable to make getMediaDirectory public and JavaRosaWrapper and BriefcaseFormDefinition call it.

For the ReferenceFactory, I only implemented getLocalURI because I believe that's all we need. The other methods throw UnsupportedOperationException so we should be able to tell easily if I was wrong. I had all media of every type resolve to the same directory and don't do any check on the jr:// prefix provided because I consider that a form design concern.

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?

I changed the implementation of getMediaDirectory to not create the directory if it didn't exist but just get the path. The only risk I could think of associated with that was when media attachments are present so I verified that. I could have missed something. Other than that the changes are additive so low-risk.

Does this change require updates to documentation? If so, please file an issue at https://github.com/getodk/docs/issues/new and include the link below.

No.

codecov-io commented 3 years ago

Codecov Report

Merging #879 into master will decrease coverage by 0.01%. The diff coverage is 57.14%.

@@             Coverage Diff              @@
##             master     #879      +/-   ##
============================================
- Coverage     48.33%   48.31%   -0.02%     
- Complexity     1647     1648       +1     
============================================
  Files           195      196       +1     
  Lines         10422    10437      +15     
  Branches        753      752       -1     
============================================
+ Hits           5037     5043       +6     
- Misses         5026     5034       +8     
- Partials        359      360       +1

Impacted Files	Coverage Δ	Complexity Δ
...it/aggregate/parser/MediaFileReferenceFactory.java	`42.85% <42.85%> (ø)`	`3.00 <3.00> (?)`
...takit/briefcase/model/BriefcaseFormDefinition.java	`33.33% <66.66%> (+0.18%)`	`16.00 <0.00> (ø)`
...it/aggregate/parser/BaseFormParserForJavaRosa.java	`26.40% <100.00%> (+0.41%)`	`18.00 <0.00> (ø)`
...rg/opendatakit/briefcase/util/FileSystemUtils.java	`42.85% <100.00%> (+2.11%)`	`19.00 <1.00> (+1.00)`
...ndatakit/briefcase/util/JavaRosaParserWrapper.java	`88.88% <100.00%> (ø)`	`6.00 <1.00> (ø)`
...g/opendatakit/briefcase/ui/export/ExportPanel.java	`48.07% <0.00%> (-1.93%)`	`14.00% <0.00%> (-1.00%)`
...g/opendatakit/briefcase/reused/UncheckedFiles.java	`44.53% <0.00%> (-1.69%)`	`31.00% <0.00%> (-1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b55a171...2845985. Read the comment docs.

lognaturel commented 3 years ago

I don't understand why the test fails on CI. It passes locally. CI says

Unable to parse external secondary instance: java.io.FileNotFoundException: /tmp/briefcase_test14560796627712759977/briefcase_test14560796627712759977-media/external-xml.xml (No such file or directory)```

That is indeed the path structure I would expect.

lognaturel commented 3 years ago

I SSHed into the build and saw /tmp/briefcase_test16513623838698692582/forms/nested-repeats/nested-repeats-media/external-xml.xml (No such file or directory) which led me to see I was building the media URI wrong. Still, I don't know how we could have ended up with that path. I noticed that there is lots of test data left in /tmp which suggests cleanup is incomplete for some tests.

@ggalmazor I'd love it if you could take a quick peek when you have a moment. Hopefully this one will be very quick.

lognaturel commented 3 years ago

~Now I have no idea why the test is failing. 😭~ We have to reset the singleton ReferenceManager between each form load.

ggalmazor commented 3 years ago

Hi, @lognaturel! I'd need to wait until the weekend to take a look. Does that work for you?

lognaturel commented 3 years ago

That works, @ggalmazor!

kkrawczyk123 commented 3 years ago

Tested with success! Verified on Ubuntu. Tested scenarios:

pull form Central
export
pull with cmd
export with cmd

unable to push to Central manually and using cmd both: what is probably connected to xml media file and we already encountered that issue in #872 that should be fixed now. Error message displayed:

Form with external secondary instance - Start pushing form and submissions
Form with external secondary instance - Can't determine form encryption:Unable to parse external secondary instance: org.javarosa.core.reference.InvalidReferenceException: The reference "jr://file/towns.xml" was invalid and couldn't be understood. The javarosa jr:// reference root "file" is not available on this system and may have been mis-typed. Some available roots: 
Problem found at nodeset: /html/head/model/instance
With element <instance id="towns" src="jr://file/towns.xml"/>
Form with external secondary instance - Success
All operations completed

download on Collect
send submissions to Central
pull before export doesn' work (reported in #882 as it's also visible on 1.17.4)
form with external csv file
xlsx form with external csv
form with external csv and xml: what is actually weird I was able to push that form https://test.central.getodk.org/#/projects/214/forms/formWithExternalFiles to central despite of xml media attachment. cc @lognaturel

mmarciniak90 commented 3 years ago

Verified on Mac. The main issue is no longer visible. I didn't notice more unexpected issues than described by Kasia.

@getodk-bot unlabel "needs testing" @getodk-bot label "behavior verified"

lognaturel commented 3 years ago

what is actually weird I was able to push that form test.central.getodk.org/#/projects/214/forms/formWithExternalFiles to central despite of xml media attachment

That seems good and expected to me! Are you thinking about the issue you identified in https://github.com/getodk/briefcase/pull/872#issuecomment-709371710? That only applies to XML files that are attached to instances, not to form definitions.