Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

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

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Oct 12, 2020

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

This comment has been minimized.

@lognaturel lognaturel force-pushed the external-instances branch 3 times, most recently from c5c11dd to 52c586d Compare October 12, 2020 17:34
@lognaturel
Copy link
Member Author

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
Copy link
Member Author

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 lognaturel requested a review from ggalmazor October 13, 2020 04:52
@lognaturel lognaturel marked this pull request as ready for review October 13, 2020 04:54
@lognaturel
Copy link
Member Author

lognaturel commented Oct 13, 2020

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

@ggalmazor
Copy link
Contributor

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

@lognaturel
Copy link
Member Author

That works, @ggalmazor!

Needed to build a FormDef for files with external secondary instances.
@kkrawczyk123
Copy link
Contributor

Tested with success!
Verified on Ubuntu.
Tested scenarios:

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

@mmarciniak90
Copy link

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
Copy link
Member Author

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 #872 (comment)? That only applies to XML files that are attached to instances, not to form definitions.

@lognaturel lognaturel merged commit 98fb98e into getodk:master Oct 27, 2020
@lognaturel lognaturel deleted the external-instances branch October 27, 2020 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forms with external secondary instances aren't shown on export or push tabs
6 participants