Merge lp:~paul-lucas/zorba/bug-1025622 into lp:zorba

[Paul J. Lucas]

Now doing proper JSON serialization.

Now doing proper JSON serialization.

[Paul J. Lucas (paul-lucas)]

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug-1025622-2012-07-18T23-51-03.584Z/log.html

[Chris Hillery (ceejatec)]

Cool, thanks. Please add a couple test cases including the problem query described in the bug report; also, add a note to the Changelog mentioning the bug fix.

review: Needs Fixing

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug-1025622-2012-07-18T23-51-03.584Z is finished. The final status was:

All tests succeeded!

[Zorba Build Bot (zorba-buildbot)]

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1, Needs Fixing < 1, Pending < 1. Got: 1 Approve, 1 Needs Fixing, 1 Pending.

[Paul J. Lucas (paul-lucas)]

I added a mention in the change log and also the one test (can't think of others).

[Chris Hillery (ceejatec)]

s/supplementory/supplementary/

[Dennis Knochenwefel (dennis-knochenwefel)]

http://json.org/ says that not only '"' and '\' need to be escaped with a backslash but also a solidus '/'.

And, some control characters have special backspace escapes: backspace (x8 -> \b), formfeed (xC -> \f), newline (xA -> \n), carriage return (xD -> \r) and HTab (x9 -> \t). Currently, they are serialized as hex.

I would propose to add tests for those as well.

review: Needs Fixing

[lp:~paul-lucas/zorba/bug-1025622]

10952. By Paul J. Lucas on 2012-07-19

Fixed typo.

10953. By Paul J. Lucas on 2012-07-19

Now emitting \b, \f, \n, etc.

[Paul J. Lucas (paul-lucas)]

json.org says that '/' *MAY* be escaped, but it's not *REQUIRED* for it to be so.

As for the rest, I've added code to emit them. (The original code didn't emit them either.) Note that it wasn't wrong, just less pretty.

I added a test (within the limits of what Zorba accepts in characters).

[Dennis Knochenwefel (dennis-knochenwefel)]

cannot test it on windows as it doesn't currently build on windows (this doesn't have anything to do with this branch).

on linux:

The following tests FAILED:
        1104 - test/rbkt/zorba/jsoniq/coll_dyn_03 (Failed)

review: Needs Fixing

[Paul J. Lucas (paul-lucas)]

I think that query is wrong since there are newline literals in strings rather than \n.

[lp:~paul-lucas/zorba/bug-1025622]

10954. By Paul J. Lucas on 2012-07-19

Fixed expected result.

[Paul J. Lucas (paul-lucas)]

Actually, the query may be OK, but the expected result is wrong. I've fixed it.

[Dennis Knochenwefel (dennis-knochenwefel)]

review: Approve

[Chris Hillery (ceejatec)]

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug-1025622-2012-07-19T18-07-03.448Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug-1025622-2012-07-19T18-07-03.448Z is finished. The final status was:

All tests succeeded!

