This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4283

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
indygreg added a subscriber: indygreg.Aug 15 2018, 8:22 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
In D4283#65988, @indygreg wrote:

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 :-)

Revision Contents
Changeset List

Diff 10360

hgext/highlight/__init__.py

def annotate_highlight(orig, web): def annotate_highlight(orig, web):
mt = web.res.headers['Content-Type'] mt = web.res.headers['Content-Type']
if 'html' in mt: if 'html' in mt:
fctx = webutil.filectx(web.repo, web.req) fctx = webutil.filectx(web.repo, web.req)
pygmentize(web, 'annotateline', fctx, web.tmpl) pygmentize(web, 'annotateline', fctx, web.tmpl)
return orig(web) return orig(web)
@webcommands.webcommand('highlightcss')
def generate_css(web): def generate_css(web):
pg_style = web.config('web', 'pygments_style', 'colorful') pg_style = web.config('web', 'pygments_style', 'colorful')
fmter = highlight.HtmlFormatter(style=pg_style) fmter = highlight.HtmlFormatter(style=pg_style)
web.res.headers['Content-Type'] = 'text/css' web.res.headers['Content-Type'] = 'text/css'
web.res.setbodybytes(''.join([ web.res.setbodybytes(''.join([
'/* pygments_style = %s */\n\n' % pg_style, '/* pygments_style = %s */\n\n' % pg_style,
fmter.get_style_defs(''), fmter.get_style_defs(''),
])) ]))
return web.res.sendresponse() return web.res.sendresponse()
def extsetup(): def extsetup():
# monkeypatch in the new version # monkeypatch in the new version
extensions.wrapfunction(webcommands, '_filerevision', extensions.wrapfunction(webcommands, '_filerevision',
filerevision_highlight) filerevision_highlight)
extensions.wrapfunction(webcommands, 'annotate', annotate_highlight) webcommands.wrapwebcommand('annotate', annotate_highlight)
webcommands.highlightcss = generate_css
webcommands.__all__.append('highlightcss')

hgext/keyword.py

extensions.wrapfunction(context.filectx, 'cmp', kwfilectx_cmp) extensions.wrapfunction(context.filectx, 'cmp', kwfilectx_cmp)
extensions.wrapfunction(patch.patchfile, '__init__', kwpatchfile_init) extensions.wrapfunction(patch.patchfile, '__init__', kwpatchfile_init)
extensions.wrapfunction(patch, 'diff', kwdiff) extensions.wrapfunction(patch, 'diff', kwdiff)
extensions.wrapfunction(cmdutil, 'amend', kw_amend) extensions.wrapfunction(cmdutil, 'amend', kw_amend)
extensions.wrapfunction(cmdutil, 'copy', kw_copy) extensions.wrapfunction(cmdutil, 'copy', kw_copy)
extensions.wrapfunction(cmdutil, 'dorecord', kw_dorecord) extensions.wrapfunction(cmdutil, 'dorecord', kw_dorecord)
for c in nokwwebcommands.split(): for c in nokwwebcommands.split():
extensions.wrapfunction(webcommands, c, kwweb_skip) webcommands.wrapwebcommand(c, kwweb_skip)
def reposetup(ui, repo): def reposetup(ui, repo):
'''Sets up repo as kwrepo for keyword substitution.''' '''Sets up repo as kwrepo for keyword substitution.'''
try: try:
if (not repo.local() or kwtools['hgcmd'] in nokwcommands.split() if (not repo.local() or kwtools['hgcmd'] in nokwcommands.split()
or '.hg' in util.splitpath(repo.root) or '.hg' in util.splitpath(repo.root)
or repo._url.startswith('bundle:')): or repo._url.startswith('bundle:')):

hgext/largefiles/uisetup.py

extensions.wrapfunction(cmdutil, 'revert', overrides.overriderevert) extensions.wrapfunction(cmdutil, 'revert', overrides.overriderevert)
extensions.wrapcommand(commands.table, 'archive', extensions.wrapcommand(commands.table, 'archive',
overrides.overridearchivecmd) overrides.overridearchivecmd)
extensions.wrapfunction(archival, 'archive', overrides.overridearchive) extensions.wrapfunction(archival, 'archive', overrides.overridearchive)
extensions.wrapfunction(subrepo.hgsubrepo, 'archive', extensions.wrapfunction(subrepo.hgsubrepo, 'archive',
overrides.hgsubrepoarchive) overrides.hgsubrepoarchive)
extensions.wrapfunction(webcommands, 'archive', overrides.hgwebarchive) webcommands.wrapwebcommand('archive', overrides.hgwebarchive)
extensions.wrapfunction(cmdutil, 'bailifchanged', extensions.wrapfunction(cmdutil, 'bailifchanged',
overrides.overridebailifchanged) overrides.overridebailifchanged)
extensions.wrapfunction(cmdutil, 'postcommitstatus', extensions.wrapfunction(cmdutil, 'postcommitstatus',
overrides.postcommitstatus) overrides.postcommitstatus)
extensions.wrapfunction(scmutil, 'marktouched', extensions.wrapfunction(scmutil, 'marktouched',
overrides.scmutilmarktouched) overrides.scmutilmarktouched)

mercurial/hgweb/hgweb_mod.py

