Page MenuHomePhabricator

webcommands: fix `@webcommand` decorator
Changes PlannedPublic

Authored by sheehan on Aug 15 2018, 3:00 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This commit fixes the @webcommand decorator used to define
webcommands. Although webcommands are currently using this
decorator, it does not behave as intended. Using the decorator
should record the name of the decorator in a commands dict,
and when a request arrives to hgweb the correct function to
call should be resolved from that dict.

In the current implementation the command is added to the
commands dict, but when hgweb attempts to resolve the function
while processing a request, it instead looks for an attribute
with the same name attached to the webcommands module. To
summarize, a command 'commandname' should be resolved from

`mercurial.hgweb.webcommands.commands['commandname']`

but is instead being resolved from

`mercurial.hgweb.webcommands.commandname`

The decorator appears to be working on the core webcommands
(such as log, rev, file) however this is because those
commands are defined with the same name in the underlying
Python function.

This commit changes hgweb_mod.py to resolve the function used
from webcommands to come from the commands dict in the
webcommands module. Due to the change in how webcommands
are resolved, a wrapwebcommand function is also added, which
wraps a webcommand by applying the wrapper as a partial function
and overwrites the existing function in the commands dict.

Wrapped webcommands in shipped extensions and hgweb tests
are changed to use the fixed decorator and wrapwebcommand
function.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sheehan created this revision.Aug 15 2018, 3:00 PM

This seems like a strict improvement.

But the proper way to register web commands from extensions would be to go through the registrar API and have the extension loader look for a well-named symbol in each extension module that is loaded and hgweb would consult the registrar for active commands. In theory, this will only activate web commands on repositories that have an extension loaded.

Search for templatefilter in mercurial/extensions.py for an example of how all this works.

Would you be willing to try that approach? It doesn't have to be perfect. But we are moving to the registrar for extensions wishing to install well-defined things. And web commands fit that bill.

sheehan planned changes to this revision.Aug 20 2018, 9:46 AM

This seems like a strict improvement.
But the proper way to register web commands from extensions would be to go through the registrar API and have the extension loader look for a well-named symbol in each extension module that is loaded and hgweb would consult the registrar for active commands. In theory, this will only activate web commands on repositories that have an extension loaded.
Search for templatefilter in mercurial/extensions.py for an example of how all this works.
Would you be willing to try that approach? It doesn't have to be perfect. But we are moving to the registrar for extensions wishing to install well-defined things. And web commands fit that bill.

Seems reasonable to me! I took a look through the code and I think I understand what is going on. I'll update this patch with something that uses the registrar API.

foozy added a subscriber: foozy.Aug 21 2018, 9:11 PM

Sorry, I overlooked delivery error of reply to phabricator.

This is re-sending of the reply at Sat, 18 Aug 2018 15:18:10 +0900,
which was cc-ed to mercurial-devel@mercurial-scm.org.

At Wed, 15 Aug 2018 19:00:51 +0000,
sheehan (Connor Sheehan) wrote:
[snip]

diff --git a/mercurial/hgweb/hgweb_mod.py b/mercurial/hgweb/hgweb_mod.py

  • a/mercurial/hgweb/hgweb_mod.py

+++ b/mercurial/hgweb/hgweb_mod.py
@@ -354,7 +354,7 @@

    cmd = cmd[style + 1:]
# avoid accepting e.g. style parameter as command
  • if util.safehasattr(webcommands, cmd):

+ if cmd in webcommands.commands:

    req.qsparams['cmd'] = cmd
if cmd == 'static':

@@ -416,15 +416,15 @@

res.headers['ETag'] = tag
  • if cmd not in webcommands.all:

+ if cmd not in webcommands.commands:

    msg = 'no such method: %s' % cmd
    raise ErrorResponse(HTTP_BAD_REQUEST, msg)
else:
    # Set some globals appropriate for web handlers. Commands can
    # override easily enough.
    res.status = '200 Script output follows'
    res.headers['Content-Type'] = ctype
  • return getattr(webcommands, cmd)(rctx)

+ return webcommands.commands[cmd](rctx)

except (error.LookupError, error.RepoLookupError) as err:
    msg = pycompat.bytestr(err)

IMHO, for backward compatibility, this kind of (API) change should
have deprecation period before stopping support, and should use
ui.deprecwan() while such period, like below (this implementation
assumes introduction of new decorator registrar.webcommand).

def _getwebcommand(ui, name):
    cmd = webcommands.commands.get(name, None)
    if not cmd:
        if (name in webcommands.__all__ and
            util.safehasattr(webcommands, name)):
            cmd = getattr(webcommands, name)
            # store into webcommands.commands for subsequent access
            webcommands.commands[name] = cmd

            ui.deprecwarn('legacy registration for webcommand %s,'
                          ' use registrar.webcommand decorator' % name,
                          '5.0')
    return cmd

diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py

  • a/hgext/largefiles/uisetup.py

+++ b/hgext/largefiles/uisetup.py
@@ -151,7 +151,7 @@

extensions.wrapfunction(archival, 'archive', overrides.overridearchive)
extensions.wrapfunction(subrepo.hgsubrepo, 'archive',
                        overrides.hgsubrepoarchive)
  • extensions.wrapfunction(webcommands, 'archive', overrides.hgwebarchive)

+ webcommands.wrapwebcommand('archive', overrides.hgwebarchive)

extensions.wrapfunction(cmdutil, 'bailifchanged',
                        overrides.overridebailifchanged)

After changes above, direct invocation of original webcommand function
'webcommands.archive()' from another code path avoids wrapping effect,
because 'wrapwebcommand' wraps only the function stored in dispatch
table 'webcommands.command'.

Not for webcommand wrapping example, but for normal command wrapping
example, largefiles extension wraps both 'rebase' command and 'rebase'
function. In largefiles/uisetup.py::

if name == 'rebase':
    extensions.wrapcommand(getattr(module, 'cmdtable'), 'rebase',
        overrides.overriderebase)
    extensions.wrapfunction(module, 'rebase',
                            overrides.overriderebase)

Of course, it will be rare that un-wrapped original function causes
problem in webcommand case. But "wrap function in dispatch table"
should be separated from "stop wrapping original webcommand function"
at first, IMHO.

I'm also working around webcommand registration, and will post patches
soon. But, of course, feel free to re-send your revised one, for
feedback between us :-)