#434 Keep AJAX DRY

[Lee Worden]

It's important to maintain a WW that works without JavaScript, which will require attention as I bring more features into Ajax actions. (How important is it?)

The non-javascript, existing WW features work by calling mostly ManageProject with special ww-action= values in the URL. That triggers a call to my WWAction classes, which do the actions and produce output to be included in the HTML page.

Ajax versions of the same actions are implemented by calling api.php, which triggers calls to WW-specific subclasses of MediaWiki's ApiBase class.

As I add more ApiBase subclasses, I'm in danger of duplicating all the existing WWAction classes, which would be bad. It's a waste of effort, and the code duplication would lead to fixes in the API code not getting implemented in the WWAction code, and the non-JavaScript implementation of WW would crumble and stop working with nobody noticing.

Much better to develop a framework allowing a single piece of code to be called for both kinds of actions - shouldn't be a big deal, really.

Or at least make sure they both contain nothing but the simplest calls to core functions that are the same in both cases. Currently this is not so - for instance, the import-project-file API action does a significant amount of work distinguishing different cases, source files vs. project files for instance, that is different from any other WW interface.

[Lee Worden]

PS. DRY = https://en.wikipedia.org/wiki/Don't_repeat_yourself

[Lee Worden]

There is also a whole system of PEOperation classes in ProjectEngine, implementing the sync, make, delete, retrieve and such actions that we ask PE to do. Many of them are the implementation layer of corresponding WWAction classes. But those are separate by design, because WW is the interface between MW and PE, and PE is the interface between WW and the filesystem.

[Lee Worden]

So what does a WWAction operation currently need to do, and what does an API action need to do?

WWAction:

• Each action class's execute() function needs to:
  • extract the argument values from the URL and transform them (look up the relevant project, for instance).
  • In some cases, return an intermediate message asking for confirmation
  • Do the operation
  • Return an array containing:
    • 'status' : WW_SUCCESS, WW_WARNING, WW_ERROR, WW_QUESTION, WW_ABORTOUTPUT
    • 'message' : HTML text in the user's language
    • 'choices' : in case of QUESTION, what confirm/cancel buttons to offer them
• some classes need to implement the requires_edit_permission() function, to return true if generic users can't do the operation
• each operation's code is responsible for checking all the inputs and dealing with missing or bad values.

API actions:

• each class's execute() needs to:
  • access the params in their native types. some need to be transformed, mainly the project data needs to get loaded.
  • do the operation
  • return things by using $this->getResult()->addValue().
  • In case of an error, bail out by calling $this->dieUsage().
  • I have them setting 'success' => true in case of no error, but this may be nonstandard and redundant.
• each class needs a getAllowedParams(), which tells the system how to error check the input before calling my code
• each class needs a getDescription() and getParamDescription(), which explain how to call the action in the help text produced by api.php.
• they don't ask for confirmation before doing an action - the calling code should do that.
• there are other things they can also implement, like mustBePosted(), to prohibit the action from being called in a GET request.
• isWriteMode() does the same as WWAction's requires_edit_permission().

So my first impression is that the API class interface encompasses just about everything that WWAction needs, so I could reimplement all the actions as API classes and provide a WWAction interface that calls those and transduces their input and output. They might need to do a little extra to handle the confirmation questions: I think I could do that by defining an extra method they can implement to explain how to prompt for missing parameters.

In other words: instead of writing a bunch of new API classes to do all the different operations that I've already implemented as WWAction classes, I'm going to restructure the WWAction classes into API class form, and they will also provide the WWAction operations that they already do. Sweet!

I think I'll do that by leaving the WWActions.php file as is, taking a copy of it, and making the copy into a bunch of API classes, then flip the switch later to start using the new classes for WWAction.

[Lee Worden]

Argh, but what about the idea of using Comet? If I convert all those classes to API classes, and then find that I have to convert them all to something else so that they can give real-time updates to the caller, it'll be a real pain. I'd better get further into the Comet planning before I do this conversion.

[Lee Worden]

OK so some questions, now that I've done some Comet work and I'm back here.

• the Api classes get their parameter values by calling $this->extractRequestParams(). When using WWAction, can I have it put the values from URL parameters into a place where that function will retrieve them?

• I'll need to pass in a parameter saying whether to use Comet or just do the work and return something at the end. Should I extend MediaWiki's format parameter to include SSE (Comet) output, or add a custom parameter, something like useComet=yes?