if 'cmd' not in req.qsparams and args and args[0]: if 'cmd' not in req.qsparams and args and args[0]:
cmd = args.pop(0) cmd = args.pop(0)
style = cmd.rfind('-') style = cmd.rfind('-')
if style != -1: if style != -1:
req.qsparams['style'] = cmd[:style] req.qsparams['style'] = cmd[:style]
cmd = cmd[style + 1:] cmd = cmd[style + 1:]
# avoid accepting e.g. style parameter as command # avoid accepting e.g. style parameter as command
if util.safehasattr(webcommands, cmd): if cmd in webcommands.commands:
req.qsparams['cmd'] = cmd req.qsparams['cmd'] = cmd
if cmd == 'static': if cmd == 'static':
req.qsparams['file'] = '/'.join(args) req.qsparams['file'] = '/'.join(args)
else: else:
if args and args[0]: if args and args[0]:
node = args.pop(0).replace('%2F', '/') node = args.pop(0).replace('%2F', '/')
req.qsparams['node'] = node req.qsparams['node'] = node
except KeyError: except KeyError:
pass pass
# Response body not allowed on 304. # Response body not allowed on 304.
res.setbodybytes('') res.setbodybytes('')
return res.sendresponse() return res.sendresponse()
res.headers['ETag'] = tag res.headers['ETag'] = tag
if cmd not in webcommands.__all__: if cmd not in webcommands.commands:
msg = 'no such method: %s' % cmd msg = 'no such method: %s' % cmd
raise ErrorResponse(HTTP_BAD_REQUEST, msg) raise ErrorResponse(HTTP_BAD_REQUEST, msg)
else: else:
# Set some globals appropriate for web handlers. Commands can # Set some globals appropriate for web handlers. Commands can
# override easily enough. # override easily enough.
res.status = '200 Script output follows' res.status = '200 Script output follows'
res.headers['Content-Type'] = ctype res.headers['Content-Type'] = ctype
return getattr(webcommands, cmd)(rctx) return webcommands.commands[cmd](rctx)
except (error.LookupError, error.RepoLookupError) as err: except (error.LookupError, error.RepoLookupError) as err:
msg = pycompat.bytestr(err) msg = pycompat.bytestr(err)
if (util.safehasattr(err, 'name') and if (util.safehasattr(err, 'name') and
not isinstance(err, error.ManifestLookupError)): not isinstance(err, error.ManifestLookupError)):
msg = 'revision not found: %s' % err.name msg = 'revision not found: %s' % err.name
res.status = '404 Not Found' res.status = '404 Not Found'

mercurial/hgweb/webcommands.py

# #
# Copyright 21 May 2005 - (c) 2005 Jake Edge <jake@edge2.net> # Copyright 21 May 2005 - (c) 2005 Jake Edge <jake@edge2.net>
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import copy import copy
import functools
import mimetypes import mimetypes
import os import os
import re import re
from ..i18n import _ from ..i18n import _
from ..node import hex, short from ..node import hex, short
from .common import ( from .common import (
from ..utils import ( from ..utils import (
stringutil, stringutil,
) )
from . import ( from . import (
webutil, webutil,
) )
__all__ = []
commands = {} commands = {}
class webcommand(object): class webcommand(object):
"""Decorator used to register a web command handler. """Decorator used to register a web command handler.
The decorator takes as its positional arguments the name/path the The decorator takes as its positional arguments the name/path the
command should be accessible under. command should be accessible under.
def mycommand(web): def mycommand(web):
pass pass
""" """
def __init__(self, name): def __init__(self, name):
self.name = name self.name = name
def __call__(self, func): def __call__(self, func):
__all__.append(self.name)
commands[self.name] = func commands[self.name] = func
return func return func
def wrapwebcommand(command, wrapper):
"""Utility to wrap functions defined as webcommands
Wrap the webcommand 'command' using the specified wrapper.
The wrapper function should have the signature
wrapper(orig, web)
where 'orig' is the webcommand to be wrapped, and 'web' is the
requestcontext instance that will be passed to the webcommand
at runtime."""
if command not in commands:
raise error.ProgrammingError('no webcommand named %s' % command)
if not callable(wrapper):
raise error.ProgrammingError('wrapper must be a callable')
# Create the new webcommand
orig = commands[command]
newfunc = functools.partial(wrapper, orig)
# Overwrite the old webcommand with the new one
commands[command] = newfunc
@webcommand('log') @webcommand('log')
def log(web): def log(web):
""" """
/log[/{revision}[/{path}]] /log[/{revision}[/{path}]]
-------------------------- --------------------------
Show repository or file history. Show repository or file history.

tests/hgweberror.py

# A dummy extension that installs an hgweb command that throws an Exception. # A dummy extension that installs an hgweb command that throws an Exception.
from __future__ import absolute_import from __future__ import absolute_import
from mercurial.hgweb import ( from mercurial.hgweb import (
webcommands, webcommands,
) )
@webcommands.webcommand('raiseerror')
def raiseerror(web): def raiseerror(web):
'''Dummy web command that raises an uncaught Exception.''' '''Dummy web command that raises an uncaught Exception.'''
# Simulate an error after partial response. # Simulate an error after partial response.
if 'partialresponse' in web.req.qsparams: if 'partialresponse' in web.req.qsparams:
web.res.status = b'200 Script output follows' web.res.status = b'200 Script output follows'
web.res.headers[b'Content-Type'] = b'text/plain' web.res.headers[b'Content-Type'] = b'text/plain'
web.res.setbodywillwrite() web.res.setbodywillwrite()
list(web.res.sendresponse()) list(web.res.sendresponse())
web.res.getbodyfile().write(b'partial content\n') web.res.getbodyfile().write(b'partial content\n')
raise AttributeError('I am an uncaught error!') raise AttributeError('I am an uncaught error!')
def extsetup(ui):
setattr(webcommands, 'raiseerror', raiseerror)
webcommands.__all__.append('raiseerror')