Fixed deserialization of forms containing selects with default answers

[grzesiek2010]

Closes #getodk/collect#6571

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

I've added an automated test and performed manual tests based on the steps described in the issue.

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

According to my findings, the issue stems from setting the instance name for tree elements in the form instance after deserialization. When reading the form from XML, everything works correctly.

According to the provided reference:

For elements in the primary instance, the instance name should be null. However, during deserialization, when attempting to read the reference (ref) of a select-one question located in the primary instance, the reference appears as:

instance(Internal choices)/data/select-from-secondary-instance

In this case, the instance name is mistakenly set to the ID of a child in the primary instance. According to the comment in the reference, the instance name should be null, and the reference should instead be (exactly as it appears when we read form from XML):

/data/select-from-secondary-instance

This difference in TreeReferences triggers an exception because the expected question cannot be found. I believe it worked correctly for a long time only because the equals method used for comparing references was faulty. Once that method was fixed in [this pull request](#578), the issue began to occur.

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?

It shouldn't affect users at all. It should only fix the problem with the deserialization of forms containing selects with default answers.

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

The form is attached to the issue.

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

No.

[grzesiek2010]

It would be ideal to have a test that fails when checking the instance name or reference of the select_one question. However, the issue occurs only after deserialization, and before any validation can be performed, the deserialization process throws an exception.

[lognaturel]

This is an integration test and that feels ok. I wish the test didn't end with _worksCorrectly but I also don't have a great alternate suggestion so I'm inclined to go with it!

[lognaturel]

Wow, this is a blast from the past.

I couldn't immediately figure out how your description as related to default values so I debugged the test myself. I now see that when there's a default value, there's a block in attachControlsToInstanceData for going from the serialized selected option to the full internal representation of that option. This requires the reference to the question which is broken after deserialization for the reasons you describe.

I don't really understand why and how the name for the FormInstance is being set (e.g. to "Internal choices" in this case). This troubles me but I'm inclined not to dig deeper unless you think it's important, @grzesiek2010.

[grzesiek2010]

I don't really understand why and how the name for the FormInstance is being set (e.g. to "Internal choices" in this case). This troubles me but I'm inclined not to dig deeper unless you think it's important, @grzesiek2010.

I don't fully understand why it was implemented that way, so I can't be completely confident that the fix is safe. Based on my tests and debugging, I'm pretty sure it is, but I wanted to check if you might know something about it.

There is a difference between TreeElement#instanceName:

and DataInstance#name:

(instanceName should be null for primary instances but name should take the form title in such cases)

so setting TreeElement#instanceName using DataInstance#name in #setRoot seemed to me like an obvious bug but maybe there was some rationale behind it.