# HG changeset patch # User Gregory Szorc # Date 2018-04-07 00:14:06 # Node ID 69e46c1834ac1896e34c5984c49ba4555717acd8 # Parent 465187fec06f196fdf1249df52adfff91754ecfd wireproto: define and expose types of wire command arguments Exposing the set of argument names is cool. But with wire protocol version 2, we're using CBOR to transport arguments and this allows us to have typing for arguments. Typed arguments are much nicer because they will cut down on transfer overhead and processing overhead for decoding values. This commit teaches @wireprotocommand to accept a dictionary for arguments. The arguments registered for version 2 transports are canonically stored as dictionaries rather than a space-delimited string. It is an error to defined arguments with a dictionary for commands using version 1 transports. This reinforces my intent to fully decouple command handlers for version 2 transports. Differential Revision: https://phab.mercurial-scm.org/D3202 diff --git a/mercurial/help/internals/wireprotocol.txt b/mercurial/help/internals/wireprotocol.txt --- a/mercurial/help/internals/wireprotocol.txt +++ b/mercurial/help/internals/wireprotocol.txt @@ -1712,7 +1712,12 @@ commands are: args - An array of argument names accepted by this command. + A map of argument names and their expected types. + + Types are defined as a representative value for the expected type. + e.g. an argument expecting a boolean type will have its value + set to true. An integer type will have its value set to 42. The + actual values are arbitrary and may not have meaning. permissions An array of permissions required to execute this command. diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py --- a/mercurial/wireproto.py +++ b/mercurial/wireproto.py @@ -713,8 +713,11 @@ def wireprotocommand(name, args='', tran ``name`` is the name of the wire protocol command being provided. - ``args`` is a space-delimited list of named arguments that the command - accepts. ``*`` is a special value that says to accept all arguments. + ``args`` defines the named arguments accepted by the command. It is + ideally a dict mapping argument names to their types. For backwards + compatibility, it can be a space-delimited list of argument names. For + version 1 transports, ``*`` denotes a special value that says to accept + all named arguments. ``transportpolicy`` is a POLICY_* constant denoting which transports this wire protocol command should be exposed to. By default, commands @@ -752,6 +755,17 @@ def wireprotocommand(name, args='', tran 'got %s; expected "push" or "pull"' % permission) + if 1 in transportversions and not isinstance(args, bytes): + raise error.ProgrammingError('arguments for version 1 commands must ' + 'be declared as bytes') + + if isinstance(args, bytes): + dictargs = {arg: b'legacy' for arg in args.split()} + elif isinstance(args, dict): + dictargs = args + else: + raise ValueError('args must be bytes or a dict') + def register(func): if 1 in transportversions: if name in commands: @@ -764,7 +778,8 @@ def wireprotocommand(name, args='', tran if name in commandsv2: raise error.ProgrammingError('%s command already registered ' 'for version 2' % name) - commandsv2[name] = commandentry(func, args=args, + + commandsv2[name] = commandentry(func, args=dictargs, transports=transports, permission=permission) @@ -1304,7 +1319,7 @@ def _capabilitiesv2(repo, proto): for command, entry in commandsv2.items(): caps['commands'][command] = { - 'args': sorted(entry.args.split()) if entry.args else [], + 'args': entry.args, 'permissions': [entry.permission], } @@ -1325,7 +1340,11 @@ def capabilitiesv2(repo, proto): return wireprototypes.cborresponse(caps) -@wireprotocommand('heads', args='publiconly', permission='pull', +@wireprotocommand('heads', + args={ + 'publiconly': False, + }, + permission='pull', transportpolicy=POLICY_V2_ONLY) def headsv2(repo, proto, publiconly=False): if publiconly: @@ -1333,14 +1352,22 @@ def headsv2(repo, proto, publiconly=Fals return wireprototypes.cborresponse(repo.heads()) -@wireprotocommand('known', 'nodes', permission='pull', +@wireprotocommand('known', + args={ + 'nodes': [b'deadbeef'], + }, + permission='pull', transportpolicy=POLICY_V2_ONLY) def knownv2(repo, proto, nodes=None): nodes = nodes or [] result = b''.join(b'1' if n else b'0' for n in repo.known(nodes)) return wireprototypes.cborresponse(result) -@wireprotocommand('listkeys', 'namespace', permission='pull', +@wireprotocommand('listkeys', + args={ + 'namespace': b'ns', + }, + permission='pull', transportpolicy=POLICY_V2_ONLY) def listkeysv2(repo, proto, namespace=None): keys = repo.listkeys(encoding.tolocal(namespace)) diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py --- a/mercurial/wireprotoserver.py +++ b/mercurial/wireprotoserver.py @@ -606,10 +606,11 @@ class httpv2protocolhandler(object): def getargs(self, args): data = {} - for k in args.split(): + for k, typ in args.items(): if k == '*': raise NotImplementedError('do not support * args') elif k in self._args: + # TODO consider validating value types. data[k] = self._args[k] return data diff --git a/tests/test-wireproto-command-capabilities.t b/tests/test-wireproto-command-capabilities.t --- a/tests/test-wireproto-command-capabilities.t +++ b/tests/test-wireproto-command-capabilities.t @@ -30,11 +30,11 @@ capabilities request returns an array of s> \r\n s> *\r\n (glob) s> *\x00\x01\x00\x02\x01F (glob) - s> \xa2Hcommands\xaaEheads\xa2Dargs\x81JpubliconlyKpermissions\x81DpullEknown\xa2Dargs\x81EnodesKpermissions\x81DpullFlookup\xa2Dargs\x81CkeyKpermissions\x81DpullGpushkey\xa2Dargs\x84CkeyInamespaceCnewColdKpermissions\x81DpushHlistkeys\xa2Dargs\x81InamespaceKpermissions\x81DpullHunbundle\xa2Dargs\x81EheadsKpermissions\x81DpushIbranchmap\xa2Dargs\x80Kpermissions\x81DpullIgetbundle\xa2Dargs\x81A*Kpermissions\x81DpullLcapabilities\xa2Dargs\x80Kpermissions\x81DpullLclonebundles\xa2Dargs\x80Kpermissions\x81DpullKcompression\x82\xa1DnameDzstd\xa1DnameDzlib + s> \xa2Hcommands\xaaEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyFlegacyKpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyFlegacyCnewFlegacyColdFlegacyInamespaceFlegacyKpermissions\x81DpushHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullHunbundle\xa2Dargs\xa1EheadsFlegacyKpermissions\x81DpushIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullIgetbundle\xa2Dargs\xa1A*FlegacyKpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullLclonebundles\xa2Dargs\xa0Kpermissions\x81DpullKcompression\x82\xa1DnameDzstd\xa1DnameDzlib s> \r\n received frame(size=*; request=1; stream=2; streamflags=stream-begin; type=bytes-response; flags=eos|cbor) (glob) s> 0\r\n s> \r\n - response: [{b'commands': {b'branchmap': {b'args': [], b'permissions': [b'pull']}, b'capabilities': {b'args': [], b'permissions': [b'pull']}, b'clonebundles': {b'args': [], b'permissions': [b'pull']}, b'getbundle': {b'args': [b'*'], b'permissions': [b'pull']}, b'heads': {b'args': [b'publiconly'], b'permissions': [b'pull']}, b'known': {b'args': [b'nodes'], b'permissions': [b'pull']}, b'listkeys': {b'args': [b'namespace'], b'permissions': [b'pull']}, b'lookup': {b'args': [b'key'], b'permissions': [b'pull']}, b'pushkey': {b'args': [b'key', b'namespace', b'new', b'old'], b'permissions': [b'push']}, b'unbundle': {b'args': [b'heads'], b'permissions': [b'push']}}, b'compression': [{b'name': b'zstd'}, {b'name': b'zlib'}]}] + response: [{b'commands': {b'branchmap': {b'args': {}, b'permissions': [b'pull']}, b'capabilities': {b'args': {}, b'permissions': [b'pull']}, b'clonebundles': {b'args': {}, b'permissions': [b'pull']}, b'getbundle': {b'args': {b'*': b'legacy'}, b'permissions': [b'pull']}, b'heads': {b'args': {b'publiconly': False}, b'permissions': [b'pull']}, b'known': {b'args': {b'nodes': [b'deadbeef']}, b'permissions': [b'pull']}, b'listkeys': {b'args': {b'namespace': b'ns'}, b'permissions': [b'pull']}, b'lookup': {b'args': {b'key': b'legacy'}, b'permissions': [b'pull']}, b'pushkey': {b'args': {b'key': b'legacy', b'namespace': b'legacy', b'new': b'legacy', b'old': b'legacy'}, b'permissions': [b'push']}, b'unbundle': {b'args': {b'heads': b'legacy'}, b'permissions': [b'push']}}, b'compression': [{b'name': b'zstd'}, {b'name': b'zlib'}]}] $ cat error.log