From a09bc5045b875d2e98e1c017f28a6162f2fbec1c Mon Sep 17 00:00:00 2001
From: Wen Hui <wen.hui.ware@gmail.com>
Date: Wed, 15 Dec 2021 02:46:32 -0500
Subject: [PATCH] Error message improvement for CONFIG SET command (#9924)

When CONFIG SET fails, print the name of the config that failed.
This is helpful since config set is now variadic.

however, there are cases where several configs have the same apply
function, and we can't be sure which one of them caused the failure.
---
 src/config.c                 | 16 ++++++++++++----
 tests/unit/introspection.tcl |  4 ++--
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/config.c b/src/config.c
index 1827abbae..0bf8df196 100644
--- a/src/config.c
+++ b/src/config.c
@@ -674,6 +674,8 @@ static void restoreBackupConfig(standardConfig **set_configs, sds *old_values, i
 
 void configSetCommand(client *c) {
     const char *errstr = NULL;
+    const char *invalid_arg_name = NULL;
+    const char *err_arg_name = NULL;
     standardConfig **set_configs; /* TODO: make this a dict for better performance */
     sds *new_values;
     sds *old_values = NULL;
@@ -712,6 +714,7 @@ void configSetCommand(client *c) {
                     if (config->flags & IMMUTABLE_CONFIG) {
                         /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
                         errstr = "can't set immutable config";
+                        err_arg_name = c->argv[2+i*2]->ptr;
                         invalid_args = 1;
                     }
 
@@ -720,6 +723,7 @@ void configSetCommand(client *c) {
                         if (set_configs[j] == config) {
                             /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
                             errstr = "duplicate parameter";
+                            err_arg_name = c->argv[2+i*2]->ptr;
                             invalid_args = 1;
                             break;
                         }
@@ -733,7 +737,7 @@ void configSetCommand(client *c) {
         /* Fail if we couldn't find this config */
         /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
         if (!invalid_args && !set_configs[i]) {
-            errstr = "unrecognized parameter";
+            invalid_arg_name = c->argv[2+i*2]->ptr;
             invalid_args = 1;
         }
     }
@@ -749,6 +753,7 @@ void configSetCommand(client *c) {
         int res = performInterfaceSet(set_configs[i], new_values[i], &errstr);
         if (!res) {
             restoreBackupConfig(set_configs, old_values, i+1, NULL);
+            err_arg_name = set_configs[i]->name;
             goto err;
         } else if (res == 1) {
             /* A new value was set, if this config has an apply function then store it for execution later */
@@ -775,6 +780,7 @@ void configSetCommand(client *c) {
         if (!apply_fns[i](&errstr)) {
             serverLog(LL_WARNING, "Failed applying new configuration. Possibly related to new %s setting. Restoring previous settings.", set_configs[config_map_fns[i]]->name);
             restoreBackupConfig(set_configs, old_values, config_count, apply_fns);
+            err_arg_name = set_configs[config_map_fns[i]]->name;
             goto err;
         }
     }
@@ -782,10 +788,12 @@ void configSetCommand(client *c) {
     goto end;
 
 err:
-    if (errstr) {
-        addReplyErrorFormat(c,"Config set failed - %s", errstr);
+    if (invalid_arg_name) {
+        addReplyErrorFormat(c,"Unknown option or number of arguments for CONFIG SET - '%s'", invalid_arg_name);
+    } else if (errstr) {
+        addReplyErrorFormat(c,"CONFIG SET failed (possibly related to argument '%s') - %s", err_arg_name, errstr);
     } else {
-        addReplyError(c,"Invalid arguments");
+        addReplyErrorFormat(c,"CONFIG SET failed (possibly related to argument '%s')", err_arg_name);
     }
 end:
     zfree(set_configs);
diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl
index cf2b0e39e..5e4d2b5e3 100644
--- a/tests/unit/introspection.tcl
+++ b/tests/unit/introspection.tcl
@@ -322,7 +322,7 @@ start_server {tags {"introspection"}} {
         # Set some value to maxmemory
         assert_equal [r config set maxmemory 10000002] "OK"
         # Set another value to maxmeory together with another invalid config
-        assert_error "ERR Config set failed - percentage argument must be less or equal to 100" {
+        assert_error "ERR CONFIG SET failed (possibly related to argument 'maxmemory-clients') - percentage argument must be less or equal to 100" {
             r config set maxmemory 10000001 maxmemory-clients 200% client-query-buffer-limit invalid
         }
         # Validate we rolled back to original values
@@ -375,7 +375,7 @@ start_server {tags {"introspection"}} {
 
         # Try to listen on the used port, pass some more configs to make sure the
         # returned failure message is for the first bad config and everything is rolled back.
-        assert_error "ERR Config set failed - Unable to listen on this port*" {
+        assert_error "ERR CONFIG SET failed (possibly related to argument 'port') - Unable to listen on this port*" {
             eval "r config set $some_configs"
         }