Collect: CSV external secondary instances don't work because the jr://file-csv scheme is undefined

Created on 6 Sep 2019 · 11Comments · Source: getodk/collect

Add file-csv as a session root translator in FormLoaderTask. It needs to go before file because file is a substring.

https://github.com/opendatakit/javarosa/files/3095508/external_csv.zip is a form for testing. See https://docs.opendatakit.org/collect-forms#loading-form-media for loading form media. If pushing the form directly to a device, a folder with name <form_id>-media needs to be created and populated with the media.

good first issue

Source

lognaturel

Most helpful comment

@seadowg Thanks, I posted the draft request. The changes include a fix for your new AuditEventSaveTask which was causing test failures on Windows. I probably should have created a separate branch for that fix since it's unrelated to this external CSV secondary instance work, but it's stuck here now. None of these changes are urgent.

breakbusyloop on 5 Dec 2019

👍2

All 11 comments

Hello @lognaturel, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 15 days.

You can reclaim this issue or claim any other issue by commenting @opendatakit-bot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

getodk-bot on 16 Sep 2019

I'm interested in this issue. There's a lot of context I don't yet have. Is the change really as simple as inserting the following in just the right place? , "file-csv"

I'm concerned about the ordering issue in the list of root translators. Is there a latent bug, or is this something outside of our control?

If I do take this issue, I'll need a crash course on secondary instances. My hunch is just that it means "an alternate (backup? local?) database that mirrors each submission to the primary (remote? unreliable?) database".

breakbusyloop on 26 Nov 2019

Yes, it really is that simple. But this will unmask #3335 which is more annoying.

I'm concerned about the ordering issue in the list of root translators. Is there a latent bug, or is this something outside of our control?

It's been a while since I looked at this so it would be good to double check but here's how I remember it:

forms can contain file paths structured like jr://file/foo or jr://file-csv/foo
the path translators use string matching to identify how to do the conversion to paths on the device
jr://file is a substring of jr://file-csv/foo so it would match

I think the design is questionable, certainly. That part is in JavaRosa. My sense is that it's not worth reworking but would be interested in what you think.

My hunch is just that it means "an alternate (backup? local?) database that mirrors each submission to the primary (remote? unreliable?) database".

Ooh, I like this game and the feature you've come up with!

https://opendatakit.github.io/xforms-spec/#secondary-instances---external has the explanation. instance is an overloaded term so beware. In most of Collect, "the form instance" refers to a particular filled form. Each form definition has a primary instance that is meant to be populated with data so that's "the instance". A form definition may also have secondary instances that provide data that can be queried. Those secondary instances can be defined by data files in which case they are external.

lognaturel on 26 Nov 2019

I think the design is questionable, certainly. That part is in JavaRosa. My sense is that it's not worth reworking but would be interested in what you think.

The path translators should be able to distinguish between a terminal value ('foo') and a form translator identifier. It sounds like a greedy string matching approach is being used. But since the paths have to follow a known structure (with '/' as an important signifier) it seems this really should be fixed in OpenRosa. I'll see if I can write a test that can expose its brittleness.

Haha about inventing features, that's my favorite way to not get work done :)

Perfect explanation in that one paragraph. Thank you! I'll read more elsewhere, but I get the gist.

breakbusyloop on 26 Nov 2019

👍1

But since the paths have to follow a known structure (with '/' as an important signifier) it seems this really should be fixed in OpenRosa

Agreed. And now I can't even remember whether the implementation is actually in JavaRosa or whether it's a JR interface implemented in Collect.

lognaturel on 26 Nov 2019

Looks like it's in JavaRosa, specifically ReferenceFactory.derives

breakbusyloop on 3 Dec 2019

👍1

I'm not able to confirm that order actually matters for the list of root translators. I've created a test that demonstrates this, but out of excess caution I also have a test that ensures the root translators are added in an unambiguous order.

breakbusyloop on 4 Dec 2019

This is fixed now, along with tests and some light refactoring. I'll wait until this has been assigned to a new milestone before I issue a pull request. In the meantime I'll tackle issue 3335.

breakbusyloop on 5 Dec 2019

@OpenDataNerd go ahead and put up a pull request if you want! If you're not ready for review you can create a Draft PR or if it is ready we're happy to have it. PRs can still be reviewed (potentially with a slower feedback cycle) during code freeze, we just don't merge them.