bug#24252: 25.1; json.el doesn't distinguish null and empty object

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

Hello,

When json-pretty-print applied to "{}", it is
unexpectedly converted to "null".
This is caused by internal representations of null
and empty object are the same:
  (json-read-from-string "{}") ; => nil
  (json-read-from-string "null") ; => nil

;; Based on the fact that
;;   (let ((json-object-type 'hash-table))
;;     (json-read-from-string "{}"))
;; is non-nil, there was a workaround in the past.
;; The current json-pretty-print bind it to alist
;; to keep ordering of elements, so the technique
;; no longer works.

Following patch make json.el to treat empty object
and null differently and solve the issue.

--
Yoichi Nakayama
\f
From 6b0ec686dab49be2309ed2dd349e31695b7cc6f2 Mon Sep 17 00:00:00 2001
From: Yoichi Nakayama <yoichi.nakayama@gmail.com>
Date: Wed, 17 Aug 2016 01:18:56 +0900
Subject: [PATCH] Distinguish empty json object and null

* lisp/json.el (json-empty-object): New variable.
(json-new-object, json-add-to-object, json-read-object, json-encode):
Use it.
---
 lisp/json.el | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/lisp/json.el b/lisp/json.el
index fdac8d9..ec2c06a 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -89,6 +89,12 @@ If this has the same value as `json-null', you might not be able to tell
 the difference between `false' and `null'.  Consider let-binding this
 around your call to `json-read' instead of `setq'ing it.")
 
+(defvar json-empty-object :json-empty-object
+  "Value to use when reading JSON `{}'.
+If this has the same value as `json-null', you might not be able to tell
+the difference between `{}' and `null'.  Consider let-binding this
+around your call to `json-read' instead of `setq'ing it.")
+
 (defvar json-null nil
   "Value to use when reading JSON `null'.
 If this has the same value as `json-false', you might not be able to
@@ -442,7 +448,7 @@ Please see the documentation of `json-object-type'."
   (cond ((eq json-object-type 'hash-table)
          (make-hash-table :test 'equal))
         (t
-         ())))
+         json-empty-object)))
 
 (defun json-add-to-object (object key value)
   "Add a new KEY -> VALUE association to OBJECT.
@@ -454,7 +460,9 @@ Please see the documentation of `json-object-type' and `json-key-type'."
              (cdr (assq json-object-type '((hash-table . string)
                                            (alist . symbol)
                                            (plist . keyword))))
-           json-key-type)))
+           json-key-type))
+        (object (cond ((eq object json-empty-object) ())
+                      (t object))))
     (setq key
           (cond ((eq json-key-type 'string)
                  key)
@@ -501,10 +509,12 @@ Please see the documentation of `json-object-type' and `json-key-type'."
           (signal 'json-object-format (list "," (json-peek))))))
     ;; Skip over the "}"
     (json-advance)
-    (pcase json-object-type
-      (`alist (nreverse elements))
-      (`plist (json--plist-reverse elements))
-      (_ elements))))
+    (cond ((eq elements json-empty-object) elements)
+          (t
+           (pcase json-object-type
+             (`alist (nreverse elements))
+             (`plist (json--plist-reverse elements))
+             (_ elements))))))
 
 ;; Hash table encoding
 
@@ -697,6 +707,7 @@ Advances point just past JSON object."
   "Return a JSON representation of OBJECT as a string."
   (cond ((memq object (list t json-null json-false))
          (json-encode-keyword object))
+        ((eq object json-empty-object) "{}")
         ((stringp object)      (json-encode-string object))
         ((keywordp object)     (json-encode-string
                                 (substring (symbol-name object) 1)))
-- 
2.8.1

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Dmitry Gutov]

Hi!

On 08/17/2016 05:54 PM, Yoichi Nakayama wrote:

> When json-pretty-print applied to "{}", it is
> unexpectedly converted to "null".
> This is caused by internal representations of null
> and empty object are the same:
>   (json-read-from-string "{}") ; => nil
>   (json-read-from-string "null") ; => nil

Why don't you bind json-null to whatever value you need? Then the 
results will be different:

ELISP> (let ((json-null 'NULL)) (json-read-from-string "null"))
NULL

> +(defvar json-empty-object :json-empty-object
> +  "Value to use when reading JSON `{}'.
> +If this has the same value as `json-null', you might not be able to tell
> +the difference between `{}' and `null'.  Consider let-binding this
> +around your call to `json-read' instead of `setq'ing it.")

This doesn't look like a good default value, at least. It's not one of 
the types that we parse JSON objects to (alist, plist, hash-table). This 
will break real code.

By the way, another option to distinguish nil and {} is to bind 
json-object-type to `hash-table'. An empty hash table is not nil.

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

> Why don't you bind json-null to whatever value you need?

(let ((json-null 'NULL)) (json-encode (json-read-from-string "{}")))
"\"nil\""

> By the way, another option to distinguish nil and {} is to bind json-object-type to `hash-table'. An empty hash table is not nil.

It does not work because json-pretty-print overwrites json-object-type.

On Fri, Aug 19, 2016 at 11:06 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Hi!
>
> On 08/17/2016 05:54 PM, Yoichi Nakayama wrote:
>
>> When json-pretty-print applied to "{}", it is
>> unexpectedly converted to "null".
>> This is caused by internal representations of null
>> and empty object are the same:
>>   (json-read-from-string "{}") ; => nil
>>   (json-read-from-string "null") ; => nil
>
>
> Why don't you bind json-null to whatever value you need? Then the results
> will be different:
>
> ELISP> (let ((json-null 'NULL)) (json-read-from-string "null"))
> NULL
>
>> +(defvar json-empty-object :json-empty-object
>> +  "Value to use when reading JSON `{}'.
>> +If this has the same value as `json-null', you might not be able to tell
>> +the difference between `{}' and `null'.  Consider let-binding this
>> +around your call to `json-read' instead of `setq'ing it.")
>
>
> This doesn't look like a good default value, at least. It's not one of the
> types that we parse JSON objects to (alist, plist, hash-table). This will
> break real code.
>
> By the way, another option to distinguish nil and {} is to bind
> json-object-type to `hash-table'. An empty hash table is not nil.



-- 
Yoichi NAKAYAMA

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Dmitry Gutov]

On 08/20/2016 02:45 AM, Yoichi Nakayama wrote:
>> Why don't you bind json-null to whatever value you need?
>
> (let ((json-null 'NULL)) (json-encode (json-read-from-string "{}")))
> "\"nil\""

Okay then:

ELISP> (let ((json-object-type 'hash-table)) (json-encode 
(json-read-from-string "{}")))
"{}"

>> By the way, another option to distinguish nil and {} is to bind json-object-type to `hash-table'. An empty hash table is not nil.
>
> It does not work because

Where/how doesn't it work?

 > json-pretty-print overwrites json-object-type.

Maybe the fix could be in json-pretty-print.

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

> Maybe the fix could be in json-pretty-print.

I agree that json-pretty-print should be responsible to the issue.
But I think

(let ((json-null 'NULL)) (json-encode (json-read-from-string "{}")))

is also a bug.
How about the following patch?

From 24a11fc81ea283c7f999bbcf87ea0f2c01c1c24e Mon Sep 17 00:00:00 2001
From: Yoichi Nakayama <yoichi.nakayama@gmail.com>
Date: Sat, 20 Aug 2016 15:00:28 +0900
Subject: [PATCH] Distinguish empty json object and null

* lisp/json.el (json-encode-list, json-encode): Handle empty object
correctly when json-null is not nil.
(json-pretty-print): Bind json-null to distinguish empty object and null.
---
 lisp/json.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/json.el b/lisp/json.el
index fdac8d9..a439f77 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -588,7 +588,7 @@ Please see the documentation of `json-object-type'
and `json-key-type'."
   "Return a JSON representation of LIST.
 Tries to DWIM: simple lists become JSON arrays, while alists and plists
 become JSON objects."
-  (cond ((null list)         "null")
+  (cond ((eq json-null list) "null")
         ((json-alist-p list) (json-encode-alist list))
         ((json-plist-p list) (json-encode-plist list))
         ((listp list)        (json-encode-array list))
@@ -700,12 +700,12 @@ Advances point just past JSON object."
         ((stringp object)      (json-encode-string object))
         ((keywordp object)     (json-encode-string
                                 (substring (symbol-name object) 1)))
-        ((symbolp object)      (json-encode-string
-                                (symbol-name object)))
         ((numberp object)      (json-encode-number object))
         ((arrayp object)       (json-encode-array object))
         ((hash-table-p object) (json-encode-hash-table object))
         ((listp object)        (json-encode-list object))
+        ((symbolp object)      (json-encode-string
+                                (symbol-name object)))
         (t                     (signal 'json-error (list object)))))

 ;; Pretty printing
@@ -722,6 +722,8 @@ Advances point just past JSON object."
     (let ((json-encoding-pretty-print t)
           ;; Ensure that ordering is maintained
           (json-object-type 'alist)
+          ;; Distinguish empty object and null
+          (json-null :json-null)
           (txt (delete-and-extract-region begin end)))
       (insert (json-encode (json-read-from-string txt))))))

-- 
2.8.1

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

The second patch dosn't work with some cases. Applying json-pretty-print to
{":json-null": 1}
cause error "Bad JSON object key: :json-null".


On Sat, Aug 20, 2016 at 3:12 PM, Yoichi Nakayama
<yoichi.nakayama@gmail.com> wrote:
>> Maybe the fix could be in json-pretty-print.
>
> I agree that json-pretty-print should be responsible to the issue.
> But I think
>
> (let ((json-null 'NULL)) (json-encode (json-read-from-string "{}")))
>
> is also a bug.
> How about the following patch?
>
> From 24a11fc81ea283c7f999bbcf87ea0f2c01c1c24e Mon Sep 17 00:00:00 2001
> From: Yoichi Nakayama <yoichi.nakayama@gmail.com>
> Date: Sat, 20 Aug 2016 15:00:28 +0900
> Subject: [PATCH] Distinguish empty json object and null
>
> * lisp/json.el (json-encode-list, json-encode): Handle empty object
> correctly when json-null is not nil.
> (json-pretty-print): Bind json-null to distinguish empty object and null.
> ---
>  lisp/json.el | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/json.el b/lisp/json.el
> index fdac8d9..a439f77 100644
> --- a/lisp/json.el
> +++ b/lisp/json.el
> @@ -588,7 +588,7 @@ Please see the documentation of `json-object-type'
> and `json-key-type'."
>    "Return a JSON representation of LIST.
>  Tries to DWIM: simple lists become JSON arrays, while alists and plists
>  become JSON objects."
> -  (cond ((null list)         "null")
> +  (cond ((eq json-null list) "null")
>          ((json-alist-p list) (json-encode-alist list))
>          ((json-plist-p list) (json-encode-plist list))
>          ((listp list)        (json-encode-array list))
> @@ -700,12 +700,12 @@ Advances point just past JSON object."
>          ((stringp object)      (json-encode-string object))
>          ((keywordp object)     (json-encode-string
>                                  (substring (symbol-name object) 1)))
> -        ((symbolp object)      (json-encode-string
> -                                (symbol-name object)))
>          ((numberp object)      (json-encode-number object))
>          ((arrayp object)       (json-encode-array object))
>          ((hash-table-p object) (json-encode-hash-table object))
>          ((listp object)        (json-encode-list object))
> +        ((symbolp object)      (json-encode-string
> +                                (symbol-name object)))
>          (t                     (signal 'json-error (list object)))))
>
>  ;; Pretty printing
> @@ -722,6 +722,8 @@ Advances point just past JSON object."
>      (let ((json-encoding-pretty-print t)
>            ;; Ensure that ordering is maintained
>            (json-object-type 'alist)
> +          ;; Distinguish empty object and null
> +          (json-null :json-null)
>            (txt (delete-and-extract-region begin end)))
>        (insert (json-encode (json-read-from-string txt))))))
>
> --
> 2.8.1



-- 
Yoichi NAKAYAMA

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Dmitry Gutov]

On 08/20/2016 09:12 AM, Yoichi Nakayama wrote:

> I agree that json-pretty-print should be responsible to the issue.
> But I think
>
> (let ((json-null 'NULL)) (json-encode (json-read-from-string "{}")))
>
> is also a bug.
> How about the following patch?

Something like this looks like a good idea to me. Especially if you 
figure out what's the problem you have "with some cases".

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

There was another potential bug that json-encode-key wrongly interpret
symbols internally used in json.el.
The problem is not caused by my previous patch, but the patch
introduces another internal
symbol :json-null and increase possibility to encounter it.

Following patch will fix it and make it safe to apply my second patch

From 4d3ad1d3bbe6b3e47743f42427ac26cf316c12b5 Mon Sep 17 00:00:00 2001
From: Yoichi Nakayama <yoichi.nakayama@gmail.com>
Date: Sun, 21 Aug 2016 20:24:03 +0900
Subject: [PATCH] Make json-encode-key not to signal with valid keys

Despite strings like ":json-false", "t", "nil" are valid object key,
their elisp representation were confused with internal symbols in
json-encode-key and cause json-key-format error unexpectedly.

* (json-encode-key): Rewrite without using json-encode.
---
 lisp/json.el | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/lisp/json.el b/lisp/json.el
index a439f77..a387b08 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -425,14 +425,30 @@ representation will be parsed correctly."
     (push "\"" res)
     (apply #'concat "\"" (nreverse res))))

-(defun json-encode-key (object)
-  "Return a JSON representation of OBJECT.
-If the resulting JSON object isn't a valid JSON object key,
-this signals `json-key-format'."
-  (let ((encoded (json-encode object)))
-    (unless (stringp (json-read-from-string encoded))
-      (signal 'json-key-format (list object)))
-    encoded))
+(defun json-encode-key (key)
+  "Return a JSON representation of object key.
+If key isn't valid Elisp object as key, this signals `json-key-format'."
+  (let ((json-key-type
+         (if (eq json-key-type nil)
+             (cdr (assq json-object-type '((hash-table . string)
+                                           (alist . symbol)
+                                           (plist . keyword))))
+           json-key-type)))
+    (json-encode-string
+     (cond ((eq json-key-type 'string)
+            (if (stringp key)
+                key
+              (signal 'json-key-format (list key))))
+           ((eq json-key-type 'symbol)
+            (if (symbolp key)
+                (symbol-name key)
+              (signal 'json-key-format (list key))))
+           ((eq json-key-type 'keyword)
+            (if (keywordp key)
+                (substring (symbol-name key) 1)
+              (signal 'json-key-format (list key))))
+           (t
+            (signal 'json-error (list key)))))))

 ;;; JSON Objects

-- 
2.8.1

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

The change in json-encode-key broke tests in json-tests.el. I'll investigate it.

Ran 34 tests, 28 results as expected, 6 unexpected (2016-08-21 22:30:23+0900)

6 unexpected results:
   FAILED  test-json-encode-alist-with-sort-predicate
   FAILED  test-json-encode-hash-table
   FAILED  test-json-encode-key
   FAILED  test-json-encode-list
   FAILED  test-json-encode-plist
   FAILED  test-json-encode-plist-with-sort-predicate

-- 
Yoichi NAKAYAMA

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Yoichi Nakayama]

[-- Attachment #1: Type: text/plain, Size: 11545 bytes --]

Fixed tests and update object encoding functions to be functional without
binding json-object-type as far as possible.
;; You can find a series of patches at
https://github.com/yoichi/emacs/tree/fix/json-empty-object2

From d8549a88633704fc76eb6cdafa6b6ea591fb14e3 Mon Sep 17 00:00:00 2001
From: Yoichi Nakayama <yoichi.nakayama@gmail.com>
Date: Sun, 21 Aug 2016 23:54:43 +0900
Subject: [PATCH] Bind json-object-type on object encoding functions

If elisp object are plist or hash-table, assume they are created with
associated json-object-type.

* json.el (json--plist-to-alist): Convert key format.
(json-encode-hash-table, json-encode-alist, json-encode-plist): Bind
json-object-type.
* json-tests.el (test-json-plist-to-alist): Use default key type for
expected value, and add test with json-key-type.
(test-json-encode-key): Add test not to confuse internal symbols.
(test-json-encode-hash-table, test-json-encode-alist-with-sort-predicate,
test-json-encode-list): Use default key type.
---
 lisp/json.el            | 134
++++++++++++++++++++++++++----------------------
 test/lisp/json-tests.el |  29 +++++++----
 2 files changed, 91 insertions(+), 72 deletions(-)

diff --git a/lisp/json.el b/lisp/json.el
index a387b08..24fefc5 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -174,6 +174,10 @@ Unlike `reverse', this keeps the property-value pairs
intact."
     (while plist
       (let ((prop (pop plist))
             (val (pop plist)))
+        (when (and (not json-key-type)
+                   (keywordp prop))
+          (setq prop (intern
+                      (substring (symbol-name prop) 1))))
         (push (cons prop val) res)))
     (nreverse res)))

@@ -526,79 +530,85 @@ Please see the documentation of `json-object-type'
and `json-key-type'."

 (defun json-encode-hash-table (hash-table)
   "Return a JSON representation of HASH-TABLE."
-  (if json-encoding-object-sort-predicate
-      (json-encode-alist (map-into hash-table 'list))
+  (let ((json-object-type 'hash-table))
+    (if json-encoding-object-sort-predicate
+        (let ((json-key-type
+               (or json-key-type
+                   'string)))
+          (json-encode-alist (map-into hash-table 'list)))
+      (format "{%s%s}"
+              (json-join
+               (let (r)
+                 (json--with-indentation
+                  (maphash
+                   (lambda (k v)
+                     (push (format
+                            (if json-encoding-pretty-print
+                                "%s%s: %s"
+                              "%s%s:%s")
+                            json--encoding-current-indentation
+                            (json-encode-key k)
+                            (json-encode v))
+                           r))
+                   hash-table))
+                 r)
+               json-encoding-separator)
+              (if (or (not json-encoding-pretty-print)
+                      json-encoding-lisp-style-closings)
+                  ""
+                json--encoding-current-indentation)))))
+
+;; List encoding (including alists and plists)
+
+(defun json-encode-alist (alist)
+  "Return a JSON representation of ALIST."
+  (let ((json-object-type 'alist))
+    (when json-encoding-object-sort-predicate
+      (setq alist
+            (sort alist (lambda (a b)
+                          (funcall json-encoding-object-sort-predicate
+                                   (car a) (car b))))))
     (format "{%s%s}"
             (json-join
-             (let (r)
-               (json--with-indentation
-                (maphash
-                 (lambda (k v)
-                   (push (format
-                          (if json-encoding-pretty-print
-                              "%s%s: %s"
-                            "%s%s:%s")
-                          json--encoding-current-indentation
-                          (json-encode-key k)
-                          (json-encode v))
-                         r))
-                 hash-table))
-               r)
+             (json--with-indentation
+              (mapcar (lambda (cons)
+                        (format (if json-encoding-pretty-print
+                                    "%s%s: %s"
+                                  "%s%s:%s")
+                                json--encoding-current-indentation
+                                (json-encode-key (car cons))
+                                (json-encode (cdr cons))))
+                      alist))
              json-encoding-separator)
             (if (or (not json-encoding-pretty-print)
                     json-encoding-lisp-style-closings)
                 ""
               json--encoding-current-indentation))))

-;; List encoding (including alists and plists)
-
-(defun json-encode-alist (alist)
-  "Return a JSON representation of ALIST."
-  (when json-encoding-object-sort-predicate
-    (setq alist
-          (sort alist (lambda (a b)
-                        (funcall json-encoding-object-sort-predicate
-                                 (car a) (car b))))))
-  (format "{%s%s}"
-          (json-join
-           (json--with-indentation
-            (mapcar (lambda (cons)
-                      (format (if json-encoding-pretty-print
-                                  "%s%s: %s"
-                                "%s%s:%s")
-                              json--encoding-current-indentation
-                              (json-encode-key (car cons))
-                              (json-encode (cdr cons))))
-                    alist))
-           json-encoding-separator)
-          (if (or (not json-encoding-pretty-print)
-                  json-encoding-lisp-style-closings)
-              ""
-            json--encoding-current-indentation)))
-
 (defun json-encode-plist (plist)
   "Return a JSON representation of PLIST."
-  (if json-encoding-object-sort-predicate
-      (json-encode-alist (json--plist-to-alist plist))
-    (let (result)
-      (json--with-indentation
-       (while plist
-         (push (concat
-                json--encoding-current-indentation
-                (json-encode-key (car plist))
-                (if json-encoding-pretty-print
-                    ": "
-                  ":")
-                (json-encode (cadr plist)))
-               result)
-         (setq plist (cddr plist))))
-      (concat "{"
-              (json-join (nreverse result) json-encoding-separator)
-              (if (and json-encoding-pretty-print
-                       (not json-encoding-lisp-style-closings))
+  (let ((json-object-type 'plist))
+    (if json-encoding-object-sort-predicate
+        (json-encode-alist (json--plist-to-alist plist))
+      (let (result)
+        (json--with-indentation
+         (while plist
+           (push (concat
                   json--encoding-current-indentation
-                "")
-              "}"))))
+                  (json-encode-key (car plist))
+                  (if json-encoding-pretty-print
+                      ": "
+                    ":")
+                  (json-encode (cadr plist)))
+                 result)
+           (setq plist (cddr plist))))
+        (concat "{"
+                (json-join (nreverse result) json-encoding-separator)
+                (if (and json-encoding-pretty-print
+                         (not json-encoding-lisp-style-closings))
+                    json--encoding-current-indentation
+                  "")
+                "}")))))

 (defun json-encode-list (list)
   "Return a JSON representation of LIST.
diff --git a/test/lisp/json-tests.el b/test/lisp/json-tests.el
index 78cebb4..d5abb58 100644
--- a/test/lisp/json-tests.el
+++ b/test/lisp/json-tests.el
@@ -62,9 +62,11 @@ Point is moved to beginning of the buffer."

 (ert-deftest test-json-plist-to-alist ()
   (should (equal (json--plist-to-alist '()) '()))
-  (should (equal (json--plist-to-alist '(:a 1)) '((:a . 1))))
+  (should (equal (json--plist-to-alist '(:a 1)) '((a . 1))))
+  (let ((json-key-type 'keyword))
+    (should (equal (json--plist-to-alist '(:a 1)) '((:a . 1)))))
   (should (equal (json--plist-to-alist '(:a 1 :b 2 :c 3))
-                 '((:a . 1) (:b . 2) (:c . 3)))))
+                 '((a . 1) (b . 2) (c . 3)))))

 (ert-deftest test-json-advance ()
   (json-tests--with-temp-buffer "{ \"a\": 1 }"
@@ -177,9 +179,16 @@ Point is moved to beginning of the buffer."
                  "\"\\nasdфыв\\u001f\u007ffgh\\t\"")))

 (ert-deftest test-json-encode-key ()
-  (should (equal (json-encode-key "foo") "\"foo\""))
-  (should (equal (json-encode-key 'foo) "\"foo\""))
-  (should (equal (json-encode-key :foo) "\"foo\""))
+  (let ((json-key-type 'string))
+    (should (equal (json-encode-key "foo") "\"foo\""))
+    (should-error (json-encode-key t) :type 'json-key-format))
+  (let ((json-key-type 'symbol))
+    (should (equal (json-encode-key 'foo) "\"foo\""))
+    (should (equal (json-encode-key t) "\"t\""))
+    (should (equal (json-encode-key :t) "\":t\"")))
+  (let ((json-key-type 'keyword))
+    (should (equal (json-encode-key :foo) "\"foo\""))
+    (should-error (json-encode-key t) :type 'json-key-format))
   (should-error (json-encode-key 5) :type 'json-key-format)
   (should-error (json-encode-key ["foo"]) :type 'json-key-format)
   (should-error (json-encode-key '("foo")) :type 'json-key-format))
@@ -238,9 +247,9 @@ Point is moved to beginning of the buffer."
   (let ((hash-table (make-hash-table))
         (json-encoding-object-sort-predicate 'string<)
         (json-encoding-pretty-print nil))
-    (puthash :a 1 hash-table)
-    (puthash :b 2 hash-table)
-    (puthash :c 3 hash-table)
+    (puthash "a" 1 hash-table)
+    (puthash "b" 2 hash-table)
+    (puthash "c" 3 hash-table)
     (should (equal (json-encode hash-table)
                    "{\"a\":1,\"b\":2,\"c\":3}"))))

@@ -261,7 +270,7 @@ Point is moved to beginning of the buffer."
     (should (equal (json-encode plist) "{\"a\":1,\"b\":2,\"c\":3}"))))

 (ert-deftest test-json-encode-alist-with-sort-predicate ()
-  (let ((alist '((:c . 3) (:a . 1) (:b . 2)))
+  (let ((alist '((c . 3) (a . 1) (b . 2)))
         (json-encoding-object-sort-predicate 'string<)
         (json-encoding-pretty-print nil))
     (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))))
@@ -270,7 +279,7 @@ Point is moved to beginning of the buffer."
   (let ((json-encoding-pretty-print nil))
     (should (equal (json-encode-list '(:a 1 :b 2))
                    "{\"a\":1,\"b\":2}"))
-    (should (equal (json-encode-list '((:a . 1) (:b . 2)))
+    (should (equal (json-encode-list '((a . 1) (b . 2)))
                    "{\"a\":1,\"b\":2}"))
     (should (equal (json-encode-list '(1 2 3 4)) "[1,2,3,4]"))))

-- 
2.8.1



2016年8月21日(日) 22:32 Yoichi Nakayama <yoichi.nakayama@gmail.com>:

> The change in json-encode-key broke tests in json-tests.el. I'll
> investigate it.
>
> Ran 34 tests, 28 results as expected, 6 unexpected (2016-08-21
> 22:30:23+0900)
>
> 6 unexpected results:
>    FAILED  test-json-encode-alist-with-sort-predicate
>    FAILED  test-json-encode-hash-table
>    FAILED  test-json-encode-key
>    FAILED  test-json-encode-list
>    FAILED  test-json-encode-plist
>    FAILED  test-json-encode-plist-with-sort-predicate
>
> --
> Yoichi NAKAYAMA
>

[-- Attachment #2: Type: text/html, Size: 16599 bytes --]

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Dmitry Gutov]

Hi Yoichi,

Thanks, but the changes to tests like below seem to indicate that the 
code breaks backward compatibility in certain respect. I'm not sure 
we'll want to merge it for that reason.

I'd like to know what other developers think.

On 08/21/2016 06:06 PM, Yoichi Nakayama wrote:

>  (ert-deftest test-json-plist-to-alist ()
>    (should (equal (json--plist-to-alist '()) '()))
> -  (should (equal (json--plist-to-alist '(:a 1)) '((:a . 1))))
> +  (should (equal (json--plist-to-alist '(:a 1)) '((a . 1))))
> +  (let ((json-key-type 'keyword))
> +    (should (equal (json--plist-to-alist '(:a 1)) '((:a . 1)))))
>    (should (equal (json--plist-to-alist '(:a 1 :b 2 :c 3))
> -                 '((:a . 1) (:b . 2) (:c . 3)))))
> +                 '((a . 1) (b . 2) (c . 3)))))
>
>  (ert-deftest test-json-advance ()
>    (json-tests--with-temp-buffer "{ \"a\": 1 }"
> @@ -177,9 +179,16 @@ Point is moved to beginning of the buffer."
>                   "\"\\nasdфыв\\u001f\u007ffgh\\t\"")))
>
>  (ert-deftest test-json-encode-key ()
> -  (should (equal (json-encode-key "foo") "\"foo\""))
> -  (should (equal (json-encode-key 'foo) "\"foo\""))
> -  (should (equal (json-encode-key :foo) "\"foo\""))
> +  (let ((json-key-type 'string))
> +    (should (equal (json-encode-key "foo") "\"foo\""))
> +    (should-error (json-encode-key t) :type 'json-key-format))
> +  (let ((json-key-type 'symbol))
> +    (should (equal (json-encode-key 'foo) "\"foo\""))
> +    (should (equal (json-encode-key t) "\"t\""))
> +    (should (equal (json-encode-key :t) "\":t\"")))
> +  (let ((json-key-type 'keyword))
> +    (should (equal (json-encode-key :foo) "\"foo\""))
> +    (should-error (json-encode-key t) :type 'json-key-format))
>    (should-error (json-encode-key 5) :type 'json-key-format)
>    (should-error (json-encode-key ["foo"]) :type 'json-key-format)
>    (should-error (json-encode-key '("foo")) :type 'json-key-format))
> @@ -238,9 +247,9 @@ Point is moved to beginning of the buffer."
>    (let ((hash-table (make-hash-table))
>          (json-encoding-object-sort-predicate 'string<)
>          (json-encoding-pretty-print nil))
> -    (puthash :a 1 hash-table)
> -    (puthash :b 2 hash-table)
> -    (puthash :c 3 hash-table)
> +    (puthash "a" 1 hash-table)
> +    (puthash "b" 2 hash-table)
> +    (puthash "c" 3 hash-table)
>      (should (equal (json-encode hash-table)
>                     "{\"a\":1,\"b\":2,\"c\":3}"))))
>
> @@ -261,7 +270,7 @@ Point is moved to beginning of the buffer."
>      (should (equal (json-encode plist) "{\"a\":1,\"b\":2,\"c\":3}"))))
>
>  (ert-deftest test-json-encode-alist-with-sort-predicate ()
> -  (let ((alist '((:c . 3) (:a . 1) (:b . 2)))
> +  (let ((alist '((c . 3) (a . 1) (b . 2)))
>          (json-encoding-object-sort-predicate 'string<)
>          (json-encoding-pretty-print nil))
>      (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))))
> @@ -270,7 +279,7 @@ Point is moved to beginning of the buffer."
>    (let ((json-encoding-pretty-print nil))
>      (should (equal (json-encode-list '(:a 1 :b 2))
>                     "{\"a\":1,\"b\":2}"))
> -    (should (equal (json-encode-list '((:a . 1) (:b . 2)))
> +    (should (equal (json-encode-list '((a . 1) (b . 2)))
>                     "{\"a\":1,\"b\":2}"))
>      (should (equal (json-encode-list '(1 2 3 4)) "[1,2,3,4]"))))

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Damien Cassou]

Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
> Fixed tests and update object encoding functions to be functional without
> binding json-object-type as far as possible.

this bug is annoying me as well while implementing a CouchDB client.

-- 
Damien Cassou
Företagsplatsen AB
Phone/Fax: +46 (0)8 774 63 00
Mobile: +33 (0)6 80 50 18 91
Address: Skeppsbron 26, 4tr, SE-111 30 Stockholm
Web: www.foretagsplatsen.se

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Damien Cassou]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
> When json-pretty-print applied to "{}", it is
> unexpectedly converted to "null".
> This is caused by internal representations of null
> and empty object are the same:
>   (json-read-from-string "{}") ; => nil
>   (json-read-from-string "null") ; => nil

please find a patch attached. This patch uses :json-null to distinguish,
during pretty-print, between the value null and an empty object.

All tests pass and many new tests are added.

Best

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-pretty-printing-empty-objects-as-null.patch --]
[-- Type: text/x-patch, Size: 4751 bytes --]

From 78eb7b659809444f7c9690db73e1f65843d5658c Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Sat, 19 May 2018 08:36:32 +0200
Subject: [PATCH] Fix pretty-printing empty objects as null

* lisp/json.el (json-pretty-print): Force distinction between empty
  objects and null.
(json-encode-list): Remove responsibility to print "null" as this
value is not a list.
(json-encode): Give higher precedence to lists so that an empty list
is printed as an empty object, not as "null".

* test/lisp/json-tests.el (test-json-encode): Add many tests to check
  the behavior of pretty-printing.
---
 lisp/json.el            |  7 +++--
 test/lisp/json-tests.el | 67 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/lisp/json.el b/lisp/json.el
index d374f452e6..cd95ec2832 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -609,8 +609,7 @@ json-encode-list
   "Return a JSON representation of LIST.
 Tries to DWIM: simple lists become JSON arrays, while alists and plists
 become JSON objects."
-  (cond ((null list)         "null")
-        ((json-alist-p list) (json-encode-alist list))
+  (cond ((json-alist-p list) (json-encode-alist list))
         ((json-plist-p list) (json-encode-plist list))
         ((listp list)        (json-encode-array list))
         (t
@@ -723,12 +722,12 @@ json-encode
         ((stringp object)      (json-encode-string object))
         ((keywordp object)     (json-encode-string
                                 (substring (symbol-name object) 1)))
+        ((listp object)        (json-encode-list object))
         ((symbolp object)      (json-encode-string
                                 (symbol-name object)))
         ((numberp object)      (json-encode-number object))
         ((arrayp object)       (json-encode-array object))
         ((hash-table-p object) (json-encode-hash-table object))
-        ((listp object)        (json-encode-list object))
         (t                     (signal 'json-error (list object)))))
 
 ;; Pretty printing
@@ -743,6 +742,8 @@ json-pretty-print
   (interactive "r")
   (atomic-change-group
     (let ((json-encoding-pretty-print t)
+          ;; Distinguish an empty objects from 'null'
+          (json-null :json-null)
           ;; Ensure that ordering is maintained
           (json-object-type 'alist)
           (txt (delete-and-extract-region begin end)))
diff --git a/test/lisp/json-tests.el b/test/lisp/json-tests.el
index ea562e8b13..84039c09ce 100644
--- a/test/lisp/json-tests.el
+++ b/test/lisp/json-tests.el
@@ -325,5 +325,72 @@ json-tests--with-temp-buffer
   (with-temp-buffer
     (should-error (json-encode (current-buffer)) :type 'json-error)))
 
+;;; Pretty-print
+
+(defun json-tests-equal-pretty-print (original &optional expected)
+  "Abort current test if pretty-printing ORIGINAL does not yield EXPECTED.
+
+Both ORIGINAL and EXPECTED should be strings.  If EXPECTED is
+nil, ORIGINAL should stay unchanged by pretty-printing."
+  (with-temp-buffer
+    (insert original)
+    (json-pretty-print-buffer)
+    (should (equal (buffer-string) (or expected original)))))
+
+(ert-deftest test-json-pretty-print-string ()
+  (json-tests-equal-pretty-print "\"\"")
+  (json-tests-equal-pretty-print "\"foo\""))
+
+(ert-deftest test-json-pretty-print-atom ()
+  (json-tests-equal-pretty-print "true")
+  (json-tests-equal-pretty-print "false")
+  (json-tests-equal-pretty-print "null"))
+
+(ert-deftest test-json-pretty-print-number ()
+  (json-tests-equal-pretty-print "123")
+  (json-tests-equal-pretty-print "0.123"))
+
+(ert-deftest test-json-pretty-print-object ()
+  ;; empty (regression test for bug#24252)
+  (json-tests-equal-pretty-print
+   "{}"
+   "{\n}")
+  ;; one pair
+  (json-tests-equal-pretty-print
+   "{\"key\":1}"
+   "{\n  \"key\": 1\n}")
+  ;; two pairs
+  (json-tests-equal-pretty-print
+   "{\"key1\":1,\"key2\":2}"
+   "{\n  \"key1\": 1,\n  \"key2\": 2\n}")
+  ;; embedded object
+  (json-tests-equal-pretty-print
+   "{\"foo\":{\"key\":1}}"
+   "{\n  \"foo\": {\n    \"key\": 1\n  }\n}")
+  ;; embedded array
+  (json-tests-equal-pretty-print
+   "{\"key\":[1,2]}"
+   "{\n  \"key\": [\n    1,\n    2\n  ]\n}"))
+
+(ert-deftest test-json-pretty-print-array ()
+  ;; empty
+  (json-tests-equal-pretty-print "[]")
+  ;; one item
+  (json-tests-equal-pretty-print
+   "[1]"
+   "[\n  1\n]")
+  ;; two items
+  (json-tests-equal-pretty-print
+   "[1,2]"
+   "[\n  1,\n  2\n]")
+  ;; embedded object
+  (json-tests-equal-pretty-print
+   "[{\"key\":1}]"
+   "[\n  {\n    \"key\": 1\n  }\n]")
+  ;; embedded array
+  (json-tests-equal-pretty-print
+   "[[1,2]]"
+   "[\n  [\n    1,\n    2\n  ]\n]"))
+
 (provide 'json-tests)
 ;;; json-tests.el ends here
-- 
2.17.0


[-- Attachment #3: Type: text/plain, Size: 164 bytes --]


-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

Damien Cassou <damien@cassou.me> writes:

> please find a patch attached. This patch uses :json-null to distinguish,
> during pretty-print, between the value null and an empty object.

It looks good to me, but I'm wondering if encoding/decoding (not
pretty-printing) will be backward-compatible or not.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Damien Cassou]

Nicolas Petton <nicolas@petton.fr> writes:
> Damien Cassou <damien@cassou.me> writes:
>> please find a patch attached. This patch uses :json-null to distinguish,
>> during pretty-print, between the value null and an empty object.
>
> It looks good to me, but I'm wondering if encoding/decoding (not
> pretty-printing) will be backward-compatible or not.

no one who has worked on this module before is willing to review this
small change?

-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Eli Zaretskii]

> From: Damien Cassou <damien@cassou.me>
> Date: Mon, 11 Jun 2018 15:36:35 +0200
> Cc: Philipp Stephani <phst@google.com>, Mark Oteiza <mvoteiza@udel.edu>,
> 	Theresa O'Connor <ted@oconnor.cx>, 24252@debbugs.gnu.org,
> 	Dmitry Gutov <dgutov@yandex.ru>
> 
> Nicolas Petton <nicolas@petton.fr> writes:
> > Damien Cassou <damien@cassou.me> writes:
> >> please find a patch attached. This patch uses :json-null to distinguish,
> >> during pretty-print, between the value null and an empty object.
> >
> > It looks good to me, but I'm wondering if encoding/decoding (not
> > pretty-printing) will be backward-compatible or not.
> 
> no one who has worked on this module before is willing to review this
> small change?

Sorry about that.  From my POV, Nico asked a question that I didn't
see answered, so the patch is still "being discussed", as far as I'm
concerned.  If you can address Nicolas's concern, we should be able to
move ahead.

Thanks.

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Damien Cassou]

Eli Zaretskii <eliz@gnu.org> writes:
> Nicolas Petton <nicolas@petton.fr> writes:
>> It looks good to me, but I'm wondering if encoding/decoding (not
>> pretty-printing) will be backward-compatible or not.

> From my POV, Nico asked a question that I didn't see answered, so the
> patch is still "being discussed", as far as I'm concerned.  If you can
> address Nicolas's concern, we should be able to move ahead.

I understood Nico's message as a call for help from other Emacs
maintainers, not from me :-).

The new code is certainly *not* backward-compatible if you look at the
details. For example, `json-encode-list` was sometimes printing `null`
and it won't do that anymore. I see that as a bug fix because `null` is
not a list but it clearly makes the code not backward compatible.

On the other hand, all existing unit tests still pass and the patch
includes a bunch of new ones. I have been using this patch since I wrote
it with no problem at all.

My opinion is that a new Emacs release based on master is far enough in
the future to merge this patch now and see if any complaints come
:-). At worst, we will find a bug and Emacs will gain a new unit test.

-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Damien Cassou <damien@cassou.me> writes:

> I understood Nico's message as a call for help from other Emacs
> maintainers, not from me :-).

It was just a question :-)

> The new code is certainly *not* backward-compatible if you look at the
> details. For example, `json-encode-list` was sometimes printing `null`
> and it won't do that anymore. I see that as a bug fix because `null` is
> not a list but it clearly makes the code not backward compatible.
>
> On the other hand, all existing unit tests still pass and the patch
> includes a bunch of new ones. I have been using this patch since I wrote
> it with no problem at all.

Seems fair to me.  I think we should install the patch.  Eli, do you
agree?  If so, I'll install it in master.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Eli Zaretskii]

> From: Nicolas Petton <nicolas@petton.fr>
> Cc: yoichi.nakayama@gmail.com, phst@google.com, mvoteiza@udel.edu, ted@oconnor.cx, 24252@debbugs.gnu.org, dgutov@yandex.ru
> Date: Wed, 13 Jun 2018 15:05:47 +0200
> 
> Seems fair to me.  I think we should install the patch.  Eli, do you
> agree?  If so, I'll install it in master.

It's okay for master, thanks.

bug#24252: 25.1; json.el doesn't distinguish null and empty object

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]


tags 24252 fixed
close 24252 27.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

> It's okay for master, thanks.

I installed it in master, so I'm closing this ticket.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]