=== modified file 'ChangeLog'
--- ChangeLog	2012-07-17 15:49:17 +0000
+++ ChangeLog	2012-07-19 17:41:22 +0000
@@ -32,6 +32,7 @@
   * Streaming execution for tumbling windows (also fixes bug #1010051).
 
 Bug Fixes/Other Changes:
+  * Fixed bug #1025622 (Incorrect JSON serialization of supplementary plane code points)
   * Fixed bug #1016606 (DOCTYPE in the input of the parse-fragment function)
   * Fixed bug #1002993 (bug during revalidation after update; improper condition
     for calling TypeOps::get_atomic_type_code() from

=== modified file 'src/api/serialization/serializer.cpp'
--- src/api/serialization/serializer.cpp	2012-07-13 06:50:40 +0000
+++ src/api/serialization/serializer.cpp	2012-07-19 17:41:22 +0000
@@ -31,8 +31,10 @@
 #include "api/unmarshaller.h"
 
 #include "util/ascii_util.h"
+#include "util/string_util.h"
+#include "util/unicode_util.h"
+#include "util/utf8_string.h"
 #include "util/utf8_util.h"
-#include "util/string_util.h"
 #include "util/xml_util.h"
 
 #include "system/globalenv.h"
@@ -1203,29 +1205,47 @@
 /*******************************************************************************
 
 ********************************************************************************/
-void serializer::json_emitter::emit_json_string(zstring string)
+void serializer::json_emitter::emit_json_string(zstring const &string)
 {
   tr << '"';
-  zstring::const_iterator i = string.begin();
-  zstring::const_iterator end = string.end();
-  for (; i < end; i++) 
-  {
-    if (*i < 0x20) 
-    {
-      // Escape control sequences
-      std::stringstream hex;
-      hex << "\\u" << std::setw(4) << std::setfill('0')
-          << std::hex << static_cast<int>(*i);
-      tr << hex.str();
-      continue;
-    }
-    if (*i == '\\' || *i == '"') 
-    {
-      // Output escape char for \ or "
-      tr << '\\';
-      // Fall through to output original character
-    }
-    tr << *i;
+  utf8_string<zstring const> const u( string );
+  FOR_EACH( utf8_string<zstring const>, i, u ) {
+    unicode::code_point const cp = *i;
+    if ( ascii::is_cntrl( cp ) ) {
+      switch ( cp ) {
+        case '\b': tr << "\\b"; break;
+        case '\f': tr << "\\f"; break;
+        case '\n': tr << "\\n"; break;
+        case '\r': tr << "\\r"; break;
+        case '\t': tr << "\\t"; break;
+        default: {
+          std::ostringstream oss;
+          oss << std::hex << std::setfill('0') << "\\u" << std::setw(4) << cp;
+          tr << oss.str();
+        }
+      }
+      continue;
+    }
+    if ( unicode::is_supplementary_plane( cp ) ) {
+      unsigned high, low;
+      unicode::convert_surrogate( cp, &high, &low );
+      std::ostringstream oss;
+      oss << std::hex << std::setfill('0')
+          << "\\u" << std::setw(4) << high
+          << "\\u" << std::setw(4) << low;
+      tr << oss.str();
+      continue;
+    }
+    switch ( cp ) {
+      case '\\':
+      case '"':
+        tr << '\\';
+        // no break;
+      default: {
+        utf8::encoded_char_type ec;
+        tr.write( ec, utf8::encode( cp, ec ) );
+      }
+    }
   }
   tr << '"';
 }

=== modified file 'src/api/serialization/serializer.h'
--- src/api/serialization/serializer.h	2012-07-13 06:50:40 +0000
+++ src/api/serialization/serializer.h	2012-07-19 17:41:22 +0000
@@ -402,7 +402,7 @@
 
     void emit_jsoniq_xdm_node(store::Item *item, int depth);
 
-    void emit_json_string(zstring string);
+    void emit_json_string(zstring const &string);
 
     store::Item_t theJSONiqValueName;
     store::Item_t theTypeName;

=== modified file 'src/util/ascii_util.h'
--- src/util/ascii_util.h	2012-07-12 17:29:55 +0000
+++ src/util/ascii_util.h	2012-07-19 17:41:22 +0000
@@ -141,6 +141,25 @@
 }
 
 /**
+ * Checks whether the given character is a control character.  This function
+ * exists to make a proper function out of the standard iscntrl(3) that may be
+ * implemented as a macro.
+ *
+ * @param CharType The character type.
+ * @param c The character to check.
+ * @return Returns \c true only if the character is a control character.
+ */
+template<typename CharType> inline
+bool is_cntrl( CharType c ) {
+#ifdef WIN32
+  // Windows' iscntrl() implementation crashes for non-ASCII characters.
+  return __isascii( c ) && iscntrl( c );
+#else
+  return iscntrl( c );
+#endif
+}
+
+/**
  * Checks whether the given character is a decimal digit.  This function exists
  * to make a proper function out of the standard isdigit(3) that may be
  * implemented as a macro.
@@ -160,6 +179,25 @@
 }
 
 /**
+ * Checks whether the given character is a printing character.  This function
+ * exists to make a proper function out of the standard isprint(3) that may be
+ * implemented as a macro.
+ *
+ * @param CharType The character type.
+ * @param c The character to check.
+ * @return Returns \c true only if the character is a printing character.
+ */
+template<typename CharType> inline
+bool is_print( CharType c ) {
+#ifdef WIN32
+  // Windows' isprint() implementation crashes for non-ASCII characters.
+  return __isascii( c ) && isprint( c );
+#else
+  return isprint( c );
+#endif
+}
+
+/**
  * Checks whether the given character is a punctuation character.  This function
  * exists to make a proper function out of the standard ispunct(3) that may be
  * implemented as a macro.

=== modified file 'test/rbkt/ExpQueryResults/zorba/jsoniq/coll_dyn_03.xml.res'
--- test/rbkt/ExpQueryResults/zorba/jsoniq/coll_dyn_03.xml.res	2012-02-08 02:23:20 +0000
+++ test/rbkt/ExpQueryResults/zorba/jsoniq/coll_dyn_03.xml.res	2012-07-19 17:41:22 +0000
@@ -1,1 +1,1 @@
-[ "\u000aXXX-01" ]{ "p1" : 1, "p2" : 2 }[ "\u000aXXX-02" ][ "\u000aXXX-03" ]{ "p1" : 3, "p2" : 4 }{ "p1" : 1, "p2" : 2 }{ "p1" : 5 }[ "\u000aXXX-04" ]{ "index" : 3 }[ "\u000aXXX-05" ][ "\u000aXXX-06" ]{ "p1" : 3, "p2" : 4 }{ "p1" : 1, "p2" : 2 }[ 1, 2, 3 ]{ "p1" : 6 }[  ]{ "p1" : 5 }[ "\u000aXXX-07" ][ "\u000aXXX-08" ][ 1 ][ "\u000aXXX-09" ]{ "p1" : 3, "p2" : 4 }[ 1, 2, 3 ]{ "p1" : 6 }[  ]{ "p1" : 5 }[ "\u000aXXX-10" ][ "\u000aXXX-11" ]{ "p1" : 3, "p2" : 4 }[ 1, 2, 3 ]{ "p1" : 6 }[ "\u000a" ]
+[ "\nXXX-01" ]{ "p1" : 1, "p2" : 2 }[ "\nXXX-02" ][ "\nXXX-03" ]{ "p1" : 3, "p2" : 4 }{ "p1" : 1, "p2" : 2 }{ "p1" : 5 }[ "\nXXX-04" ]{ "index" : 3 }[ "\nXXX-05" ][ "\nXXX-06" ]{ "p1" : 3, "p2" : 4 }{ "p1" : 1, "p2" : 2 }[ 1, 2, 3 ]{ "p1" : 6 }[  ]{ "p1" : 5 }[ "\nXXX-07" ][ "\nXXX-08" ][ 1 ][ "\nXXX-09" ]{ "p1" : 3, "p2" : 4 }[ 1, 2, 3 ]{ "p1" : 6 }[  ]{ "p1" : 5 }[ "\nXXX-10" ][ "\nXXX-11" ]{ "p1" : 3, "p2" : 4 }[ 1, 2, 3 ]{ "p1" : 6 }[ "\n" ]

=== added file 'test/rbkt/ExpQueryResults/zorba/jsoniq/string_escapes.xml.res'
--- test/rbkt/ExpQueryResults/zorba/jsoniq/string_escapes.xml.res	1970-01-01 00:00:00 +0000
+++ test/rbkt/ExpQueryResults/zorba/jsoniq/string_escapes.xml.res	2012-07-19 17:41:22 +0000
@@ -0,0 +1,1 @@
+{ "string" : "\n\r\t\\\"" }

=== added file 'test/rbkt/ExpQueryResults/zorba/jsoniq/supplemental_plane.xml.res'
--- test/rbkt/ExpQueryResults/zorba/jsoniq/supplemental_plane.xml.res	1970-01-01 00:00:00 +0000
+++ test/rbkt/ExpQueryResults/zorba/jsoniq/supplemental_plane.xml.res	2012-07-19 17:41:22 +0000
@@ -0,0 +1,1 @@
+{ "string" : "\ud83d\udc4a" }

=== added file 'test/rbkt/Queries/zorba/jsoniq/string_escapes.xq'
--- test/rbkt/Queries/zorba/jsoniq/string_escapes.xq	1970-01-01 00:00:00 +0000
+++ test/rbkt/Queries/zorba/jsoniq/string_escapes.xq	2012-07-19 17:41:22 +0000
@@ -0,0 +1,2 @@
+let $s := "&#10;&#13;&#9;&#92;&#34;"
+return { "string": $s }

=== added file 'test/rbkt/Queries/zorba/jsoniq/supplemental_plane.xq'
--- test/rbkt/Queries/zorba/jsoniq/supplemental_plane.xq	1970-01-01 00:00:00 +0000
+++ test/rbkt/Queries/zorba/jsoniq/supplemental_plane.xq	2012-07-19 17:41:22 +0000
@@ -0,0 +1,2 @@
+let $s := "&#128074;"
+return { "string": $s }