Add JEXL evaluator access from Swift and Kotlin #4813

jhugman · 2022-02-01T19:03:57Z

This PR refactors the targeting infrastructure to allow JEXL to be available for external applications.

This is relatively specialised: it allows evaluation against the targeting attributes Nimbus uses, and allows a JSON object/Codable to be add extra variables.

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

codecov-commenter · 2022-02-01T19:11:17Z

Codecov Report

Merging #4813 (a587631) into main (2e88010) will increase coverage by 0.14%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #4813      +/-   ##
==========================================
+ Coverage   81.49%   81.64%   +0.14%     
==========================================
  Files          49       50       +1     
  Lines        5653     5660       +7     
==========================================
+ Hits         4607     4621      +14     
+ Misses       1046     1039       -7

Impacted Files	Coverage Δ
components/nimbus/src/matcher.rs	`94.44% <ø> (+47.22%)`	⬆️
components/nimbus/src/lib.rs	`85.97% <57.14%> (-0.88%)`	⬇️
components/nimbus/src/evaluator.rs	`96.44% <75.00%> (-0.84%)`	⬇️
components/nimbus/tests/test_message_helpers.rs	`100.00% <100.00%> (ø)`
components/nimbus/src/error.rs	`20.58% <0.00%> (+8.82%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e88010...a587631. Read the comment docs.

jhugman · 2022-02-01T20:01:42Z

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt

@@ -34,7 +34,7 @@ import org.robolectric.RobolectricTestRunner
 import java.util.concurrent.Executors

 @RunWith(RobolectricTestRunner::class)
-class NimbusTest {
+class NimbusTests {


Aside from the rename, I think I accidentally ran a re-format on this file.

jhugman · 2022-02-01T20:02:11Z

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt

@@ -292,7 +363,8 @@ class NimbusTest {
                  "id": "test-experiment",
                  "last_modified": 1602197324372
                }]}
-            """.trimIndent())
+            """.trimIndent()
+        )


Nerp. No extra changes.

tarikeshaq

Awesome! Looks good to me - it would be awesome if we can add a couple more tests cases in the kotlin/swift code, but otherwise no need to block on my review!

I am absolutely in love with exposing the ability to run targeting by the consumers 🚀 This should allow us to improve our end-to-end testing story as well 🕺

tarikeshaq · 2022-02-03T04:15:39Z

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/GleanPlumbTests.kt

+    }
+
+    @Test
+    fun `jexl can be run against the json context`() {


should we add more complex jexl statements (at the very least something with "ors" and "and")?

tarikeshaq · 2022-02-03T04:30:08Z

components/nimbus/src/lib.rs

+        match json.as_object() {
+            Some(obj) => Ok(obj.clone()),
+            _ => Err(uniffi::deps::anyhow::anyhow!(
+                "Unexpected JSON-non-object in the bagging area"


Hmm can we add tests for a non-object json? to make sure the exception is handled correctly by this - I think the invalid json case is covered by the bindings side custom types right?

I've added tests, but we try hard to make sure strings that don't code for objects enter the system.

tarikeshaq · 2022-02-03T04:30:45Z

components/nimbus/src/matcher.rs

-#[derive(Deserialize, Serialize, Debug, Clone, Default)]
-pub struct Matcher {
-    pub app_name: String,
-    pub app_id: String,
-    pub channel: String,
-    pub app_display_version: Option<String>,
-    pub app_min_version: Option<String>,
-    pub app_max_version: Option<String>,
-    pub app_build: Option<String>,
-    pub app_min_build: Option<String>,
-    pub app_max_build: Option<String>,
-    pub architecture: Option<String>,
-    pub device_manufacturer: Option<String>,
-    pub device_model: Option<String>,
-    pub locale: Option<String>,
-    pub os: Option<String>,
-    pub os_version: Option<String>,
-    pub android_sdk_version: Option<String>,
-    pub debug_tags: Vec<String>,
-}


tarikeshaq · 2022-02-03T04:33:46Z

components/nimbus/tests/test_message_helpers.rs

+#[macro_use]
+use nimbus::error::Result;
+
+#[cfg(feature = "rkv-safe-mode")]


out of curiosity, why is this necessary? I know it's in the other test files, but does it have any implication on targeting tests?

I think it's because we actually use a DB in testing. I don't know definitively, except it doesn't work without it. It's cargo cult by this time.

tarikeshaq · 2022-02-03T04:37:28Z

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/GleanPlumbTests.kt

+            messageHelper.evalJexl("test_value_from_json == 42")
+        }
+
+        assertTrue(


😍 🎉 🚀

Swift fmt

jhugman force-pushed the jhugman/add-jexl-helper branch from a587631 to 23fa587 Compare February 1, 2022 19:23

jhugman requested review from tarikeshaq and travis79 February 1, 2022 19:57

jhugman commented Feb 1, 2022

View reviewed changes

data-sync-user mentioned this pull request Feb 2, 2022

Expose a JEXL evaluator for use by the MessagingController mozilla/experimenter#7020

Closed

tarikeshaq approved these changes Feb 3, 2022

View reviewed changes

jhugman added 8 commits February 3, 2022 17:55

Quick add without the json context

d50f51d

Allow JSON string to be passed in from UDL

743194b

Add access to JEXL evaluator to UDL/Rust

556c364

Rust fmt

5981b5a

Add access to JEXL evaluator from Kotlin

c860059

Add access to JEXL evaluator from Swift

ad50f0a

Swift fmt

Changelog

335d755

Add extra tests for JEXL with invalid JSON

e5797ca

jhugman force-pushed the jhugman/add-jexl-helper branch from 23fa587 to e5797ca Compare February 3, 2022 18:50

jhugman merged commit 0818678 into main Feb 3, 2022

jhugman deleted the jhugman/add-jexl-helper branch February 3, 2022 19:07

This was referenced Feb 15, 2022

Update to A-S 91.1.0 on main mozilla-mobile/android-components#11705

Merged

Bumps application services to 91.1.0 mozilla-mobile/firefox-ios#10048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JEXL evaluator access from Swift and Kotlin #4813

Add JEXL evaluator access from Swift and Kotlin #4813

jhugman commented Feb 1, 2022 •

edited

Loading

codecov-commenter commented Feb 1, 2022

jhugman Feb 1, 2022

jhugman Feb 1, 2022

tarikeshaq left a comment

tarikeshaq Feb 3, 2022

tarikeshaq Feb 3, 2022

jhugman Feb 3, 2022

tarikeshaq Feb 3, 2022

tarikeshaq Feb 3, 2022

jhugman Feb 3, 2022

tarikeshaq Feb 3, 2022

Add JEXL evaluator access from Swift and Kotlin #4813

Add JEXL evaluator access from Swift and Kotlin #4813

Conversation

jhugman commented Feb 1, 2022 • edited Loading

Pull Request checklist

codecov-commenter commented Feb 1, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikeshaq 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

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

jhugman commented Feb 1, 2022 •

edited

Loading