changeset 184:150cfa0c71e1

Merge pull request #1 from piper-audio/fix/regression-json-responses

Fix/regression json responses

--- a/vamp-json/VampJson.h	Tue Jan 31 18:05:11 2017 +0000
+++ b/vamp-json/VampJson.h	Fri Feb 03 13:00:42 2017 +0000
@@ -305,7 +305,7 @@
             return {};
         }
 
-        od = toConfiguredOutputDescriptor(j, err);
+        od = toConfiguredOutputDescriptor(j["configured"], err);
         if (failed(err)) return {};
     
         toBasicDescriptor(j["basic"], od, err);
@@ -849,7 +849,7 @@
     toListResponse(json11::Json j, std::string &err) {
 
         ListResponse resp;
-        for (const auto &a: j["result"]["available"].array_items()) {
+        for (const auto &a: j["available"].array_items()) {
             resp.available.push_back(toPluginStaticData(a, err));
             if (failed(err)) return {};
         }
@@ -1109,11 +1109,19 @@
 
     static bool
     successful(json11::Json j, std::string &err) {
-        if (!j["success"].is_bool()) {
-            err = "bool expected for success";
+        const bool hasResult = j["result"].is_object();
+        const bool hasError = j["error"].is_object();
+        if (hasResult && hasError) {
+            err = "valid response may contain only one of result and error objects";
             return false;
+        } else if (hasError) {
+            return false;
+        } else if (!hasResult) {
+            err = "either a result or an error object is required for a valid response";
+            return false;
+        } else {
+            return true;
         }
-        return j["success"].bool_value();
     }
 
     static void
@@ -1298,7 +1306,8 @@
     static json11::Json
     fromError(std::string errorText,
               RRType responseType,
-              const json11::Json &id) {
+              const json11::Json &id,
+              bool writeVerbatimError = false) {
 
         json11::Json::object jo;
         markRPC(jo);
@@ -1315,7 +1324,7 @@
         json11::Json::object eo;
         eo["code"] = 0;
 
-        if (responseType == RRType::NotValid) {
+        if (responseType == RRType::NotValid || writeVerbatimError) {
             eo["message"] = errorText;
         } else {
             eo["message"] = 

--- a/vamp-server/convert.cpp	Tue Jan 31 18:05:11 2017 +0000
+++ b/vamp-server/convert.cpp	Fri Feb 03 13:00:42 2017 +0000
@@ -295,8 +295,10 @@
     VampJson::BufferSerialisation serialisation =
         VampJson::BufferSerialisation::Array;
 
-    rr.success = j["success"].bool_value();
-    rr.errorText = j["errorText"].string_value();
+    const bool isSuccess = j["result"].is_object();
+    const bool isError = j["error"].is_object();
+    rr.success = isSuccess;
+    rr.errorText = isError ? j["error"]["message"].string_value() : "";
 
     switch (rr.type) {
 
@@ -335,11 +337,11 @@
     Json id = writeJsonId(rr.id);
 
     if (!rr.success) {
-
-        j = VampJson::fromError(rr.errorText, rr.type, id);
+         // errorText here likely contains a full message produced by simple-server
+         // setting writeVerbatimError to true avoids doubling error descriptions 
+        j = VampJson::fromError(rr.errorText, rr.type, id, true);
 
     } else {
-    
         switch (rr.type) {
 
         case RRType::List:
@@ -659,7 +661,6 @@
             writeOutput(outformat, rr);
 
         } catch (std::exception &e) {
-
             cerr << "Error: " << e.what() << endl;
             exit(1);
         }

--- a/vamp-server/test.sh	Tue Jan 31 18:05:11 2017 +0000
+++ b/vamp-server/test.sh	Fri Feb 03 13:00:42 2017 +0000
@@ -27,6 +27,7 @@
 allrespfile="$tmpdir/resp.all"
 input="$tmpdir/input"
 expected="$tmpdir/expected"
+expected_less_strict="$tmpdir/expected-less-strict"
 obtained="$tmpdir/obtained"
 
 validate() {
@@ -82,23 +83,31 @@
 {"error": {"code": 0, "message": "error in finish request: unknown plugin handle supplied to finish"}, "id": "blah", "jsonrpc": "2.0", "method": "finish"}
 EOF
 
-# We run the whole test twice, once with the server in Capnp mode
-# (converting to JSON using piper-convert) and once with it directly
-# in JSON mode
+# We run the whole test three times,
+# to cover (de)serialisation of json and capnp requests and responses
+# as well as exercising both server modes (json and capnp)
+# converting / reading from capnp requests is currently not tested
 
 #debugflag=-d
 debugflag=
 
-for format in json capnp ; do  # nb must be json first: see comment at end of loop
+for request_response_conversion in none json_to_json json_to_capnp ; do # nb json_to_capnp must be last: see comment in else case
 
     ( export VAMP_PATH="$vampsdkdir"/examples ;
       while read request ; do
           validate_request "$request"
           echo "$request"
       done |
-          if [ "$format" = "json" ]; then
+          if [ "$request_response_conversion" = "none" ]; then
               "$bindir"/piper-vamp-simple-server $debugflag json
+          elif [ "$request_response_conversion" = "json_to_json" ]; then
+              "$bindir"/piper-convert request -i json -o json |
+                  "$bindir"/piper-vamp-simple-server $debugflag json |
+                  "$bindir"/piper-convert response -i json -o json
           else
+              # The capnp output doesn't preserve the method name in error
+              # responses, so replace those now that we've done the json tests
+              perl -i -p -e 's/(error.*"method": )"[^"]*"/$1"invalid"/' "$expected"
               "$bindir"/piper-convert request -i json -o capnp |
                   "$bindir"/piper-vamp-simple-server $debugflag capnp |
                   "$bindir"/piper-convert response -i capnp -o json
@@ -112,11 +121,16 @@
     # Skip plugin lists
     tail -n +4 "$allrespfile" > "$obtained"
 
-    echo "Checking response contents against expected contents..."
-    if ! cmp "$obtained" "$expected"; then
-        diff -U 1 "$obtained" "$expected"
+    echo "Checking response contents against expected contents..."  
+    # the expected configuration response is fragile, capnp fills in optional fields,
+    # json doesn't - which is fine behaviour, but causes the test to fail - remove empty binCount and binNames
+    expected_without_optional_fields=$( cat "$expected" | sed -E 's/\"(binCount|binNames)\": ?((\[\])|0),? ?//g')
+    echo "$expected_without_optional_fields" > "$expected_less_strict"
+
+    if cmp "$obtained" "$expected" -s || cmp "$obtained" "$expected_less_strict" -s; then
+      echo "OK"
     else
-        echo "OK"
+      diff -U 1 "$obtained" "$expected"
     fi
 
     echo "Checking plugin counts from list responses..."
@@ -153,10 +167,6 @@
     echo OK
     
     rm "$allrespfile"
-
-    # The capnp output doesn't preserve the method name in error
-    # responses, so replace those now that we've done the json test
-    perl -i -p -e 's/(error.*"method": )"[^"]*"/$1"invalid"/' "$expected"
 done
 
 echo "Tests succeeded"  # set -e at top should ensure we don't get here otherwise