Expose created entities after finalizing form

[seadowg]

Work towards getodk/collect#5231
Blocked by #692

See follow-up in #694, #697

A form with an entities:create element will now have an entity exposed (built from the saveto attribute questions) through the FormEntryModel after finalization:

formEntryController.finalizeFormEntry()
val entities = formEntryController.getModel().getExtras().get(Entities.class).getEntities()

Instead of being included in the standard parse/runtime code, the entity features are available as a "plugin" that's added by wrapping the XFormParserFactory and adding a FormPostProcessor to FormEntryController:

XFormUtils.setXFormParserFactory(EntityXFormParserFactory(XFormParserFactory()))
val controller = FormEntryController(XFormUtils.getFormFromFormXml("myForm.xml", null)).also {
    it.addPostProcessor(EntityFormFinalizationProcessor())
}

For all this to work, the PR adds a few features to allow these kinds of "plugins":

• XFormParser can have BindAttributeProcessor, FormDefProcessor and ModelAttributeProcessor objects added to it to allow a plugin to inspect attributes on bind elements, the model attributes and to inspect parsed FormDef objects respectively
• Arbitrary data can be attached to and retrieved from FormDef objects through getExtras
• Extras on FormDef are retained when through the serialization process that clients uses for caching (using writeExternal/readExternal)
• Data can also be attached to FormEntryModel through model with a similar interface (although this data is not ready for serialization as the FormEntryModel is not expected to be serialized)
• As shown in the above example, post processors (run as form finalization) can be added to FormEntryController through addPostProcessor
• XFormParser.parse now throws a specific (non-runtime) exception and the old XFormParseException has been deprecated (it seemed like too much work to remove this as part of the PR)
• The above change meant a lot of files were touched to change the throws declaration on test methods

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

New tests.

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

We initially looked at a solution that just added entity parsing/processing into existing JavaRosa classes. This felt like it was adding a lot of noise, which is why we experimented with trying out a more "plugin" oriented approach. I really like where this has ended up - it should let us extract other features in similar ways (like geometry) which will help us declutter XFormParser.

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?

Should just add the new feature! Very little existing code was changed.

Do we need any specific form for testing your changes? If so, please attach one.

Entity forms (there is a spec link in the issue).

Does this change require updates to documentation? If so, please file an issue here and include the link below.

I imagine we will want to update docs, but not sure we're ready quite yet.

[seadowg]

@lognaturel I'd like to do some tidying up on the code and have a bit of a deeper think about the touchpoints between Collect and JavaRosa. It would be good to get initial feedback on this before that though.

[lognaturel]

I think this is a very fine spike!

One high-level feeling I have looking at this is I wish it weren't so entangled with existing machinery. I think of the entities concept as a wrapper/layer around the existing ODK XForms spec. It would be really nice if the code could reflect that and not touch everything. On the other hand, given that we've decided to add this layer as part of the form spec I don't see a good way to achieve that goal without entirely sacrificing performance.

[lognaturel]

Maybe split out the API checker change? Should be an easy merge once you get it working.

[lognaturel]

I really love how the separation turned out! It's really nice to have all entities-related implementation in one place and it feels like the new hooks could provide broader value.

I think the theme in my feedback is "naming is hard" but particularly it's helpful if we can use names that clearly identify whether something is an abstract declaration (an entity declaration, a form def) vs a concrete thing (an actual built entity, a filled form instance).

[lognaturel]

What's the relationship between this and ExtWrapBase? Seems they're nearly identical? Should this maybe extend ExtWrapBase or is it possible/reasonable to use ExtWrapBase directly?

[seadowg]

Ah I hadn't seen ExtWrapBase - will have a look at it.

[seadowg]

ExtWrapBased doesn't "externalize" the object's type meaning that it must be known at compile time. This doesn't work when externalizing a value that is being referenced by a super type like with Extras.

[lognaturel]

To add: 💥 if spec version doesn't match -- https://docs.google.com/document/d/1-O_wT0JZZFaWvzHl_krrzERJcnrdGMKP55ZZ_hxV0gU/edit#heading=h.pilfnyfgdjnn

[lognaturel]

I'm always terrified to see any kind of nested iteration but I guess we don't really care too much about form parse time (within reason) and we generally expect at most a handful of attributes and processors.

[lognaturel]

You don't want to add any kind of assertion here? I think this is fine but I feel like I've seen you throw back tests like this so want to give you a chance to consider it again in case you do care.

[seadowg]

I guess it came from writing these out in order (in a TDD red-green cycle). The only needed a test where something didn't explode to get the code where I wanted, but you're right that it's a bit confusing to read after the fact. I'll add a "this is working as expected" assertion.

[lognaturel]

The new naming really helps pull things together for me. Thanks for investing in it. Liking the way some other things have moved, too.