Merge pull request #104939 from HolonProduction/json-rpc-notification-return-fix

JSONRPC: Fix notification return behavior

modules/jsonrpc/jsonrpc.cpp

@@ -100,9 +100,6 @@ Variant JSONRPC::process_action(const Variant &p_action, bool p_process_arr_elem
 	if (p_action.get_type() == Variant::DICTIONARY) {
 		Dictionary dict = p_action;
 		String method = dict.get("method", "");
-		if (method.begins_with("$/")) {
-			return ret;
-		}
 
 		Array args;
 		if (dict.has("params")) {
@@ -114,8 +111,11 @@ Variant JSONRPC::process_action(const Variant &p_action, bool p_process_arr_elem
 			}
 		}
 
+		/// A Notification is a Request object without an "id" member.
+		bool is_notification = !dict.has("id");
+
 		Variant id;
-		if (dict.has("id")) {
+		if (!is_notification) {
 			id = dict["id"];
 
 			// Account for implementations that discern between int and float on the json serialization level, by using an int if there is a .0 fraction. See #100914
@@ -126,23 +126,33 @@ Variant JSONRPC::process_action(const Variant &p_action, bool p_process_arr_elem
 
 		if (methods.has(method)) {
 			Variant call_ret = methods[method].callv(args);
-			if (id.get_type() != Variant::NIL) {
-				ret = make_response(call_ret, id);
-			}
+			ret = make_response(call_ret, id);
 		} else {
 			ret = make_response_error(JSONRPC::METHOD_NOT_FOUND, "Method not found: " + method, id);
 		}
+
+		/// The Server MUST NOT reply to a Notification
+		if (is_notification) {
+			ret = Variant();
+		}
 	} else if (p_action.get_type() == Variant::ARRAY && p_process_arr_elements) {
 		Array arr = p_action;
-		int size = arr.size();
-		if (size) {
+		if (!arr.is_empty()) {
 			Array arr_ret;
-			for (int i = 0; i < size; i++) {
-				const Variant &var = arr.get(i);
-				arr_ret.push_back(process_action(var));
+			for (const Variant &var : arr) {
+				Variant res = process_action(var);
+
+				if (res.get_type() != Variant::NIL) {
+					arr_ret.push_back(res);
+				}
+			}
+
+			/// If there are no Response objects contained within the Response array as it is to be sent to the client, the server MUST NOT return an empty Array and should return nothing at all.
+			if (!arr_ret.is_empty()) {
+				ret = arr_ret;
 			}
-			ret = arr_ret;
 		} else {
+			/// If the batch rpc call itself fails to be recognized ... as an Array with at least one value, the response from the Server MUST be a single Response object.
 			ret = make_response_error(JSONRPC::INVALID_REQUEST, "Invalid Request");
 		}
 	} else {

modules/jsonrpc/tests/test_jsonrpc.cpp

@@ -79,4 +79,10 @@ void test_process_action_bad_method(const Dictionary &p_in) {
 	check_error_no_method(out_dict);
 }
 
+void test_no_response(const Variant &p_in) {
+	TestClassJSONRPC json_rpc = TestClassJSONRPC();
+	const Variant &res = json_rpc.process_action(p_in, true);
+	CHECK(res.get_type() == Variant::NIL);
+}
+
 } // namespace TestJSONRPC

modules/jsonrpc/tests/test_jsonrpc.h

@@ -46,6 +46,7 @@ TEST_CASE("[JSONRPC] process_action invalid") {
 	check_invalid(json_rpc.process_action(1234));
 	check_invalid(json_rpc.process_action(false));
 	check_invalid(json_rpc.process_action(3.14159));
+	check_invalid(json_rpc.process_action(Array()));
 }
 
 void check_invalid_string(const String &p_str);
@@ -57,6 +58,7 @@ TEST_CASE("[JSONRPC] process_string invalid") {
 	check_invalid_string(json_rpc.process_string("1234"));
 	check_invalid_string(json_rpc.process_string("false"));
 	check_invalid_string(json_rpc.process_string("3.14159"));
+	check_invalid_string(json_rpc.process_string("[]"));
 }
 
 class TestClassJSONRPC : public JSONRPC {
@@ -125,9 +127,73 @@ void test_process_action_bad_method(const Dictionary &p_in);
 
 TEST_CASE("[JSONRPC] process_action bad method") {
 	Dictionary in_dict;
+	in_dict["id"] = 1;
 	in_dict["method"] = "nothing";
 
 	test_process_action_bad_method(in_dict);
 }
 
+void test_no_response(const Variant &p_in);
+
+TEST_CASE("[JSONRPC] process_action notification") {
+	Dictionary in_dict = Dictionary();
+	in_dict["method"] = "something";
+	in_dict["params"] = "yes";
+
+	test_no_response(in_dict);
+}
+
+TEST_CASE("[JSONRPC] process_action notification bad method") {
+	Dictionary in_dict;
+	in_dict["method"] = "nothing";
+
+	test_no_response(in_dict);
+}
+
+TEST_CASE("[JSONRPC] process_action notification batch") {
+	Array in;
+	Dictionary in_1;
+	in_1["method"] = "something";
+	in_1["params"] = "more";
+	in.push_back(in_1);
+	Dictionary in_2;
+	in_2["method"] = "something";
+	in_2["params"] = "yes";
+	in.push_back(in_2);
+
+	test_no_response(in);
+}
+
+TEST_CASE("[JSONRPC] mixed batch") {
+	Array in;
+	Dictionary in_1;
+	in_1["method"] = "something";
+	in_1["id"] = 1;
+	in_1["params"] = "more";
+	in.push_back(in_1);
+	Dictionary in_2;
+	in_2["method"] = "something";
+	in_2["id"] = 2;
+	in_2["params"] = "yes";
+	in.push_back(in_2);
+	Dictionary in_3;
+	in_3["method"] = "something";
+	in_3["params"] = "yes";
+	in.push_back(in_3);
+
+	Array expected;
+	Dictionary expected_1;
+	expected_1["jsonrpc"] = "2.0";
+	expected_1["id"] = 1;
+	expected_1["result"] = "more, please";
+	expected.push_back(expected_1);
+	Dictionary expected_2;
+	expected_2["jsonrpc"] = "2.0";
+	expected_2["id"] = 2;
+	expected_2["result"] = "yes, please";
+	expected.push_back(expected_2);
+
+	test_process_action(in, expected, true);
+}
+
 } // namespace TestJSONRPC