• How to handle dieUsage() (and are there other, similar exit routes?)?

[Lee Worden]

extractRequestParams() calls things like $this->getMain()->getCheck('...').

Which might actually retrieve WWAction's GET parameters, if I use them just right. I think I'll play with that.

[Lee Worden]

doesn't look like I can add SSE to the list of output formats, so I'll just use an extra argument telling me to ignore the requested format and do it my way.

[Lee Worden]

Or, I might be able to shoehorn it in using the ApiCheckCanExecute hook??

If that does work, I could use it while I request a more sane hook get added to the core code. It would allow me to provide a custom subclass of ApiFormatBase to format the output.

It does seem that this object is created before the execute() function is called, so I could have the action classes call on it to output data and headers. I was skeptical, but this does have some appeal. Maybe I'll try it.

[Lee Worden]

Uh oh, ApiCheckCanExecute hook is called too late. Will see if there's something else I can hook into instead.

[Lee Worden]

Regarding adding SSE to the supported output formats, see https://bugzilla.wikimedia.org/show_bug.cgi?id=57637, thank you very much. It'll be possible in MW 1.23.

[Lee Worden]

As for dieUsage(), the ApiBase::die* functions throw exceptions, and ApiMain catches them to do error reporting, so if I'm calling from WWAction I can catch them there and do different reporting. So it seems like I can go ahead and use those for error handling, which is good in the Api framework.

[Lee Worden]

I'm rewriting the merge-background-job action as an API action, with shim code to call that action from the WWAction framework. As part of that process, I've got merge-background-job running as an Ajax action from the browser, and I can switch back and forth between calling it via ww-action=merge-from-background in the URL params and calling it as 'merge-background-job' via api.php. Weirdly, it works via ww-action, but via Ajax it reports that it succeeded, displays an error message: "ProjectEngine: Project pe-ww:http://localhost/wiki:Sandbox not locked." and fails to merge the job.

[Lee Worden]

This is weird. I need to untangle this odd mergery. One question is why the error isn't reported as an error in the UI. The other one is why the merge doesn't succeed when called by api.php, but the same code works when called by Special:ManageProject.

Both need to be fixed, and I think I'll take them in that order, arbitrarily.

[Lee Worden]

The error reporting is fixed. But the WW messages need to be output differently when there's an error. I'll update the standard now.

[Lee Worden]

And on why the same operation works when it's a 'ww-action' URL option and fails when it's an API operation. I think I see why now.

There's code in the PERequest_hook() function in the background jobs code, and in a bunch of other hook functions in there, that checks whether there's a 'jobid' in the URL parameters, and modifies the meaning of other operations. This allows us to use the "list working directory" page and so on to operate in a background job rather than the persistent directory by adding jobid=XXXX to the URL.

What it does when it detects the jobid is two things: it adds the jobid to the request data sent to ProjectEngine, and if there's no project name given, it adds a 'pe-session-dir' value to the request data to tell ProjectEngine to do the operation in the background session directory. This allows URLs like ...Special:GetProjectFile?filename=.status&jobid=2345 to get the .status file in the base directory of background job 2345 - outside all the project directories in that session.

So that's the context. Actually 'jobid' is not in the URL when we use the URL to merge a background job: it's &ww-action=merge-from-background&action-jobid=2345, to give the ID of the job for the merge operation, not for the rest of the page's operations.

So it doesn't add jobid and pe-session-dir to the PE request, which is right. The jobid is put there explicitly by the merge code, and pe-session-dir needs to not be there, because it breaks the merge operation.

In the API call, on the other hand, we have a URL like api.php?action=ww-merge-background-job&jobid=2345. This triggers the hook's code to set the jobid (which is redundant and harmless) and 'pe-session-dir', which breaks the operation.

I guess one solution is to have it not do that when using api.php rather than index.php... well, no, I need to have it not do that when making an API call, even if it were within an index.php process... I guess this is what RequestContexts are for (introduced in about MW 1.16). If I were passing one of those around, I could just use it and not get confused.

... I don't actually know if RequestContext is quite the right thing for this, actually. Anyway I can do something like it, which is probably a lot better than what I've got now, API aside: I can have the background code evaluate whether the page's operations should all be within a background job at one time, early, and then not check anymore in all the different hooks. I can have it do that near the beginning of S:MP and S:GPF and not anywhere else.

