1451031 - Implement JEXL filtering on remote settings entries

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Mathieu Leplatre [:leplatrem]]

Each remote settings entry can have a ``target`` attribute, with a JEXL expression in it (string).

If present, this target will be evaluated so that ``RemoteSettings("key").get()`` returns only matching entries.

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1451031 - Add JEXL filter support in Remote Settings

Review commit: https://reviewboard.mozilla.org/r/243392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243392/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

https://reviewboard.mozilla.org/r/243392/#review249628


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/tests/browser/browser.ini:1
(Diff revision 1)
> +# [DEFAULT]

Error: "use 'disabled=<reason>' to disable a test instead of a comment"
 [no-comment-disable]

Comment 3

[Mathieu Leplatre [:leplatrem]]

Mike, I obtain "undefined" from the JEXL evaluations. Both from the xpcshell and mochitest test suites (not from a browser console though).

Do you remember having faced something similar? 

Thanks ;)

Comment 4

[Michael Cooper [:mythmon]]

JEXL is much more permissive about undefined than JS is. Specifically, if `foo` is undefined, then `foo.bar` will return undefined. That is, undefined values cascade.

I didn't see anything obvious in this patch, but I would guess that the context you are passing the JEXL is somehow ending up without the keys you expect. Maybe test a expression that doesn't rely on the context at all, such as `true` or `1 + 1 == 2`? If that doesn't work, can you point me to a specific failing test that I can take a look at?

Comment 5

[Mathieu Leplatre [:leplatrem]]

Thanks Mike for your support, I finally found the source of the error.

When the given `context` is undefined, every expression is evaluated as undefined.

I was not providing directly an undefined context, but a proxy to a class. And strangely it was interpreted as undefined. Providing an object with the proxied class in a field works perfectly.

Comment 6

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243392/diff/1-2/

Comment 7

[Michael Cooper [:mythmon]]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

https://reviewboard.mozilla.org/r/243392/#review251190

Yay! This looks like it fits in well with your existing filters. r=me with those comments fixed.

::: services/common/blocklist-clients.js:138
(Diff revision 2)
> - * 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.

Will these old entries eventually be retired? Would a bug to track the old entries and eventually remove legacy filter support would be a good idea?

::: services/common/docs/RemoteSettings.rst:115
(Diff revision 2)
> +
> +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:`SHIELD recipe client <components/normandy>` (ie. JEXL filters),

"SHIELD recipe client" was at one time a good name for this, but nowadays it is probably better to call it "Normandy" or "Normandy recipe client".

::: services/common/remote-settings.js:85
(Diff revision 2)
> +    const context = {
> +      environment
> +    };
> +    result = await FilterExpressions.eval(filters, context);
> +  } catch (e) {
> +    dump(`${e}\n\n`);

Oops, this `dump` statement looks like it should be removed.

::: services/common/tests/unit/test_remote_settings_jexl_filters.js:79
(Diff revision 2)
> +
> +add_task(async function test_ignores_entries_where_jexl_is_invalid() {
> +  await createRecords([{
> +    filters: "true === true"  // JavaScript Error: "Invalid expression token: ="
> +  }, {
> +    filters: "unknown.name == 1"

This isn't invalid JEXL, even though the name doesn't exist (since JEXL cascades undefined). It is just a false statement. I don't think that's what this test was trying to do.

::: services/common/tests/unit/test_remote_settings_jexl_filters.js:108
(Diff revision 2)
> +    willMatch: true,
> +    filters: '"services.settings.changes.path"|preferenceIsUserSet == false'
> +  }, {
> +    willMatch: true,
> +    filters: '"services.settings.last_etag"|preferenceIsUserSet == true'
> +  }]);

It would probably be good to have some negative cases in these tests too.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

https://reviewboard.mozilla.org/r/243392/#review251418

I think I trust mythmon's review here more than my own. Off-hand this looks fine besides conflicting with the stuff in bug 1257565, but we should probably land this first anyway... :P

One thing I'd note is that the previous target filtering could be done server-side. I really wish we could keep doing some server-side filtering to avoid the busy-work of filtering client side, but I guess that may be difficult to do within the extant Kinto infrastructure?

Comment 9

[Mathieu Leplatre [:leplatrem]]

> I think I trust mythmon's review here more than my own. Off-hand this looks fine besides conflicting with the stuff in bug 1257565, but we should probably land this first anyway... :P

Appart some possible minor rebase conflicts, it should not conflict with the work being being done in bug 1257565. What do you have in mind?

> One thing I'd note is that the previous target filtering could be done server-side. I really wish we could keep doing some server-side filtering to avoid the busy-work of filtering client side, but I guess that may be difficult to do within the extant Kinto infrastructure?

This is something that was largely discussed, and filtering on the client side was the simplest approach, not so much because of Kinto but mainly because of content signatures. Filtering on the server side would mean signing each possible dataset (combinations of version etc.) and signatures would be harder to monitor / refresh / validate etc. Plus, the whole dataset size is not crazy big, and for use-cases that only rely on "sync" event to run code, the filtering processing only happens when a change is published, which is not so often (once every two weeks)

Comment 10

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243392/diff/2-3/

Comment 11

[:Gijs (he/him)]

(In reply to Mathieu Leplatre (:leplatrem) from comment #9)
> > I think I trust mythmon's review here more than my own. Off-hand this looks fine besides conflicting with the stuff in bug 1257565, but we should probably land this first anyway... :P
> 
> Appart some possible minor rebase conflicts, it should not conflict with the
> work being being done in bug 1257565. What do you have in mind?

Eh, I must have been confused when I wrote that comment about where some of the changes were, but I was mostly thinking of rebase issues. I think it should be fine. :-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dc86982439a
Add JEXL filter support in Remote Settings r=mythmon

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out changeset 2dc86982439a (bug 1451031) for xpcshell failures on services/common/tests/unit/test_blocklist_clients.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=33aa8e983a92263d976963f3fef99a45069ec20c&fromchange=2dc86982439a78ebed470b0270f80ba3ecf87605&tochange=c5d34260a2239287635823bd5cc412c4199d9ffa&selectedJob=179671389

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179671389&repo=autoland&lineNumber=1618

Backout link: https://hg.mozilla.org/integration/autoland/rev/c5d34260a2239287635823bd5cc412c4199d9ffa

Comment 14

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8975506 [details]
Bug 1451031 - Add JEXL filter support in Remote Settings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243392/diff/3-4/

Comment 15

[Mathieu Leplatre [:leplatrem]]

I hadn't realized that tests would run on Android. I disabled them, and it seems to be fixed on Try.

Sorry about the trouble ;)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13f0a3149181
Add JEXL filter support in Remote Settings r=mythmon

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/13f0a3149181