From 4bbd1c000de7465192ff066ae65773fb4e51544f Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:38:08 +0000 Subject: [PATCH 1/3] make tests more specific --- .../xpath/expr/RandomizeTypesTest.java | 73 ++++++++++++++----- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java index 091df0c4a..f7257733f 100644 --- a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java +++ b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java @@ -30,21 +30,35 @@ public void stringNumberSeedConvertsWhenUsedInNodesetExpression() throws IOExcep title("Randomize non-numeric seed"), model( mainInstance(t("data id=\"rand-non-numeric\"", - t("choice") + t("choices_numeric_seed"), + t("choices_stringified_numeric_seed") )), instance("choices", item("a", "A"), - item("b", "B") + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") ), - bind("/data/choice").type("string") + bind("/data/choices_numeric_seed").type("string"), + bind("/data/choices_stringified_numeric_seed").type("string") ) ), body( - select1Dynamic("/data/choice", "randomize(instance('choices')/root/item, '1')") + select1Dynamic("/data/choices_numeric_seed", "randomize(instance('choices')/root/item, 1234)"), + select1Dynamic("/data/choices_stringified_numeric_seed", "randomize(instance('choices')/root/item, '1234')") ) )); - - assertThat(scenario.choicesOf("/data/choice").get(0).getValue(), is("b")); + String[] shuffled = {"g", "f", "e", "d", "a", "h", "b", "c"}; + String[] nodes = {"/data/choices_numeric_seed", "/data/choices_stringified_numeric_seed"}; + for (int i = 0; i < shuffled.length; i++) { + for (String node : nodes) { + assertThat(scenario.choicesOf(node).get(i).getValue(), is(shuffled[i])); + } + } } @Test @@ -54,21 +68,30 @@ public void stringNumberSeedConvertsWhenUsedInCalculate() throws IOException, XF title("Randomize non-numeric seed"), model( mainInstance(t("data id=\"rand-non-numeric\"", - t("choice") + t("choices_numeric_seed"), + t("choices_stringified_numeric_seed") )), instance("choices", item("a", "A"), - item("b", "B") + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") ), - bind("/data/choice").type("string").calculate("selected-at(join(' ', randomize(instance('choices')/root/item/label, '1')), 0)") + bind("/data/choices_numeric_seed").type("string").calculate("join('', randomize(instance('choices')/root/item/label, 1234))"), + bind("/data/choices_stringified_numeric_seed").type("string").calculate("join('', randomize(instance('choices')/root/item/label, '1234'))") ) ), body( - input("/data/choice") + input("/data/choices_numeric_seed"), + input("/data/choices_stringified_numeric_seed") ) )); - - assertThat(scenario.answerOf("/data/choice").getDisplayText(), is("B")); + assertThat(scenario.answerOf("/data/choices_numeric_seed").getDisplayText(), is(scenario.answerOf("/data/choices_stringified_numeric_seed").getDisplayText())); + assertThat(scenario.answerOf("/data/choices_numeric_seed").getDisplayText(), is("GFEDAHBC")); } @Test @@ -82,7 +105,13 @@ public void stringTextSeedConvertsWhenUsedInNodesetExpression() throws IOExcepti )), instance("choices", item("a", "A"), - item("b", "B") + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") ), bind("/data/choice").type("string") ) @@ -91,8 +120,10 @@ public void stringTextSeedConvertsWhenUsedInNodesetExpression() throws IOExcepti select1Dynamic("/data/choice", "randomize(instance('choices')/root/item, 'foo')") ) )); - - assertThat(scenario.choicesOf("/data/choice").get(0).getValue(), is("b")); + String[] shuffled = {"e", "a", "d", "b", "h", "g", "c", "f"}; + for (int i = 0; i < shuffled.length; i++) { + assertThat(scenario.choicesOf("/data/choice").get(i).getValue(), is(shuffled[i])); + } } @Test @@ -106,9 +137,15 @@ public void stringTextSeedConvertsWhenUsedInCalculate() throws IOException, XFor )), instance("choices", item("a", "A"), - item("b", "B") + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") ), - bind("/data/choice").type("string").calculate("selected-at(join(' ', randomize(instance('choices')/root/item/label, 'foo')), 0)") + bind("/data/choice").type("string").calculate("join('', randomize(instance('choices')/root/item/label, 'foo'))") ) ), body( @@ -116,7 +153,7 @@ public void stringTextSeedConvertsWhenUsedInCalculate() throws IOException, XFor ) )); - assertThat(scenario.answerOf("/data/choice").getDisplayText(), is("B")); + assertThat(scenario.answerOf("/data/choice").getDisplayText(), is("EADBHGCF")); } @Test From 0458a56c9a8f6923b998732fcb9a9a09c9904812 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Tue, 22 Oct 2024 15:23:38 +0000 Subject: [PATCH 2/3] explicitly test randomization seed provided through an input --- .../xpath/expr/RandomizeTypesTest.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java index f7257733f..9863f52c2 100644 --- a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java +++ b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java @@ -192,4 +192,41 @@ public void seedInRepeatIsEvaluatedForEachInstance() throws IOException, XFormPa assertThat(scenario.choicesOf("/data/repeat[2]/choice").get(0).getValue(), is("b")); assertThat(scenario.choicesOf("/data/repeat[1]/choice").get(0).getValue(), is("a")); } + + @Test + public void seedFromArbitraryInputCanBeUsed() throws IOException, XFormParser.ParseException { + Scenario scenario = Scenario.init("Randomize non-numeric seed", html( + head( + title("Randomize non-numeric seed"), + model( + mainInstance(t("data id=\"rand-non-numeric\"", + t("input"), + t("choice") + )), + instance("choices", + item("a", "A"), + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") + ), + bind("/data/input").type("geopoint"), + bind("/data/choice").type("string") + ) + ), + body( + input("/data/input"), + select1Dynamic("/data/choice", "randomize(instance('choices')/root/item, /data/input)") + ) + )); + + scenario.answer("/data/input", "-6.8137120026589315 39.29392995851879"); + String[] shuffled = {"h", "b", "d", "f", "a", "g", "c", "e"}; + for (int i = 0; i < shuffled.length; i++) { + assertThat(scenario.choicesOf("/data/choice").get(i).getValue(), is(shuffled[i])); + } + } } From d6db3f8abd116c698db61262ae66e5bbca4b227a Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:43:00 +0000 Subject: [PATCH 3/3] preserve classic seed-derivation behaviour when using the empty string as a seed --- .../javarosa/xpath/expr/XPathFuncExpr.java | 8 ++- .../xpath/expr/RandomizeTypesTest.java | 58 +++++++++++++++++++ .../xpath/expr/XPathFuncAsSomethingTest.java | 2 +- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java b/src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java index 6f11159ef..1484a36ef 100644 --- a/src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java +++ b/src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java @@ -705,12 +705,18 @@ public static Double toDouble(Object o) { } /** - * convert a string value to an integer by: + * Convert a non-zero-length string value to an integer by: * - encoding it as utf-8 * - hashing it with sha256 (available cross-platform, including via browser crypto API) * - interpreting the first 8 bytes of the hash as a long + * + * A zero-length string results in a 0L — this is the classic behaviour that we want + * to conserve for backward compatibility with a case that is expected to be common. + * In practical terms, we don't want the sort order of choice lists using an empty string as + * a seed to change with the introduction of this seed derivation mechanism. */ public static long toLongHash(String sourceString) { + if (sourceString.length() == 0) return 0L; byte[] hasheeBuf = sourceString.getBytes(Charset.forName("UTF-8")); SHA256Digest hasher = new SHA256Digest(); hasher.update(hasheeBuf, 0, hasheeBuf.length); diff --git a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java index 9863f52c2..7bd034639 100644 --- a/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java +++ b/src/test/java/org/javarosa/xpath/expr/RandomizeTypesTest.java @@ -5,6 +5,7 @@ import org.junit.Test; import java.io.IOException; +import java.util.Arrays; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -229,4 +230,61 @@ public void seedFromArbitraryInputCanBeUsed() throws IOException, XFormParser.Pa assertThat(scenario.choicesOf("/data/choice").get(i).getValue(), is(shuffled[i])); } } + + @Test + public void seed0FromNaNs() throws IOException, XFormParser.ParseException { + Scenario scenario = Scenario.init("Randomize non-numeric seed", html( + head( + title("Randomize non-numeric seed"), + model( + mainInstance(t("data id=\"rand-non-numeric\"", + t("input_emptystring"), + t("input_somestring"), + t("input_int"), + t("choice_emptystring"), + t("choice_somestring"), + t("choice_int") + )), + instance("choices", + item("a", "A"), + item("b", "B"), + item("c", "C"), + item("d", "D"), + item("e", "E"), + item("f", "F"), + item("g", "G"), + item("h", "H") + ), + bind("/data/input_emptystring").type("string"), + bind("/data/input_somestring").type("string"), + bind("/data/input_int").type("int") + ) + ), + body( + input("/data/input_emptystring"), + input("/data/input_somestring"), + input("/data/input_int"), + select1Dynamic("/data/choice_emptystring", "randomize(instance('choices')/root/item, /data/input_emptystring)"), + select1Dynamic("/data/choice_somestring", "randomize(instance('choices')/root/item, /data/input_somestring)"), + select1Dynamic("/data/choice_int", "randomize(instance('choices')/root/item, /data/input_int)") + ) + )); + + scenario.answer("/data/input_emptystring", ""); + scenario.answer("/data/input_somestring", "somestring"); + scenario.answer("/data/input_int", "0"); + + String[] shuffled_NaN_or_0 = {"c", "b", "h", "a", "f", "d", "g", "e"}; + String[] shuffled_somestring = {"e", "b", "c", "g", "d", "a", "f", "h"}; + assertThat("somestring-seeded expected order is distinct from 0-seeded expected order", !Arrays.equals(shuffled_NaN_or_0, shuffled_somestring)); + String[] shuffledfields = {"/data/choice_emptystring", "/data/choice_int"}; + for (int i = 0; i < shuffled_NaN_or_0.length; i++) { + for (String shuffledfield : shuffledfields) { + assertThat(scenario.choicesOf(shuffledfield).get(i).getValue(), is(shuffled_NaN_or_0[i])); + } + } + for (int i = 0; i < shuffled_somestring.length; i++) { + assertThat(scenario.choicesOf("/data/choice_somestring").get(i).getValue(), is(shuffled_somestring[i])); + } + } } diff --git a/src/test/java/org/javarosa/xpath/expr/XPathFuncAsSomethingTest.java b/src/test/java/org/javarosa/xpath/expr/XPathFuncAsSomethingTest.java index 03aa535d1..5ba3f65e7 100644 --- a/src/test/java/org/javarosa/xpath/expr/XPathFuncAsSomethingTest.java +++ b/src/test/java/org/javarosa/xpath/expr/XPathFuncAsSomethingTest.java @@ -15,7 +15,7 @@ public class XPathFuncAsSomethingTest { @Test public void toLongHashHashesWell() { assertThat(toLongHash("Hello"), equalTo(1756278180214341157L)); - assertThat(toLongHash(""), equalTo(-2039914840885289964L)); + assertThat(toLongHash(""), equalTo(0L)); // the empty string would actually hash to -2039914840885289964L; but we've added this quirk for backward compatibility. } @Test