Page MenuHomePhabricator

Write an Ace worker for AbuseFilter and add clientside validation
Closed, ResolvedPublic

Description

Currently, we have under review a patch to switch AbuseFilter to use Ace editor if CodeEditor is installed. We may take the opportunity and write a custom Ace worker to provide not only a syntax highlight but also a client-side syntax validation for filter rules (while leaving as it is the server-side one). This way the user doesn't need to click "check syntax", he is reported with row-by-row error messages, also a bit more explicative than the current ones. Moreover, we may add warnings and info. The first could be used e.g. to warn the user of a deprecated syntax (like for T173889), the latter to offer some performance tips, like the ones in the Rules format page.
I'm separating this from the syntax highlight itself since this is a more complex task.

Event Timeline

Daimona created this task.Feb 19 2018, 9:24 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 19 2018, 9:24 AM
Daimona triaged this task as Low priority.Feb 19 2018, 9:24 AM
Daimona moved this task from Backlog to Management features on the AbuseFilter board.
Daimona added subscribers: matej_suchanek, OldUser02.

@matej_suchanek Yeah, definitely something we want to implement :-) Unfortunately I took a look at the workers some time ago but I'm afraid I'm not able to write one.

matej_suchanek rescinded a token.

Another thing we could put as a warning: users are allowed to write filters like

1 == 1 &

And leaving a "&" without operators always return false. This behaviour might have its sense and could be here to avoid dealing with nasty situations and/or parser bugs, but we should at least make clear to the user that such condition will always return false. Due to this, I accidentally disabled a quite important itwiki filter for a month, noticed it by chance thanks to MusikBot reporting it as staled and it took me another 10 minutes to spot the error.

Another random idea for warnings: use them when a function taking x parameter is called with more than x parameters.

So I tried to do this today... Setting up the worker is not too hard, the only thing we need to pay attention to are paths: ace uses relative paths, and CodeEditor (and also our edit module) set the base path to extension/CodeEditor/modules, because that's where ace lives. But this new mode would live in AF/modules, so we need to change the path. But anyway, it shouldn't be super-hard to fix that.

The main problem is how to validate the filter code. I see two alternatives:

  1. Port the PHP parser to JS. That'd be a ridiculous waste of time, and hard to maintain.
  2. Do that via an API call to AbuseFilterCheckSyntax (like the "Check syntax" button does). However, we'd need the mw global to be available inside the worker, and I'm not sure if that's possible. I tried to play around a bit, and wasn't able to do that.

Change 647024 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Run real-time validation of rules with an Ace worker

https://gerrit.wikimedia.org/r/647024

Daimona claimed this task.Dec 8 2020, 5:24 PM

So, the approach above works as expected, and if we're just looking at that, it can be merged as-is. However, due to the use of Workers (of which I really have little experience), we need to bootstrap a lot of ace functionalities, which results in roughly 1600 lines of copypasta boilerplate. This is probably worsened by the ResourceLoader layer and the fact that most Ace scripts come from CodeEditor.

If we found a way to avoid that (or determine that it cannot be avoided), then this is done.

So, the approach above works as expected, and if we're just looking at that, it can be merged as-is. However, due to the use of Workers (of which I really have little experience), we need to bootstrap a lot of ace functionalities, which results in roughly 1600 lines of copypasta boilerplate. This is probably worsened by the ResourceLoader layer and the fact that most Ace scripts come from CodeEditor.

If we found a way to avoid that (or determine that it cannot be avoided), then this is done.

I believe this is not possible. That is also further complicated by the worker running in a blob, and us not having requirejs (because we use RL). The other workers (that come with ace itself) included in CodeEditor also have this boilerplate. So I think it would be a pointless effort to avoid the boilerplate, and the patch can be merged as-is.

Daimona closed this task as Resolved.Dec 18 2020, 7:29 PM
Daimona removed a project: Patch-For-Review.
Daimona reopened this task as Open.Dec 18 2020, 7:40 PM
Daimona added a project: User-notice.

Stub for Tech News:

The AbuseFilter edit interface now shows syntax errors while typing. This is the similar to JavaScript pages. It also displays a warning for regular expressions that match the empty string, and new warnings will be added. [[:phab:T187686|]]

Change 647024 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Run real-time validation of rules with an Ace worker

https://gerrit.wikimedia.org/r/647024

Daimona closed this task as Resolved.Thu, Jan 7, 11:04 PM