Show Entities version error to the user

[lognaturel]

If a form has an unsupported Entities version, Collect currently silently fails at download time.

With match exactly, the refresh with error icon is shown and there's no way to see any error at all.

With manual download, you can "Show error" but then only get a generic message about contacting the person who asked to collect data.

In both cases, there should be a clear message telling the user that the form is not compatible with their version of Collect and that they should upgrade.

Proposed text:

This form is not supported by this version of ODK Collect. Please upgrade Collect. If you keep having this problem, report it to the person who asked you to collect data.

Form Entities spec version: ${version}

The approach here should be to stop using the full form parsing mechanism for extracting metadata at download time. That way, the error message would be shown when attempting to open the form. The code to parse metadata from an XForm is already partially complete and in use in StubOpenRosaServer.

Acceptance

• Given I have added a form with an entity version that Collect does not support to my server
  And I've added that project to Collect
  And I'm using Exactly Match Server form management
  When I refresh forms
  Then I do not see an error
  And the forms appear on the device

• Given I have added a form with an entity version that Collect does not support to my server
  And I've added that project to Collect
  And I've downloaded forms
  When I start the form
  Then I see an error that the entity version is unrecognized/unsupported

• Given I have added a form with an entity version that Collect does not support to my server
  And I've added that project to Collect
  When I manually download the form
  Then I do not see an error
  And the forms appear on the device

[seadowg]

Just came here to add that once again the metadata parsing using the full JavaRosa parse stack actively got in my way while working on something absolutely unrelated to metadata - I was playing around with entity parsing code and failing out in tests earlier than was useful for investigating something. Just another reason to make this change by using more basic XML parsing there!

[grzesiek2010]

@lognaturel I need some clarification.

From what I understand, we don't want to check versions in EntityFormParseProcessor or throw any exceptions, allowing forms with unsupported versions to still appear in the form list for users to click on. Upon clicking, we would then display a descriptive message. How do we determine if a form uses an unsupported entity version? My initial thought was that you intended to parse it before opening, but that doesn't seem right. You also mentioned reading metadata, so:

1. When you referred to reading metadata, were you talking about this part of the code?

   collect/collect_app/src/main/java/org/odk/collect/android/utilities/FileUtils.java

   Line 172 in 4dbe61e

   public static HashMap<String, String> getMetadataFromFormDefinition(File formDefinitionXml) throws XFormParser.ParseException {

2. If the answer is yes, should we read the entity version there as well, save it to the database along with other metadata, and then read that value before opening the form to display an error if necessary?
3. If so, you also want to refactor that method to avoid performing full-form parsing as I understand?

[lognaturel]

we don't want to check versions in EntityFormParseProcessor or throw any exceptions, allowing forms with unsupported versions to still appear in the form list for users to click on. Upon clicking, we would then display a descriptive message

From my perspective, the only requirement is that the user get a clear error message and end up in a consistent state. I thought there already was a message implemented but now I see there isn't. I've added a suggested message to the issue.

I believe @seadowg has strongly suggested changing the way forms are parsed to get metadata for the database instead of trying to show the error at download time because #5026 has a lot of subtleties to it. Also, we don't currently build the cache file with this initial parse so it doesn't have much value. If we could make it faster by having it stop as soon as it has all it needs, that would be beneficial. The one piece that I think may be tricky is the geometry XPath. That isn't shown in the UI so maybe it could be filled in when the form is first opened for filling / parsed by JavaRosa.

See EntityFormParseProcessor.processModelAttribute for where the Entity version detection currently happens.

When you referred to reading metadata, were you talking about this part of the code?

Yes. The reason there's a form parse at that point in time is to get information that's needed to display the form in form lists.

should we read the entity version there as well, save it to the database along with other metadata, and then read that value before opening the form to display an error if necessary?

I don't think that's necessary. It feels like it should be possible to let form parsing happen on form open and then show any parse errors in a blocking dialog.

refactor that method to avoid performing full-form parsing

The idea is that if we avoid form parsing with JavaRosa when all we need are a few specific pieces of metadata, we won't get any exceptions unless the XML is totally corrupt or not a valid form at all. When the user opens the form, then we'll get any exceptions that JavaRosa throws, giving the user a chance to read a detailed message about why the form doesn't work.

[grzesiek2010]

Alright, so these are two related but distinct tasks:

1. We need to implement a simple form parser to read the form metadata - in this case, the entities version to display an error when opening a form (if the version is unsupported).
2. Once we have this parser, we can replace the logic in this section.

Am I right or missing something?

[lognaturel]

We need to implement a simple form parser to read the form metadata

Yes, and there’s a note in the issue about this being partially implemented. This MUST capture the data needed to display a form in a form list. It’s less clear to me what to do about other metadata like encryption key and geometry XPath. Encryption key feels like it must be captured at that time too — if it’s not correctly captured the form will not work as intended. Maybe there’s a way to move that condition to form open time, I’m not sure. In other words, if encryption key exists in the form, it should be guaranteed to be written to the db before the form can be open. Geometry XPath is somewhat less important but is expected to be there if applicable.

in this case, the entities version to display an error when opening a form (if the version is unsupported)

The idea is to ignore this in the parse pass intended to get metadata to display forms. The check should then run at form open time and block open if it fails.

[grzesiek2010]

The idea is to ignore this in the parse pass intended to get metadata to display forms.

Why? Writing our own parser wouldn't significantly impact reading or skipping the entity version. However, if we go that route, we'd need to allow the parser to accept a parameter like includeEntitiesVersion, which could complicate the implementation.

Are you perhaps considering a parser that accepts a list of expected metadata and returns only those values? My thought was more along the lines of a parser that returns all metadata when available. I don’t believe there would be a notable performance difference between the two approaches.

[lognaturel]

Here's an example of what parsing out metadata without using JavaRosa might look like:

collect/collect_app/src/androidTest/java/org/odk/collect/android/support/StubOpenRosaServer.java

Line 319 in 66e9c1f

private void addFormFromInputStream(String formXML, List<MediaFileItem> mediaFiles, InputStream formDefStream) {

That code gets the formid, the version and the title which is all that's strictly needed to display forms in form lists. I thought it was a streaming parser and stopped once it got everything it needed but no, it does build the full document tree in memory so there are definitely further performance improvement possible.

If we replace FileUtils.getMetadataFromFormDefinition with that implementation, then processing a form with a too-new Entity spec version after download should succeed and the form should be correctly displayed. Then, when we open the form, we should see the UnrecognizedEntityVersionException from JavaRosa/EntityFormProcessor. Maybe that would be a good first spike to validate the approach.

If that works as desired, then the question is what to do with metadata that isn't needed for display in form lists and in particular with "geometry XPath" which is more complex than just reading a specific element.

Hopefully this helps clarify the concept. If not, let's get on a quick call to discuss.