[Lee Worden]

Yes, that was easy, and the API action works now. The old action also works, and it browses files in background jobs correctly. That's better!

I think I have a working merge API action now. I'll check it and deploy it to lalashan.

[Lee Worden]

Keeping in mind that (while useful) it's an Ajax action, and I'm not done until it's got Comet output for full responsiveness.

[Lee Worden]

Should I just develop Comet support for 1.23+, since that's when I'll be able to use format=sse? I kind of burned myself when I developed ImportProjectFiles out of Special:Upload rather than ImportWizard because ImportWizard didn't support 1.19, but then it took so long to develop that we don't care about 1.19 anyway. https://www.mediawiki.org/wiki/Version_lifecycle says that 1.23 will replace 1.19 as the LTS release in May 2014. (Why did I think 1.22 was going to be the new LTS?)

[Lee Worden]

For now, why don't I work on this API/WWAction integration, and come back to the Comet implementation afterward. I have ww-list-background-jobs, ww-create-background-job, ww-merge-background-job, and ww-destroy-background-job in the API framework now, and I think I'll take ww-destroy-background-job as my test case for this integration because it's earlier in the file, and because it includes a confirmation step.

[Lee Worden]

The integrated version of the destroy-background-job action works great - that was easy! It does 3 things along with calling the ww-destroy-background-job api, and other actions will have to do them in various combinations as well:

• translate the action's 'action-jobid' parameter to the api call's 'jobid' parameter.
  • The action uses 'action-jobid' because you might be browsing in one background job (specified by 'jobid' in the URL) and destroying another one (specified by 'action-jobid'). For the API it isn't like that and for simplicity and consistency I want to just call it 'jobid'.
• Ask for confirmation before destroying if it hasn't already been provided. I had thought I could have the API class direct this, but it seems the calling action doesn't actually have access to the API class, it just calls ApiMain::execute() with the API action name in the parameters, and that locates the API class just before it does the execute. So I need to keep track of confirmation somewhere other than in the API class. I could:
  • have a data structure mapping WW action names to API class names (I need to do this in some way) that also records whether and how to confirm before executing
  • provide a simple class for each WW action that makes the API call using the relevant API action name and takes care of confirmation
  • I also need to provide each confirmation question in the form of an i18n message: maybe I can just have a standardized name scheme for those, and just have it check whether the message exists to find out whether to confirm? This might have an interesting feature that wiki owners could customize what things get confirmation by changing those messages...
• check the API's results for success or failure and produce an output message. In case of error, just report what the API's error message says; in case of success the message is simple and standard. Are there actions that need to produce more complex output? Well, if so, I'm sure the API method will produce that output...

It'd be great to just have a single WWAction class and an array like

array( 'destroy-background-job' => 'ww-destroy-background-job',
    'merge-background-job' => 'ww-merge-background-job',
    ...
);

and nothing else on the Action side.

That example definitely suggests that it might be possible to deduce the API action names, also. So actually maybe I can have a single class that deduces API action names and calls them, doing certain standard transformations on the parameters ('action-jobid' to 'jobid', 'action-filename' to 'filename'), using the i18n message data to find out whether to ask for confirmation, and using the i18n message data and/or api's return data to produce the output message...

I'll do a survey of all the existing WWAction classes to see if that would cover what they do...

[Lee Worden]

As promised, survey of WWAction classes to see what kinds of exceptional treatment they need. I'm considering a structure like:

• WW gets a url like ...?action=X&params=YZ and calls WWAction to figure out what to do based on the action name X. X may be 'sync-source-files' or 'kill-background-job' or whatever.
• WWAction assumes the action is to be carried out by calling a corresponding API action called ww-X.
• WWAction has a list of which of these actions needs confirmation and what message to use for the confirmation. (Note these messages and maybe the list should also be used in the JavaScript code that calls the API new-style.)
• WWAction translates 'action-jobid' and 'action-filename' to their simpler counterparts before calling the API function.
• It calls the API function.
• If the return data contains error data, return the error message it contains. if not, return a simple success message.

Now I look over the existing WWAction stuff for anything that doesn't fit that pattern.

