Add support for pulldata with local entities #6451

seadowg · 2024-10-10T14:00:40Z

Closes #6443
~~Blocked by getodk/javarosa#802~~

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

I'd initially wanted to implement this by just executing a dynamically built XPath expression, but this proved tricky as JavaRosa currently doesn't attach secondary instances that aren't referenced by an instance expression. This would mean that a form that contained only a pulldata referencing an entity list wouldn't work (as the external instance would never be parsed and attached to the main instance). We'll look to move to the originally intended implementation in the next release as part of #6454.

Instead, I've ended up using the LocalEntityInstanceAdapter in a similar way to LocalEntityFilterStrategy with modifications so that it now supports querying any child of an entity item (like label for example). This new support doesn't add anything significant in the way of optimisation however: queries for children like label work by just loading every entity in the list from the DB and then performing an in-mem filter. This limits the amount of work we do to just support pulldata.

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?

The big risks here will be to all pulldata forms (including ones that use with entities obviously) and to entity filters in forms in general.

One thing to note: I've not extended the instance "normalising" that pulldata does to entity lists. For normal secondary instances, pulldata would let you use the wrong case for the name or add .csv to the end. We'll discuss if we want to maintain this (currently undocumented) behaviour separately.

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

grzesiek2010 · 2024-10-17T17:07:33Z

entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt

+        val filterValue = XPathFuncExpr.toString(args[3])
+
+        return if (instanceAdapter.supportsInstance(instanceId)) {
+            instanceAdapter.queryEq(instanceId, filterChild, filterValue)!!.firstOrNull()


It looks like queryEq can't return null values so we could update that function by removing ? from the returned type and then get rid of those !!.

grzesiek2010 · 2024-10-17T17:09:36Z

...ties/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt

@@ -33,62 +33,82 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos
            }
        } else {
            entitiesRepository.getEntities(instanceId).map { entity ->
-                convertToElement(entity, false)
+                convertToElement(entity)
            }
        }
    }

    fun queryEq(instanceId: String, child: String, value: String): List<TreeElement>? {
        return when {


You can now simplify this by adding child as a subject of when:

return when (child) { EntityItemElement.ID ->...

grzesiek2010 · 2024-10-17T17:21:42Z

entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt

+
+    override fun eval(args: Array<Any>, ec: EvaluationContext): Any {
+        val instanceId = XPathFuncExpr.toString(args[0])
+        val child = XPathFuncExpr.toString(args[1])


I think you can move extracting child, filterChild and filterValue down and call only if instanceAdapter.supportsInstance(instanceId) returns true.

grzesiek2010 · 2024-10-17T18:46:00Z

...ties/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt

@@ -23,7 +23,7 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos

                0.until(count).map {
                    if (it == 0) {
-                        convertToElement(first, true)
+                        convertToElement(first)
                    } else {
                        TreeElement("item", it, true)


Now I wonder if we need that isPartial flag at all... such elements should have at least label and value right so if they don't have it we could treat them as partial and try to populate.

The partial concept is currently too general for something like that as any TreeElement can be "partial". It's not confined to secondary instances with item children.

grzesiek2010 · 2024-10-17T18:47:43Z

...ties/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt

@@ -23,7 +23,7 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos

                0.until(count).map {
                    if (it == 0) {
-                        convertToElement(first, true)


Now I wonder if we need that isPartial flag at all... such elements should have at least label and value right so if they don't have it we could treat them as partial and try to populate.
It is set to true in only one place.

#6451 (comment)

grzesiek2010 · 2024-10-17T18:52:22Z

...src/test/java/org/odk/collect/entities/javarosa/instance/LocalEntitiesInstanceAdapterTest.kt

+
+        val instanceAdapter = LocalEntitiesInstanceAdapter(entitiesRepository)
+        val results = instanceAdapter.queryEq("things", EntityItemElement.LABEL, "Thing 2")
+        assertThat(results!!.size, equalTo(1))


!! is unnecessary here. The same in other tests.

lognaturel

I've taken a quick look through and discussed briefly with @grzesiek2010 who says his comments could be addressed in follow-up. I'm going to merge this now so it can be part of our next beta to maximize time to get user feedback.

seadowg · 2024-10-18T08:12:22Z

@lognaturel @grzesiek2010 I'll add an issue to follow up.

WKobus · 2024-10-22T12:53:02Z

Tested with success

Verified on device with Android 15

Verified cases:

Entity update forms with pulldata
Entity select forms with pulldata
Pulldata in non-entity forms
Entiy forms with filters

dbemke · 2024-10-22T12:53:38Z

Tested with success

Verified on device with Android 10

Address comments from #6451

seadowg added 3 commits October 10, 2024 12:49

Switch to using fake serer in pulldata test

63ded83

Add failing test for pulldata with local entities

b463ad7

Use EntitiesInstanceAdapter to implement pulldata

bed7c86

seadowg force-pushed the entities-pulldata branch 2 times, most recently from 7eb396e to bed7c86 Compare October 14, 2024 09:10

seadowg added 6 commits October 14, 2024 12:57

Add tests for pulldata function behaviour

7eae24d

Correct variable names

880a2e4

Add unoptimized support for label to queryEq

92c0a9c

Add unoptimized support for version to queryEq

19d52ad

Add unoptimized support for branch ID and trunk version to queryEq

471f015

Pull out shared for unoptimized entity eq queries

9c2a5ca

seadowg marked this pull request as ready for review October 14, 2024 13:39

seadowg requested a review from grzesiek2010 October 14, 2024 13:39

seadowg added the blocked label Oct 15, 2024

Simplify partial parsing for entities

78b30c6

seadowg force-pushed the entities-pulldata branch from 5beb718 to 78b30c6 Compare October 17, 2024 13:19

seadowg removed the blocked label Oct 17, 2024

seadowg mentioned this pull request Oct 17, 2024

Update to latest JavaRosa #6462

Merged

lognaturel mentioned this pull request Oct 17, 2024

Pulldata treats ".” and "_” as the same symbol in entity properties #6461

Closed

grzesiek2010 reviewed Oct 17, 2024

View reviewed changes

lognaturel approved these changes Oct 17, 2024

View reviewed changes

lognaturel merged commit 4603e67 into getodk:master Oct 17, 2024
6 checks passed

lognaturel mentioned this pull request Oct 17, 2024

pulldata should support local entities #6443

Closed

seadowg deleted the entities-pulldata branch October 18, 2024 08:11

seadowg mentioned this pull request Oct 18, 2024

Address review comments for PR 6451 #6463

Closed

grzesiek2010 added the needs testing label Oct 18, 2024

dbemke added behavior verified and removed needs testing labels Oct 22, 2024

seadowg mentioned this pull request Oct 23, 2024

Address comments from #6451 #6469

Merged

lognaturel mentioned this pull request Oct 23, 2024

Don't create a search/pulldata database when pulldata is used over an Entity List #6471

Open

grzesiek2010 added a commit that referenced this pull request Oct 24, 2024

Merge pull request #6469 from seadowg/address-6451

4252f94

Address comments from #6451

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pulldata with local entities #6451

Add support for pulldata with local entities #6451

seadowg commented Oct 10, 2024 •

edited

Loading

grzesiek2010 Oct 17, 2024

grzesiek2010 Oct 17, 2024

grzesiek2010 Oct 17, 2024

grzesiek2010 Oct 17, 2024

seadowg Oct 23, 2024

grzesiek2010 Oct 17, 2024

seadowg Oct 23, 2024

grzesiek2010 Oct 17, 2024

lognaturel left a comment

seadowg commented Oct 18, 2024

WKobus commented Oct 22, 2024

dbemke commented Oct 22, 2024

Add support for pulldata with local entities #6451

Add support for pulldata with local entities #6451

Conversation

seadowg commented Oct 10, 2024 • edited Loading

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

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?

Before submitting this PR, please make sure you have:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

seadowg commented Oct 18, 2024

WKobus commented Oct 22, 2024

dbemke commented Oct 22, 2024

seadowg commented Oct 10, 2024 •

edited

Loading