Patchwork D4283: webcommands: fix `@webcommand` decorator

login
register
mail settings
Submitter phabricator
Date Aug. 15, 2018, 7 p.m.
Message ID <differential-rev-PHID-DREV-p76b32jbbk5lsydvrvbu-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/33749/
State New
Headers show

Comments

phabricator - Aug. 15, 2018, 7 p.m.
sheehan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4283

AFFECTED FILES
  hgext/highlight/__init__.py
  hgext/keyword.py
  hgext/largefiles/uisetup.py
  mercurial/hgweb/hgweb_mod.py
  mercurial/hgweb/webcommands.py
  tests/hgweberror.py

CHANGE DETAILS




To: sheehan, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 16, 2018, 12:22 a.m.
indygreg added a comment.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4283

To: sheehan, #hg-reviewers
Cc: indygreg, mercurial-devel
Katsunori FUJIWARA - Aug. 18, 2018, 6:18 a.m.
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,
                              '4.9')  # or 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 :-)


> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -740,7 +740,7 @@
>      extensions.wrapfunction(cmdutil, 'copy', kw_copy)
>      extensions.wrapfunction(cmdutil, 'dorecord', kw_dorecord)
>      for c in nokwwebcommands.split():
> -        extensions.wrapfunction(webcommands, c, kwweb_skip)
> +        webcommands.wrapwebcommand(c, kwweb_skip)
>  
>  def reposetup(ui, repo):
>      '''Sets up repo as kwrepo for keyword substitution.'''
> diff --git a/hgext/highlight/__init__.py b/hgext/highlight/__init__.py
> --- a/hgext/highlight/__init__.py
> +++ b/hgext/highlight/__init__.py
> @@ -77,6 +77,7 @@
>  
>      return orig(web)
>  
> +@webcommands.webcommand('highlightcss')
>  def generate_css(web):
>      pg_style = web.config('web', 'pygments_style', 'colorful')
>      fmter = highlight.HtmlFormatter(style=pg_style)
> @@ -91,6 +92,4 @@
>      # monkeypatch in the new version
>      extensions.wrapfunction(webcommands, '_filerevision',
>                              filerevision_highlight)
> -    extensions.wrapfunction(webcommands, 'annotate', annotate_highlight)
> -    webcommands.highlightcss = generate_css
> -    webcommands.__all__.append('highlightcss')
> +    webcommands.wrapwebcommand('annotate', annotate_highlight)
> 
> 
> 
> To: sheehan, #hg-reviewers
> Cc: mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
phabricator - Aug. 20, 2018, 1:46 p.m.
sheehan planned changes to this revision.
sheehan added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4283

To: sheehan, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Aug. 22, 2018, 1:11 a.m.
foozy added a comment.


  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:]
  >     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:
  >   1. Set some globals appropriate for web handlers. Commands can
  >   2. 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 :-)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4283

To: sheehan, #hg-reviewers
Cc: foozy, indygreg, mercurial-devel

Patch

diff --git a/tests/hgweberror.py b/tests/hgweberror.py
--- a/tests/hgweberror.py
+++ b/tests/hgweberror.py
@@ -6,6 +6,7 @@ 
     webcommands,
 )
 
+@webcommands.webcommand('raiseerror')
 def raiseerror(web):
     '''Dummy web command that raises an uncaught Exception.'''
 
@@ -18,7 +19,3 @@ 
         web.res.getbodyfile().write(b'partial content\n')
 
     raise AttributeError('I am an uncaught error!')
-
-def extsetup(ui):
-    setattr(webcommands, 'raiseerror', raiseerror)
-    webcommands.__all__.append('raiseerror')
diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
--- a/mercurial/hgweb/webcommands.py
+++ b/mercurial/hgweb/webcommands.py
@@ -8,6 +8,7 @@ 
 from __future__ import absolute_import
 
 import copy
+import functools
 import mimetypes
 import os
 import re
@@ -47,7 +48,6 @@ 
     webutil,
 )
 
-__all__ = []
 commands = {}
 
 class webcommand(object):
@@ -77,10 +77,34 @@ 
         self.name = name
 
     def __call__(self, func):
-        __all__.append(self.name)
         commands[self.name] = 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')
 def log(web):
     """
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)
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)
 
diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -740,7 +740,7 @@ 
     extensions.wrapfunction(cmdutil, 'copy', kw_copy)
     extensions.wrapfunction(cmdutil, 'dorecord', kw_dorecord)
     for c in nokwwebcommands.split():
-        extensions.wrapfunction(webcommands, c, kwweb_skip)
+        webcommands.wrapwebcommand(c, kwweb_skip)
 
 def reposetup(ui, repo):
     '''Sets up repo as kwrepo for keyword substitution.'''
diff --git a/hgext/highlight/__init__.py b/hgext/highlight/__init__.py
--- a/hgext/highlight/__init__.py
+++ b/hgext/highlight/__init__.py
@@ -77,6 +77,7 @@ 
 
     return orig(web)
 
+@webcommands.webcommand('highlightcss')
 def generate_css(web):
     pg_style = web.config('web', 'pygments_style', 'colorful')
     fmter = highlight.HtmlFormatter(style=pg_style)
@@ -91,6 +92,4 @@ 
     # monkeypatch in the new version
     extensions.wrapfunction(webcommands, '_filerevision',
                             filerevision_highlight)
-    extensions.wrapfunction(webcommands, 'annotate', annotate_highlight)
-    webcommands.highlightcss = generate_css
-    webcommands.__all__.append('highlightcss')
+    webcommands.wrapwebcommand('annotate', annotate_highlight)