From 8bddf3d2c95bdccd43c2b7c9ffd69e87f3f34c15 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 11 May 2018 17:09:44 +0200 Subject: [PATCH] Bug 1451031 - Add JEXL filter support in Remote Settings r=mythmon MozReview-Commit-ID: DwVdW1G3yZG --HG-- extra : rebase_source : 19e17e2af94afda999e557e89b786f7049a1fdc6 --- services/common/blocklist-clients.js | 19 +- services/common/docs/RemoteSettings.rst | 12 ++ services/common/remote-settings.js | 29 ++- .../tests/unit/test_blocklist_clients.js | 45 +++++ .../unit/test_remote_settings_jexl_filters.js | 171 ++++++++++++++++++ services/common/tests/unit/xpcshell.ini | 2 + 6 files changed, 269 insertions(+), 9 deletions(-) create mode 100644 services/common/tests/unit/test_remote_settings_jexl_filters.js diff --git a/services/common/blocklist-clients.js b/services/common/blocklist-clients.js index 5a4eb74bf182..616667d1681c 100644 --- a/services/common/blocklist-clients.js +++ b/services/common/blocklist-clients.js @@ -11,8 +11,8 @@ var EXPORTED_SYMBOLS = [ ChromeUtils.import("resource://gre/modules/Services.jsm"); const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm", {}); -ChromeUtils.defineModuleGetter(this, "RemoteSettings", - "resource://services-common/remote-settings.js"); +ChromeUtils.defineModuleGetter(this, "RemoteSettings", "resource://services-common/remote-settings.js"); +ChromeUtils.defineModuleGetter(this, "jexlFilterFunc", "resource://services-common/remote-settings.js"); const PREF_BLOCKLIST_BUCKET = "services.blocklist.bucket"; const PREF_BLOCKLIST_ONECRL_COLLECTION = "services.blocklist.onecrl.collection"; @@ -132,17 +132,22 @@ async function updateJSONBlocklist(client, { data: { current: records } }) { * This custom filter function is used to limit the entries returned * by `RemoteSettings("...").get()` depending on the target app information * defined on entries. - * - * When landing Bug 1451031, this function will have to check if the `entry` - * has a JEXL attribute and rely on the JEXL filter function in priority. - * The legacy target app mechanism will be kept in place for old entries. */ -async function targetAppFilter(entry, { appID, version: appVersion }) { +async function targetAppFilter(entry, environment) { + // If the entry has JEXL filters, they should prevail. + // The legacy target app mechanism will be kept in place for old entries. + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1463377 + const { filters } = entry; + if (filters) { + return jexlFilterFunc(entry, environment); + } + // Keep entries without target information. if (!("versionRange" in entry)) { return entry; } + const { appID, version: appVersion } = environment; const { versionRange } = entry; // Gfx blocklist has a specific versionRange object, which is not a list. diff --git a/services/common/docs/RemoteSettings.rst b/services/common/docs/RemoteSettings.rst index 94b9d06f41c4..52234eb6c9f2 100644 --- a/services/common/docs/RemoteSettings.rst +++ b/services/common/docs/RemoteSettings.rst @@ -103,6 +103,18 @@ It is possible to package a dump of the server records that will be loaded into Now, when ``RemoteSettings("some-key").get()`` is called from an empty profile, the ``some-key.json`` file is going to be loaded before the results are returned. +Targets and A/B testing +======================= + +In order to deliver settings to subsets of the population, you can set targets on entries (platform, language, channel, version range, preferences values, samples, etc.) when editing records on the server. + +From the client API standpoint, this is completely transparent: the ``.get()`` method — as well as the event data — will always filter the entries on which the target matches. + +.. note:: + + The remote settings targets follow the same approach as the :ref:`Normandy recipe client ` (ie. JEXL filters), + + Uptake Telemetry ================ diff --git a/services/common/remote-settings.js b/services/common/remote-settings.js index 789d1d5f4782..d0220de6b588 100644 --- a/services/common/remote-settings.js +++ b/services/common/remote-settings.js @@ -4,7 +4,10 @@ "use strict"; -var EXPORTED_SYMBOLS = ["RemoteSettings"]; +var EXPORTED_SYMBOLS = [ + "RemoteSettings", + "jexlFilterFunc" +]; ChromeUtils.import("resource://gre/modules/Services.jsm"); ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); @@ -21,6 +24,7 @@ ChromeUtils.defineModuleGetter(this, "UptakeTelemetry", "resource://services-common/uptake-telemetry.js"); ChromeUtils.defineModuleGetter(this, "ClientEnvironmentBase", "resource://gre/modules/components-utils/ClientEnvironment.jsm"); +ChromeUtils.defineModuleGetter(this, "FilterExpressions", "resource://normandy/lib/FilterExpressions.jsm"); const PREF_SETTINGS_SERVER = "services.settings.server"; const PREF_SETTINGS_DEFAULT_BUCKET = "services.settings.default_bucket"; @@ -62,6 +66,27 @@ class ClientEnvironment extends ClientEnvironmentBase { } } +/** + * Default entry filtering function, in charge of excluding remote settings entries + * where the JEXL expression evaluates into a falsy value. + */ +async function jexlFilterFunc(entry, environment) { + const { filters } = entry; + if (!filters) { + return entry; + } + let result; + try { + const context = { + environment + }; + result = await FilterExpressions.eval(filters, context); + } catch (e) { + Cu.reportError(e); + } + return result ? entry : null; +} + function mergeChanges(collection, localRecords, changes) { const records = {}; @@ -150,7 +175,7 @@ async function fetchLatestChanges(url, lastEtag) { class RemoteSettingsClient { - constructor(collectionName, { bucketName, signerName, filterFunc, lastCheckTimePref }) { + constructor(collectionName, { bucketName, signerName, filterFunc = jexlFilterFunc, lastCheckTimePref }) { this.collectionName = collectionName; this.bucketName = bucketName; this.signerName = signerName; diff --git a/services/common/tests/unit/test_blocklist_clients.js b/services/common/tests/unit/test_blocklist_clients.js index c54324471ece..1e6f878a1a13 100644 --- a/services/common/tests/unit/test_blocklist_clients.js +++ b/services/common/tests/unit/test_blocklist_clients.js @@ -245,6 +245,51 @@ add_task(async function test_sync_event_data_is_filtered_for_target() { }); add_task(clear_state); +add_task(async function test_entries_are_filtered_when_jexl_filters_is_present() { + const records = [{ + willMatch: true, + }, { + willMatch: true, + filters: null + }, { + willMatch: true, + filters: "1 == 1" + }, { + willMatch: false, + filters: "1 == 2" + }, { + willMatch: true, + filters: "1 == 1", + versionRange: [{ + targetApplication: [{ + guid: "some-guid" + }], + }] + }, { + willMatch: false, // jexl prevails over versionRange. + filters: "1 == 2", + versionRange: [{ + targetApplication: [{ + guid: "xpcshell@tests.mozilla.org", + minVersion: "0", + maxVersion: "*", + }], + }] + } + ]; + for (let {client} of gBlocklistClients) { + const collection = await client.openCollection(); + for (const record of records) { + await collection.create(record); + } + await collection.db.saveLastModified(42); // Prevent from loading JSON dump. + const list = await client.get(); + equal(list.length, 4); + ok(list.every(e => e.willMatch)); + } +}); +add_task(clear_state); + // get a response for a given request from sample data function getSampleResponse(req, port) { diff --git a/services/common/tests/unit/test_remote_settings_jexl_filters.js b/services/common/tests/unit/test_remote_settings_jexl_filters.js new file mode 100644 index 000000000000..1e960061203f --- /dev/null +++ b/services/common/tests/unit/test_remote_settings_jexl_filters.js @@ -0,0 +1,171 @@ +const { RemoteSettings } = ChromeUtils.import("resource://services-common/remote-settings.js", {}); + +let client; + +async function createRecords(records) { + const collection = await client.openCollection(); + await collection.clear(); + for (const record of records) { + await collection.create(record); + } + await collection.db.saveLastModified(42); // Prevent from loading JSON dump. +} + + +function run_test() { + client = RemoteSettings("some-key"); + + run_next_test(); +} + +add_task(async function test_returns_all_without_target() { + await createRecords([{ + passwordSelector: "#pass-signin" + }, { + filters: null, + }, { + filters: "", + }]); + + const list = await client.get(); + equal(list.length, 3); +}); + +add_task(async function test_filters_can_be_disabled() { + const c = RemoteSettings("no-jexl", { filterFunc: null }); + const collection = await c.openCollection(); + await collection.create({ + filters: "1 == 2" + }); + await collection.db.saveLastModified(42); // Prevent from loading JSON dump. + + const list = await c.get(); + equal(list.length, 1); +}); + +add_task(async function test_returns_entries_where_jexl_is_true() { + await createRecords([{ + willMatch: true, + filters: "1" + }, { + willMatch: true, + filters: "[42]" + }, { + willMatch: true, + filters: "1 == 2 || 1 == 1" + }, { + willMatch: true, + filters: 'environment.appID == "xpcshell@tests.mozilla.org"' + }, { + willMatch: false, + filters: "environment.version == undefined" + }, { + willMatch: true, + filters: "environment.unknown == undefined" + }, { + willMatch: false, + filters: "1 == 2" + }]); + + const list = await client.get(); + equal(list.length, 5); + ok(list.every(e => e.willMatch)); +}); + +add_task(async function test_ignores_entries_where_jexl_is_invalid() { + await createRecords([{ + filters: "true === true" // JavaScript Error: "Invalid expression token: =" + }, { + filters: "Objects.keys({}) == []" // Token ( (openParen) unexpected in expression + }]); + + const list = await client.get(); + equal(list.length, 0); +}); + +add_task(async function test_support_of_date_filters() { + await createRecords([{ + willMatch: true, + filters: '"1982-05-08"|date < "2016-03-22"|date' + }, { + willMatch: false, + filters: '"2000-01-01"|date < "1970-01-01"|date' + }]); + + const list = await client.get(); + equal(list.length, 1); + ok(list.every(e => e.willMatch)); +}); + +add_task(async function test_support_of_preferences_filters() { + await createRecords([{ + willMatch: true, + filters: '"services.settings.last_etag"|preferenceValue == 42' + }, { + willMatch: true, + filters: '"services.settings.changes.path"|preferenceExists == true' + }, { + willMatch: true, + filters: '"services.settings.changes.path"|preferenceIsUserSet == false' + }, { + willMatch: true, + filters: '"services.settings.last_etag"|preferenceIsUserSet == true' + }]); + + // Set a pref for the user. + Services.prefs.setIntPref("services.settings.last_etag", 42); + + const list = await client.get(); + equal(list.length, 4); + ok(list.every(e => e.willMatch)); +}); + +add_task(async function test_support_of_intersect_operator() { + await createRecords([{ + willMatch: true, + filters: '{foo: 1, bar: 2}|keys intersect ["foo"]' + }, { + willMatch: true, + filters: '(["a", "b"] intersect ["a", 1, 4]) == "a"' + }, { + willMatch: false, + filters: '(["a", "b"] intersect [3, 1, 4]) == "c"' + }, { + willMatch: true, + filters: ` + [1, 2, 3] + intersect + [3, 4, 5] + ` + }]); + + const list = await client.get(); + equal(list.length, 3); + ok(list.every(e => e.willMatch)); +}); + +add_task(async function test_support_of_samples() { + await createRecords([{ + willMatch: true, + filters: '"always-true"|stableSample(1)' + }, { + willMatch: false, + filters: '"always-false"|stableSample(0)' + }, { + willMatch: true, + filters: '"turns-to-true-0"|stableSample(0.5)' + }, { + willMatch: false, + filters: '"turns-to-false-1"|stableSample(0.5)' + }, { + willMatch: true, + filters: '"turns-to-true-0"|bucketSample(0, 50, 100)' + }, { + willMatch: false, + filters: '"turns-to-false-1"|bucketSample(0, 50, 100)' + }]); + + const list = await client.get(); + equal(list.length, 3); + ok(list.every(e => e.willMatch)); +}); diff --git a/services/common/tests/unit/xpcshell.ini b/services/common/tests/unit/xpcshell.ini index 47386c29c638..4badf91bb006 100644 --- a/services/common/tests/unit/xpcshell.ini +++ b/services/common/tests/unit/xpcshell.ini @@ -22,6 +22,8 @@ tags = blocklist tags = remote-settings blocklist [test_remote_settings_poll.js] tags = remote-settings blocklist +[test_remote_settings_jexl_filters.js] +tags = remote-settings [test_kinto.js] tags = blocklist