Patchwork [STABLE] phabricator: properly encode boolean types in the request body

login
register
mail settings
Submitter Matt Harbison
Date Dec. 21, 2018, 11:15 p.m.
Message ID <366889e199653dad6052.1545434140@Envy>
Download mbox | patch
Permalink /patch/37308/
State Superseded
Headers show

Comments

Matt Harbison - Dec. 21, 2018, 11:15 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1545431772 18000
#      Fri Dec 21 17:36:12 2018 -0500
# Node ID 366889e199653dad6052970c48a2bd2ae62e5e1d
# Parent  6f483b107eb5ee100d8a308cb6216da576fc4c41
phabricator: properly encode boolean types in the request body

I tripped over this playing with `hg debugcallconduit` to query for valid
reviewers.  If the JSON on stdin is written as 'True' or 'False', python
complains it isn't valid JSON.  If it's written as 'true' or 'false', it made it
to the server, but got kicked back with this:

    abort: Conduit Error (ERR-CONDUIT-CORE): Error while reading "isBot":
           Expected boolean (true or false), got something else.

The test isn't really relevant here (the code can be reverted, and it will
pass), but this gives us coverage for the debug command.
Yuya Nishihara - Dec. 22, 2018, 7:44 a.m.
On Fri, 21 Dec 2018 18:15:40 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1545431772 18000
> #      Fri Dec 21 17:36:12 2018 -0500
> # Node ID 366889e199653dad6052970c48a2bd2ae62e5e1d
> # Parent  6f483b107eb5ee100d8a308cb6216da576fc4c41
> phabricator: properly encode boolean types in the request body
> 
> I tripped over this playing with `hg debugcallconduit` to query for valid
> reviewers.  If the JSON on stdin is written as 'True' or 'False', python
> complains it isn't valid JSON.  If it's written as 'true' or 'false', it made it
> to the server, but got kicked back with this:
> 
>     abort: Conduit Error (ERR-CONDUIT-CORE): Error while reading "isBot":
>            Expected boolean (true or false), got something else.
> 
> The test isn't really relevant here (the code can be reverted, and it will
> pass), but this gives us coverage for the debug command.
> 
> diff --git a/hgext/phabricator.py b/hgext/phabricator.py
> --- a/hgext/phabricator.py
> +++ b/hgext/phabricator.py
> @@ -160,6 +160,8 @@ def urlencodenested(params):
>              flatparams[prefix] = obj
>          else:
>              for k, v in items(obj):
> +                if isinstance(v, bool):
> +                    v = { True: b'true', False: b'false' }[v]

It's probably better to consolidate this into the main type dispatch.

  if isinstance(obj, bool):
      obj = <convert obj to PHP-compatible notation>
  ...

Patch

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -160,6 +160,8 @@  def urlencodenested(params):
             flatparams[prefix] = obj
         else:
             for k, v in items(obj):
+                if isinstance(v, bool):
+                    v = { True: b'true', False: b'false' }[v]
                 if prefix:
                     process(b'%s[%s]' % (prefix, k), v)
                 else:
diff --git a/tests/phabricator/phab-conduit.json b/tests/phabricator/phab-conduit.json
new file mode 100644
--- /dev/null
+++ b/tests/phabricator/phab-conduit.json
@@ -0,0 +1,73 @@ 
+{
+    "interactions": [
+        {
+            "response": {
+                "status": {
+                    "message": "OK", 
+                    "code": 200
+                }, 
+                "headers": {
+                    "content-type": [
+                        "application/json"
+                    ], 
+                    "date": [
+                        "Fri, 21 Dec 2018 22:19:11 GMT"
+                    ], 
+                    "x-content-type-options": [
+                        "nosniff"
+                    ], 
+                    "cache-control": [
+                        "no-store"
+                    ], 
+                    "strict-transport-security": [
+                        "max-age=0; includeSubdomains; preload"
+                    ], 
+                    "x-frame-options": [
+                        "Deny"
+                    ], 
+                    "set-cookie": [
+                        "phsid=A%2Fdv22bpksbdis3vfeksluagfslhfojblbnkro7we4; expires=Wed, 20-Dec-2023 22:19:11 GMT; Max-Age=157680000; path=/; domain=phab.mercurial-scm.org; secure; httponly"
+                    ], 
+                    "x-xss-protection": [
+                        "1; mode=block"
+                    ], 
+                    "expires": [
+                        "Sat, 01 Jan 2000 00:00:00 GMT"
+                    ], 
+                    "transfer-encoding": [
+                        "chunked"
+                    ], 
+                    "server": [
+                        "Apache/2.4.10 (Debian)"
+                    ]
+                }, 
+                "body": {
+                    "string": "{\"result\":{\"data\":[],\"maps\":{},\"query\":{\"queryKey\":null},\"cursor\":{\"limit\":100,\"after\":null,\"before\":null,\"order\":null}},\"error_code\":null,\"error_info\":null}"
+                }
+            }, 
+            "request": {
+                "method": "POST", 
+                "headers": {
+                    "accept": [
+                        "application/mercurial-0.1"
+                    ], 
+                    "content-type": [
+                        "application/x-www-form-urlencoded"
+                    ], 
+                    "content-length": [
+                        "70"
+                    ], 
+                    "host": [
+                        "phab.mercurial-scm.org"
+                    ], 
+                    "user-agent": [
+                        "mercurial/proto-1.0 (Mercurial 4.8.1+564-6f483b107eb5+20181221)"
+                    ]
+                }, 
+                "uri": "https://phab.mercurial-scm.org//api/user.search", 
+                "body": "constraints%5BisBot%5D=true&api.token=cli-hahayouwish"
+            }
+        }
+    ], 
+    "version": 1
+}
\ No newline at end of file
diff --git a/tests/test-phabricator.t b/tests/test-phabricator.t
--- a/tests/test-phabricator.t
+++ b/tests/test-phabricator.t
@@ -65,6 +65,27 @@  Create a differential diff:
   D4597 - created - 1a5640df7bbf: create beta for phabricator test
   saved backup bundle to $TESTTMP/repo/.hg/strip-backup/1a5640df7bbf-6daf3e6e-phabsend.hg
 
+  $ hg debugcallconduit user.search --test-vcr "$VCR/phab-conduit.json" <<EOF
+  > {
+  >     "constraints": {
+  >         "isBot": true
+  >     }
+  > }
+  > EOF
+  {
+    "cursor": {
+      "after": null,
+      "before": null,
+      "limit": 100,
+      "order": null
+    },
+    "data": [],
+    "maps": {},
+    "query": {
+      "queryKey": null
+    }
+  }
+
 Template keywords
   $ hg log -T'{rev} {phabreview|json}\n'
   1 {"id": "D4597", "url": "https://phab.mercurial-scm.org/D4597"}