Fix handling of numbers in SQLite query arguments #6619

grzesiek2010 · 2025-02-20T18:59:30Z

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

When filters include numeric values like age=25, JavaRosa represents the number as a Double (25.0). However, SQLite requires arguments to be strings. To ensure correct comparisons in the database, we need to check if the Double value has a .0 fractional part. If it does, we should first convert it to an Int and then to a String, resulting in 25 instead of 25.0. This prevents issues caused by the redundant decimal part during value comparisons.

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?

We need to test filters with numbers like age=25. The form that is attached to the issue contains such a filter and that was the cause of the crash.

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

The from that is attached to the issue can be used.

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

No.

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

lognaturel · 2025-02-20T20:04:52Z

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

@@ -87,7 +87,13 @@ class LocalEntitiesFilterStrategy(entitiesRepository: EntitiesRepository) :

        return if (candidate != null) {
            val child = candidate.nodeSide.steps[0].name.name
-            val value = candidate.evalContextSide(sourceInstance, evaluationContext) as String
+            var value = candidate.evalContextSide(sourceInstance, evaluationContext)


Where does the type of value come from? Could it also be any other type like maybe a date type or boolean?

In the filter it is just a number like age=25 but JR keeps numbers as doubles so it is 25.0. Before converting it to string we need to get rid of that .0 which I described above.

Could it also be any other type like maybe

Yes I was thinking about dates as well. I'm going to check that.

Great, thanks. My recollection is that this was addressed for the simple expression case. I would have expected the same tests related to that to catch this. Is there branching between the simple expression and complex expression (with and/or) case? Am I remembering incorrectly about those tests?

My recollection is that this was addressed for the simple expression case. I would have expected the same tests related to that to catch this. Is there branching between the simple expression and complex expression (with and/or) case? Am I remembering incorrectly about those tests?

It's not clear to me what you're referring to here @lognaturel. What's the simple expression case?

I distinctly remember a conversation you and I had about doubles when you were first writing support for expressions without and/or. I thought this class of problems was addressed there but I see that the form crashes in v2024.3.x too so maybe not?

I distinctly remember a conversation you and I had about doubles when you were first writing support for expressions without and/or. I thought this class of problems was addressed there but I see that the form crashes in v2024.3.x too so maybe not?

Yeah, I don't remember considering it at this level sadly!

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

seadowg · 2025-02-24T08:42:15Z

db/src/main/java/org/odk/collect/db/sqlite/SqlQuery.kt

-        is Query.Eq -> SqlQuery("$column = ?", arrayOf(value))
-        is Query.NotEq -> SqlQuery("$column != ?", arrayOf(value))
+        is Query.Eq -> {
+            if (value.toDoubleOrNull() != null) {


The general approach here is that if we have a "numeric" (a String that looks like an Int or a Double) value for the query, we convert both to double so we end up with essentially Double = Double right?

One thing that feels off to me is that we'll explicitly have the type (an Int or a Double) in LocalEntitiyFilterStrategy and then we're "erasing" it by converting to a String and then reconverting based on a heuristic. Instead could we add DoubleEq and DoubleNotEq types to Query (I guess the existing ones would be StringEq)? Then we'd just lift Int to Double when creating a Query and toSql wouldn't have to make any guesses.

The test case to write to drive out the above I'm realizing is a filter expression like id = '1234'. This implementation would match an id value of '1234.0' (as both get converted to doubles) whereas only the exact string '1234' should match.

seadowg

Liking this! There's one point I'd like to discuss, but I think we're ok on (the assumption that everything is a `Double), and I think we should make one test tweak. This is definitely ready to hit QA though so I'll add "needs testing".

seadowg · 2025-02-25T11:32:14Z

...es/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt

+                body(
+                    select1Dynamic(
+                        "/data/question",
+                        "instance('things')/root/item[age=25]",


I was a little concerned about only having one test here as my instinct was that we need to consider cases for both Double and Int. However, as far as I can see, JavaRosa always treats numbers as Double types for the purposes of evaluation:

IntegerData carries an Int around, but when a path expression is unpacked (using #unpack) for evaluation it gets converted to a Double

XPathNumericLiteral carries a Double around so integer literals never end up as Int

I was aware of 2, but 1 surprised me. Does this blanket statement around not needing to worry about Int seem true @lognaturel? If so, the nice thing is this single test is enough as all "numeric" data will have the same type.

seadowg · 2025-02-25T11:34:56Z

db/src/test/java/org/odk/collect/db/sqlite/SqlQueryTest.kt

@@ -8,24 +8,24 @@ import org.odk.collect.shared.Query
 class SqlQueryTest {


I'm thinking that this test should really be more of an "integration" one and check queries against a temp SQLite DB. That's a change we can follow up with.

seadowg · 2025-02-25T11:36:54Z

collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt

+    }
+
+    @Test
+    fun `#query returns matching entities with numeric eq selection arguments`() {


I'd like to split these 2 tests into 4 so we have individual ones for int and decimal property values.

seadowg · 2025-02-25T12:03:30Z

Just as a note for testing: I think looking at filters that use numbers (both int and decimal) is obviously the first thing to check, but I think checking other "unusual" types like dates would be a good idea as well.

lognaturel reviewed Feb 20, 2025

View reviewed changes

grzesiek2010 requested a review from seadowg February 20, 2025 20:20

grzesiek2010 marked this pull request as ready for review February 20, 2025 20:20

grzesiek2010 added the high priority Should be looked at before other PRs/issues label Feb 20, 2025

seadowg requested changes Feb 21, 2025

View reviewed changes

entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt Outdated Show resolved Hide resolved

grzesiek2010 force-pushed the COLLECT-6617 branch from 05fc3fe to 8339251 Compare February 21, 2025 22:38

grzesiek2010 requested a review from seadowg February 21, 2025 23:13

grzesiek2010 force-pushed the COLLECT-6617 branch from 8339251 to bab2345 Compare February 21, 2025 23:17

seadowg requested changes Feb 24, 2025

View reviewed changes

grzesiek2010 requested a review from seadowg February 24, 2025 19:30

grzesiek2010 added 5 commits February 24, 2025 20:31

Added a failing test

56024f6

Fix handling of numbers in SQLite query arguments

30dcce5

Fix SQL queries with numeric selection arguments

671385d

Introduced NumericEq and NumericNotEq queries

eddbdc3

Naming improvements

9869ff7

grzesiek2010 force-pushed the COLLECT-6617 branch from 9aaec8f to 9869ff7 Compare February 24, 2025 19:32

seadowg requested changes Feb 25, 2025

View reviewed changes

seadowg added the needs testing label Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of numbers in SQLite query arguments #6619

Fix handling of numbers in SQLite query arguments #6619

grzesiek2010 commented Feb 20, 2025 •

edited

Loading

lognaturel Feb 20, 2025

grzesiek2010 Feb 20, 2025

grzesiek2010 Feb 20, 2025

lognaturel Feb 20, 2025

seadowg Feb 21, 2025

lognaturel Feb 24, 2025

seadowg Feb 24, 2025

seadowg Feb 24, 2025

seadowg Feb 24, 2025

grzesiek2010 Feb 24, 2025

seadowg left a comment

seadowg Feb 25, 2025

seadowg Feb 25, 2025

seadowg Feb 25, 2025

seadowg commented Feb 25, 2025

		@@ -8,24 +8,24 @@ import org.odk.collect.shared.Query
		class SqlQueryTest {

Fix handling of numbers in SQLite query arguments #6619

Are you sure you want to change the base?

Fix handling of numbers in SQLite query arguments #6619

Conversation

grzesiek2010 commented Feb 20, 2025 • 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?

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a 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

seadowg commented Feb 25, 2025

grzesiek2010 commented Feb 20, 2025 •

edited

Loading