• 'merge-from-background' => 'ww-merge-background-job'
• 'background-make' => 'ww-create-background-job'
• does WWAction need to catch UsageExceptions? I think I did need to do that in WWAction_merge_from_background.
• 'sync' => what? 'sync-source-file'? But will we actually want an API call for this? Maybe just a ww-sync-source-files api that can be called with a single filename?
• 'set-appears' => ?? - this is to record what pages a project file appears on. I'm skeptical this is used. I could better imagine it being used via the API actually. But in that case it should be one use of a more general purpose ww-set-project-file-data or something.
• 'set-archived', 'remove-archived' => 'ww-set-project-file-data', I think, with parameters telling it to add/remove an archived location for the project file.
• 'export-sf' and 'export-wd' => 'ww-export'? But also, careful with this one, because they produce raw .tar.gz output, and don't return data in the usual way.
• Some actions require edit permission. Make sure ApiMain::execute() handles this check, because it will be hard for WWAction to do it in this framework.
• 'remove-source-file', 'remove-archived', 'remove-project' have a more complex kind of confirmation, which depends on the existing project data (but maybe could be simplified).
• 'add-prerequisite-project', 'update-prerequisite-project' should be merged, and may want to call some kind of general 'ww-set-project-data' API.
• 'sync-all' => 'ww-sync-source-files' with appropriate parameters.
• 'make' => 'ww-get-project-file' with appropriate parameters
• 'remove-and-remake' => 'ww-remove-project-file' + 'ww-get-project-file'?

To summarize:

• I think there are enough non-1-1 mappings that I need to have the option to have a WWAction subclass provide an implementation (which should do its work by calling 1 or more API actions), but have WWAction call the API directly if there isn't a subclass.
• The ones that have complex confirmations can do it in a WWAction subclass, I guess.
• Some of the name mismatches can be resolved by just changing the WWAction names.
• Not sure what to do with the output of ww-export. Is it useful for these to be API calls? Do I even want them to be WWAction calls? I was wanting to redesign this feature as a Special:Export page allowing more customization of the output.

[Lee Worden]

How many actions are there that will work the default way? Is it worth having a default way?

• set-source-file-location,
• remove-appears (given possible renaming)
• clear-directory
• add-prerequisite-project, update-prerequisite-project (given merging on the WWAction/ManageProject side)
• sync and sync-all given renaming and reorganizing parameters on the WWAction side
• prune-working-directories (which should be redesigned deeper and maybe shouldn't be a WWAction anymore)
• destroy-background-job, kill-background-job
• background-make, merge-from-background (rename)

So yes, it does seem worth it.

With the default, I'll either need to provide a whitelist of API actions that can be called in the default way as WWActions, or there'll be a risk of it calling API actions that aren't intended to work that way. Given that the API actions can be trusted to do permissions checking, though, it doesn't seem like much of a problem. It would be a problem if an API action produces weird output, like the raw .tar.gz output of export-wd or if one produces SSE output by default... But even so, it'd only be a problem if someone hand-constructs an index.php URL with the unsupported action name in it, and they can be responsible for the consequences...

Steps:

• Do the coding and testing offsite - too disruptive for editing live on lalashan.
• Create the default framework, and make it work for destroy-background-job and other actions that I've already reimplemented in API form.
• Convert all the other actions to API form, and replace the Action version of each by API calls, one by one.
• Rename as needed for compatibility with the planned API action names.
• Make the buttons and links that call these actions into ajax links calling the API (falling back to the existing WWAction links in non-JS browsers).
• Commit each to SVN and deploy to lalashan once it's converted and tested.
• Generalize the JS code as needed along the way. Some is already being developed in ext.workingwiki.background.js and should be moved to ext.workingwiki.js for use with non-background ajax links. Make a clean framework for installing ajax click handlers on links and buttons with a fallback to action links, give them graceful handling when clicked while the page is still loading, do ajax API calls in a standard way with clean error handling, confirming, spinner handling, etc.

[Lee Worden]

Have created the default framework and am using it for destroy-background-job.

The newly invented array $wwConfirmationsForApiActions controls which actions come with a confirmation step, and the text that's used for the confirmation. (though not in more complex confirmations, like the 3-way questions that remove-source-file uses.)

I think the next step is to pass that array to the JS side and use it there as well.

[Lee Worden]

the JS now has access to $wwConfirmationsForApiActions and code to use it, and a bit more infrastructure for general API usage. It's not in use yet - ww-destroy-background-job is all ready to use it, but I want to have more time on my hands for debugging when I switch it over.

[Lee Worden]

even though I said I was going to work on it offline :(