Add base64-decode #708

lognaturel · 2023-02-10T21:58:13Z

Discussion at https://forum.getodk.org/t/proposal-add-centroid-and-base64binary-to-string-functions/40289/5

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

Added tests.

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

Name was selected with community input. Since we're not following an existing spec, I decided not to add an encoding parameter. In practice, most things are UTF-8 these days. We can add an encoding parameter later if needed.

We had talked about throwing an exception in case of invalid input but I decided to use a blank/null value instead. The problem with throwing an exception is that the exception can manifest as a Validate error and/or be thrown at undesirable moment if the input is not complete yet. With a blank value, it may be harder to track down unexpected values but I think it's an overall better experience.

I wanted to use standard Java Base64 but that's not in Android until API 26. We already have a precedent for pulling code in from https://github.com/brsanthu/migbase64/blob/master/src/main/java/com/migcomponents/migbase64/Base64.java so I continued that.

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?

Isolated addition so all risk is to the new code which could be wrong.

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

See test.

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

getodk/xforms-spec#301

lognaturel · 2023-02-10T23:11:43Z

src/test/java/org/javarosa/xpath/expr/XPathFuncExprBase64DecodeTest.java

+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Enclosed.class)


I wanted to have both valid and invalid input cases in the same file. This is the cleanest way I found to do that.

What about removing Parameterized and just creating a different test for each of the 5 cases? I know it's more code, but I usually find parameterized tests (and especially JUnit's implementation of them) very hard to read and reason about. They're also very hard to rework if the one of the cases is modified, so there's a caveat (different setup, more assertions etc).

I agree with you in some cases but I think it really depends on the shape of the tests. With this style of test where there's a standard form with different values subbed in, I find parameterized tests easier to work with. We have a lot of examples of Parameterized tests in JR.

That said, I'm ok moving to lowercase-p parameterized tests. I've done that in the latest commit. I think the important thing is that the form is only defined once. Hopefully this is a reasonable compromise for you.

I much prefer the new approach. I find using common builders/helpers a much nicer approach than an entire parametrized test, as you're still able to have a unique procedure for each case (without branching).

Makes sense! We can move towards that as we introduce new tests.

seadowg · 2023-03-15T14:00:43Z

src/test/java/org/javarosa/xpath/expr/XPathFuncExprBase64DecodeTest.java

+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class XPathFuncExprBase64DecodeTest {


Could this just be Base64DecodeTest? I think the fact that it's nested in a way that makes it's clear it's testing an XPath expression.

seadowg · 2023-03-15T14:08:41Z

src/test/java/org/javarosa/xpath/expr/XPathFuncExprBase64DecodeTest.java

+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Enclosed.class)


What about removing Parameterized and just creating a different test for each of the 5 cases? I know it's more code, but I usually find parameterized tests (and especially JUnit's implementation of them) very hard to read and reason about. They're also very hard to rework if the one of the cases is modified, so there's a caveat (different setup, more assertions etc).

seadowg · 2023-03-15T14:14:06Z

src/main/java/org/javarosa/xpath/expr/Encoding.java

+        // Copied from: https://github.com/brsanthu/migbase64/blob/master/src/main/java/com/migcomponents/migbase64/Base64.java
+        // Irrelevant after Java8
+        @Override
+        String encode(byte[] sArr) {


Could encode just throw an Unsupported operation for now seeing as it's not being used and it's untested?

This is only a whitespace change to make indentation consistent with rest of project. encode was introduced and tested in #247.

Ah, sorry I got confused. I thought the whitespace change was just for the HEX stuff, but I see now that you only added decode to this file.

seadowg · 2023-03-15T14:18:18Z

src/test/java/org/javarosa/xpath/expr/EncodingDecodeTest.java

+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class EncodingDecodeTest {


It looks like this adds testing for hex, but as far as I can see, the Base 64 cases are double coverage of the testing provides by XPathFuncExprBase64DecodeTest. Would it make sense just to add a test for hex decoding instead? That could be a follow-up PR I reckon.

I have a pretty strong preference to test at both levels because it's a pattern that exists throughout JR. There have been cases of functions that work when tested in isolation like this but not in forms. It's helpful to have the test coverage to differentiate the two, especially when there are regressions.

In this specific case, I further like the symmetry with EncodingEncodeTest.

seadowg · 2023-03-15T14:19:06Z

src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java

@@ -486,6 +487,9 @@ public Object eval(DataInstance model, EvaluationContext evalContext) {
                return XPathNodeset.shuffle((XPathNodeset) argVals[0], toNumeric(argVals[1]).longValue());

            throw new XPathUnhandledException("function \'randomize\' requires 1 or 2 arguments. " + args.length + " provided.");
+        } else if (name.equals("base64-decode")) {
+            assertArgsCount(name, args, 1);


It doesn't look like there's a test for this line.

Add base64-decode

f9d7b1d

lognaturel marked this pull request as draft February 10, 2023 22:04

lognaturel added 2 commits February 10, 2023 15:00

Reformat

b81ae6f

Use Android 21-compatible decode

00a5221

lognaturel marked this pull request as ready for review February 10, 2023 23:10

lognaturel commented Feb 10, 2023

View reviewed changes

lognaturel requested a review from seadowg March 7, 2023 15:49

seadowg requested changes Mar 15, 2023

View reviewed changes

lognaturel added 3 commits March 15, 2023 11:19

Remove test filename prefixes

54074d3

Test check for parameter count

f49591d

Don't use JUnit Parameterized

8f73505

seadowg approved these changes Mar 16, 2023

View reviewed changes

seadowg merged commit 96c37ee into getodk:master Mar 16, 2023

lognaturel deleted the base64 branch March 17, 2023 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add base64-decode #708

Add base64-decode #708

lognaturel commented Feb 10, 2023 •

edited

lognaturel Feb 10, 2023

seadowg Mar 15, 2023

lognaturel Mar 15, 2023

seadowg Mar 16, 2023

lognaturel Mar 17, 2023

seadowg Mar 15, 2023

seadowg Mar 15, 2023

seadowg Mar 15, 2023

lognaturel Mar 15, 2023

seadowg Mar 16, 2023

seadowg Mar 15, 2023

lognaturel Mar 15, 2023

seadowg Mar 15, 2023

Add base64-decode #708

Add base64-decode #708

Conversation

lognaturel commented Feb 10, 2023 • edited

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

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.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel commented Feb 10, 2023 •

edited