Add ability to identify functions in expressions and appearances on questions

[seadowg]

Work towards getodk/collect#5620

This change allows a client add a processor to XFormParser that would allow it to detect the presence of particular functions or properties of a question:

xFormParser.addProcessor(object : BindAttributeProcessor, QuestionProcessor {
    override fun getBindAttributes(): Set<Pair<String, String>> {
        return setOf(Pair("", "calculate"))
    }

    override fun processBindAttribute(name: String, value: String, binding: DataBinding) {
        val calculate = binding.calculate
        val containsFunc = calculate.expr.expr.containsFunc("myFunc")
    }

    override fun processQuestion(question: Question) {
        val containsAppearance = question.appearanceAttr.contains("myCustomAppearance")
    }
})

This will let us detect forms that do/don't contain pulldata or search.

I've also added a convenience implementation of a wrapper IXFormParserFactory (which will make Collect's code cleaner).

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 could have used the existing BindAttributeProcessor, but that would have meant parsing calculate in the processor which didn't feel ideal.

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?

No user facing changes as this is opt in.

[seadowg]

Marking this as a draft for the moment. I think we'll also need a way to detect search in appearances.

[seadowg]

We could potentially just remove this, but given someone could theoretically be using it, we'd need to a major version bump.

[lognaturel]

I'm having trouble understanding the BindAttributeProcessor -> BindProcessor change. Currently BindAttributeProcessor makes it possible to create a processor for a specific bind attribute and namespace. It feels like creating a processor for calculate in the same way would be fine and good. I'm not seeing any advantage to the BindProcessor approach.

[seadowg]

I'd written a comment in the PR description about this:

We could have used the existing BindAttributeProcessor, but that would have meant parsing calculate in the processor which didn't feel ideal.

Does that make sense? Happy to discuss further. I actually couldn't remember why I made this choice until I read that!

[lognaturel]

Is this saying that the value for the calculate attribute is raw so it would require parsing? I think it would be ok for a BindAttributeProcessor to use the DataBinding that it's given to access the parsed calculate and ignore the value. Maybe BindAttributeProcessor could get a processBindAttribute(String name, DataBinding binding) method that's preferred for attributes that are already parsed into the DataBinding?

[seadowg]

Is this saying that the value for the calculate attribute is raw so it would require parsing?

Yeah exactly. The BindAttributeProcessor would have to parse the contents of calculate (which would just be a string).

I think it would be ok for a BindAttributeProcessor to use the DataBinding that it's given to access the parsed calculate and ignore the value. Maybe BindAttributeProcessor could get a processBindAttribute(String name, DataBinding binding) method that's preferred for attributes that are already parsed into the DataBinding?

Ah I guess it could! I think I still prefer having a BindProcessor as it just feels simpler to me. You hook into every bind and can make your decision about what you want to do rather than having to implement (and understand) getBindAttributes and processBindAttribute. Are you just feeling that the change isn't worth the disruption? I think you've got me on the fence now so I can definitely be pushed 😆.

[seadowg]

I'll investigate implementing this with BindAttributeProcessor.

[seadowg]

I don't know why this was important? Happy to be corrected, but I don't see any reason to not allow accessing saveto from DataBinding.

[lognaturel]

Looks like it was introduced at the very beginning in #691. I don't see a specific explanation but in my head it makes sense to want confidence that entity directives are handled by the JR processor and not available for clients to muck with. I see it as a nice way to enforce hygiene which is something that Collect has had trouble with in the past.

[seadowg]

That's fair. I'm not feeling as concerned about that now. Introducing these processors has given us a structured way to interact with form parsing, and my gut reaction is that removing attributes feels overly protective.

[lognaturel]

Sounds reasonable to me!

[lognaturel]

We could have used the existing BindAttributeProcessor, but that would have meant parsing calculate in the processor which didn't feel ideal.

As I wrote in a comment, I'm not understanding this. processBindAttribute takes in a DataBinding. The only real difference with the new BindProcessor that I can see is that BindAttributeProcessor specifies which attribute it cares about. Maybe it feels weird to iterate over all the attributes? But I think the alternative for attributes like saveto or the recording ones would be to iterate through TreeElements (additionalAttrs) which feels worse.

Other than that, the biggest risk that I see is that there could be something missing in the containsFunc implementations. I spent some time looking at the tests and implementations and they look complete to me.