From 664efbd2894732ad0e44d6643b1fce4cd6f3b6ca Mon Sep 17 00:00:00 2001
From: Michael Ilyin <milyin@gmail.com>
Date: Tue, 16 Apr 2024 10:42:40 +0200
Subject: [PATCH] replaced unwraps for plugin configurations insert to correct
 error check (#936)

* replaced unwraps for plugin configurations insert to correct error check

* cargo fmt

* comments corrected
---
 commons/zenoh-config/src/lib.rs | 40 ++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/commons/zenoh-config/src/lib.rs b/commons/zenoh-config/src/lib.rs
index 2b5485fa6b..fcc5379f7a 100644
--- a/commons/zenoh-config/src/lib.rs
+++ b/commons/zenoh-config/src/lib.rs
@@ -19,7 +19,7 @@ mod include;
 use include::recursive_include;
 use secrecy::{CloneableSecret, DebugSecret, Secret, SerializableSecret, Zeroize};
 use serde::{Deserialize, Serialize};
-use serde_json::Value;
+use serde_json::{Map, Value};
 #[allow(unused_imports)]
 use std::convert::TryFrom; // This is a false positive from the rust analyser
 use std::{
@@ -1185,20 +1185,34 @@ impl validated_struct::ValidatedMap for PluginsConfig {
             .unwrap()
             .entry(plugin)
             .or_insert(Value::Null);
-        let mut new_value = value.clone().merge(key, new_value)?;
-        if let Some(validator) = self.validator.upgrade() {
-            match validator.check_config(
-                plugin,
-                key,
-                value.as_object().unwrap(),
-                new_value.as_object().unwrap(),
-            ) {
-                Ok(Some(val)) => new_value = Value::Object(val),
-                Ok(None) => {}
+        let new_value = value.clone().merge(key, new_value)?;
+        *value = if let Some(validator) = self.validator.upgrade() {
+            // New plugin configuration for compare with original configuration.
+            // Return error if it's not an object.
+            // Note: it's ok if original "new_value" is not an object: this can be some subkey of the plugin configuration. But the result of the merge should be an object.
+            // Error occurs  if the original plugin configuration is not an object itself (e.g. null).
+            let Some(new_plugin_config) = new_value.as_object() else {
+                return Err(format!(
+                    "Attempt to provide non-object value as configuration for plugin `{plugin}`"
+                )
+                .into());
+            };
+            // Original plugin configuration for compare with new configuration.
+            // If for some reason it's not defined or not an object, we default to an empty object.
+            // Usually this happens when no plugin with this name defined. Reject then should be performed by the validator with `plugin not found` error.
+            let empty_config = Map::new();
+            let current_plugin_config = value.as_object().unwrap_or(&empty_config);
+            match validator.check_config(plugin, key, current_plugin_config, new_plugin_config) {
+                // Validator made changes to the proposed configuration, take these changes
+                Ok(Some(val)) => Value::Object(val),
+                // Validator accepted the proposed configuration as is
+                Ok(None) => new_value,
+                // Validator rejected the proposed configuration
                 Err(e) => return Err(format!("{e}").into()),
             }
-        }
-        *value = new_value;
+        } else {
+            new_value
+        };
         Ok(())
     }
     fn get<'a>(&'a self, mut key: &str) -> Result<&'a dyn Any, GetError> {