From bfa04dd9924114c10098087ff07e52fc1caf65af Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 05:55:08 +0000
Subject: [PATCH 1/7] Fix: DBInterface::get/get_all() return nullable strings

---
 common/dbinterface.cpp      | 29 ++++++++++++++---------------
 common/dbinterface.h        |  5 +++--
 common/sonicv2connector.cpp |  4 ++--
 common/sonicv2connector.h   |  4 ++--
 tests/redis_ut.cpp          |  2 +-
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp
index 53e6ccd31..eb8962338 100644
--- a/common/dbinterface.cpp
+++ b/common/dbinterface.cpp
@@ -55,7 +55,7 @@ bool DBInterface::exists(const string& dbName, const std::string& key)
     return m_redisClient.at(dbName).exists(key);
 }
 
-std::string DBInterface::get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking)
+std::shared_ptr<std::string> DBInterface::get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking)
 {
     auto innerfunc = [&]
     {
@@ -67,9 +67,9 @@ std::string DBInterface::get(const std::string& dbName, const std::string& hash,
             throw UnavailableDataError(message, hash);
         }
         const std::string& value = *pvalue;
-        return value == "None" ? "" : value;
+        return value == "None" ? shared_ptr<string>() : make_shared<string>(value);
     };
-    return blockable<std::string>(innerfunc, dbName, blocking);
+    return blockable<shared_ptr<string>>(innerfunc, dbName, blocking);
 }
 
 bool DBInterface::hexists(const std::string& dbName, const std::string& hash, const std::string& key)
@@ -77,31 +77,30 @@ bool DBInterface::hexists(const std::string& dbName, const std::string& hash, co
     return m_redisClient.at(dbName).hexists(hash, key);
 }
 
-std::map<std::string, std::string> DBInterface::get_all(const std::string& dbName, const std::string& hash, bool blocking)
+std::map<std::string, std::shared_ptr<std::string>> DBInterface::get_all(const std::string& dbName, const std::string& hash, bool blocking)
 {
     auto innerfunc = [&]
     {
-        std::map<std::string, std::string> map;
-        m_redisClient.at(dbName).hgetall(hash, std::inserter(map, map.end()));
+        map<string, string> table;
+        m_redisClient.at(dbName).hgetall(hash, std::inserter(table, table.end()));
 
-        if (map.empty())
+        if (table.empty())
         {
             std::string message = "Key '{" + hash + "}' unavailable in database '{" + dbName + "}'";
             SWSS_LOG_WARN("%s", message.c_str());
             throw UnavailableDataError(message, hash);
         }
-        for (auto& i : map)
+        map<string, shared_ptr<string>> ret;
+        for (auto& i : table)
         {
-            std::string& value = i.second;
-            if (value == "None")
-            {
-                value = "";
-            }
+            auto& key = i.first;
+            auto& value = i.second;
+            ret.emplace(key, value == "None" ? shared_ptr<string>() : make_shared<string>(value));
         }
 
-        return map;
+        return ret;
     };
-    return blockable<std::map<std::string, std::string>>(innerfunc, dbName, blocking);
+    return blockable<map<string, shared_ptr<string>>>(innerfunc, dbName, blocking);
 }
 
 std::vector<std::string> DBInterface::keys(const std::string& dbName, const char *pattern, bool blocking)
diff --git a/common/dbinterface.h b/common/dbinterface.h
index ccf114a07..ce4fc2987 100644
--- a/common/dbinterface.h
+++ b/common/dbinterface.h
@@ -3,6 +3,7 @@
 #include <stdint.h>
 #include <unistd.h>
 #include <stdexcept>
+#include <memory>
 
 #include "dbconnector.h"
 #include "logger.h"
@@ -37,9 +38,9 @@ class DBInterface
     // Delete all keys which match %pattern from DB
     void delete_all_by_pattern(const std::string& dbName, const std::string& pattern);
     bool exists(const std::string& dbName, const std::string& key);
-    std::string get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false);
+    std::shared_ptr<std::string> get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false);
     bool hexists(const std::string& dbName, const std::string& hash, const std::string& key);
-    std::map<std::string, std::string> get_all(const std::string& dbName, const std::string& hash, bool blocking = false);
+    std::map<std::string, std::shared_ptr<std::string>> get_all(const std::string& dbName, const std::string& hash, bool blocking = false);
     std::vector<std::string> keys(const std::string& dbName, const char *pattern = "*", bool blocking = false);
     std::pair<int, std::vector<std::string>> scan(const std::string& db_name, int cursor, const char *match, uint32_t count);
     int64_t publish(const std::string& dbName, const std::string& channel, const std::string& message);
diff --git a/common/sonicv2connector.cpp b/common/sonicv2connector.cpp
index fcce5c2d4..13e8d3eab 100644
--- a/common/sonicv2connector.cpp
+++ b/common/sonicv2connector.cpp
@@ -74,7 +74,7 @@ std::pair<int, std::vector<std::string>> SonicV2Connector_Native::scan(const std
     return m_dbintf.scan(db_name, cursor, match, count);
 }
 
-std::string SonicV2Connector_Native::get(const std::string& db_name, const std::string& _hash, const std::string& key, bool blocking)
+std::shared_ptr<std::string> SonicV2Connector_Native::get(const std::string& db_name, const std::string& _hash, const std::string& key, bool blocking)
 {
     return m_dbintf.get(db_name, _hash, key, blocking);
 }
@@ -84,7 +84,7 @@ bool SonicV2Connector_Native::hexists(const std::string& db_name, const std::str
     return m_dbintf.hexists(db_name, _hash, key);
 }
 
-std::map<std::string, std::string> SonicV2Connector_Native::get_all(const std::string& db_name, const std::string& _hash, bool blocking)
+std::map<std::string, std::shared_ptr<std::string>> SonicV2Connector_Native::get_all(const std::string& db_name, const std::string& _hash, bool blocking)
 {
     return m_dbintf.get_all(db_name, _hash, blocking);
 }
diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h
index d34593185..92ef4fb1d 100644
--- a/common/sonicv2connector.h
+++ b/common/sonicv2connector.h
@@ -35,11 +35,11 @@ class SonicV2Connector_Native
 
     std::pair<int, std::vector<std::string>> scan(const std::string& db_name, int cursor = 0, const char *match = "", uint32_t count = 10);
 
-    std::string get(const std::string& db_name, const std::string& _hash, const std::string& key, bool blocking=false);
+    std::shared_ptr<std::string> get(const std::string& db_name, const std::string& _hash, const std::string& key, bool blocking=false);
 
     bool hexists(const std::string& db_name, const std::string& _hash, const std::string& key);
 
-    std::map<std::string, std::string> get_all(const std::string& db_name, const std::string& _hash, bool blocking=false);
+    std::map<std::string, std::shared_ptr<std::string>> get_all(const std::string& db_name, const std::string& _hash, bool blocking=false);
 
     void hmset(const std::string& db_name, const std::string &key, const std::map<std::string, std::string> &values);
 
diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp
index 8999a14eb..4125b3a8f 100644
--- a/tests/redis_ut.cpp
+++ b/tests/redis_ut.cpp
@@ -325,7 +325,7 @@ TEST(DBConnector, DBInterface)
     auto fvs = db.get_all("TEST_DB", "key0");
     auto rc = fvs.find("field1");
     EXPECT_NE(rc, fvs.end());
-    EXPECT_EQ(rc->second, "value2");
+    EXPECT_EQ(*rc->second, "value2");
 }
 
 TEST(DBConnector, RedisClient)

From 3c49ae53b8346bedd6aedaf7874c06a82987ac1c Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 11:36:40 +0000
Subject: [PATCH 2/7] Swig cast C++ shared_ptr string into python str

---
 pyext/swsscommon.i     | 16 ++++++++++++++++
 tests/test_redis_ut.py |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i
index 99fc6eb05..d48519527 100644
--- a/pyext/swsscommon.i
+++ b/pyext/swsscommon.i
@@ -41,6 +41,7 @@
 %include <std_vector.i>
 %include <std_pair.i>
 %include <std_map.i>
+%include <std_shared_ptr.i>
 %include <typemaps.i>
 %include <stdint.i>
 %include <exception.i>
@@ -109,6 +110,21 @@
     $1 = &temp;
 }
 
+%typemap(out) std::shared_ptr<std::string> %{
+    {
+        std::shared_ptr<std::string>& p = static_cast<std::shared_ptr<std::string>&>($1);
+        if(p)
+        {
+            $result = PyUnicode_FromStringAndSize(p->c_str(), p->size());
+        }
+        else
+        {
+            $result = Py_None;
+            Py_INCREF(Py_None);
+        }
+    }
+%}
+
 %typemap(argout) swss::Selectable ** {
     PyObject* temp = NULL;
     if (!PyList_Check($result)) {
diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py
index 8a60ab7ae..d1f63f4af 100644
--- a/tests/test_redis_ut.py
+++ b/tests/test_redis_ut.py
@@ -204,6 +204,8 @@ def test_DBInterface():
     redisclient = db.get_redis_client("TEST_DB")
     redisclient.flushdb()
     db.set("TEST_DB", "key0", "field1", "value2")
+    val = db.get("TEST_DB", "key0", "field1")
+    assert val == "value2"
     fvs = db.get_all("TEST_DB", "key0")
     assert "field1" in fvs
     assert fvs["field1"] == "value2"

From bfb42e99ec4b48605f4eca0a4c85f859349dd58f Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 11:55:41 +0000
Subject: [PATCH 3/7] Refactor

---
 pyext/swsscommon.i | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i
index d48519527..5380250ea 100644
--- a/pyext/swsscommon.i
+++ b/pyext/swsscommon.i
@@ -89,6 +89,21 @@
 %template(GetTableResult) std::map<std::string, std::map<std::string, std::string>>;
 %template(GetConfigResult) std::map<std::string, std::map<std::string, std::map<std::string, std::string>>>;
 
+%typemap(out) std::shared_ptr<std::string> %{
+    {
+        auto& p = static_cast<std::shared_ptr<std::string>&>($1);
+        if(p)
+        {
+            $result = PyUnicode_FromStringAndSize(p->c_str(), p->size());
+        }
+        else
+        {
+            $result = Py_None;
+            Py_INCREF(Py_None);
+        }
+    }
+%}
+
 %pythoncode %{
     def _FieldValueMap__get(self, key, default=None):
         if key in self:
@@ -110,21 +125,6 @@
     $1 = &temp;
 }
 
-%typemap(out) std::shared_ptr<std::string> %{
-    {
-        std::shared_ptr<std::string>& p = static_cast<std::shared_ptr<std::string>&>($1);
-        if(p)
-        {
-            $result = PyUnicode_FromStringAndSize(p->c_str(), p->size());
-        }
-        else
-        {
-            $result = Py_None;
-            Py_INCREF(Py_None);
-        }
-    }
-%}
-
 %typemap(argout) swss::Selectable ** {
     PyObject* temp = NULL;
     if (!PyList_Check($result)) {

From 9e3b6052b46ba8f8927eef284754d4fb8ee5932c Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 12:05:48 +0000
Subject: [PATCH 4/7] Revert "Fix: DBInterface::get/get_all() return nullable
 strings"

This reverts commit bfa04dd9924114c10098087ff07e52fc1caf65af.
---
 common/dbinterface.cpp      | 23 ++++++++++++-----------
 common/dbinterface.h        |  2 +-
 common/sonicv2connector.cpp |  2 +-
 common/sonicv2connector.h   |  2 +-
 tests/redis_ut.cpp          |  2 +-
 5 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp
index eb8962338..73b3e5f4e 100644
--- a/common/dbinterface.cpp
+++ b/common/dbinterface.cpp
@@ -77,30 +77,31 @@ bool DBInterface::hexists(const std::string& dbName, const std::string& hash, co
     return m_redisClient.at(dbName).hexists(hash, key);
 }
 
-std::map<std::string, std::shared_ptr<std::string>> DBInterface::get_all(const std::string& dbName, const std::string& hash, bool blocking)
+std::map<std::string, std::string> DBInterface::get_all(const std::string& dbName, const std::string& hash, bool blocking)
 {
     auto innerfunc = [&]
     {
-        map<string, string> table;
-        m_redisClient.at(dbName).hgetall(hash, std::inserter(table, table.end()));
+        std::map<std::string, std::string> map;
+        m_redisClient.at(dbName).hgetall(hash, std::inserter(map, map.end()));
 
-        if (table.empty())
+        if (map.empty())
         {
             std::string message = "Key '{" + hash + "}' unavailable in database '{" + dbName + "}'";
             SWSS_LOG_WARN("%s", message.c_str());
             throw UnavailableDataError(message, hash);
         }
-        map<string, shared_ptr<string>> ret;
-        for (auto& i : table)
+        for (auto& i : map)
         {
-            auto& key = i.first;
-            auto& value = i.second;
-            ret.emplace(key, value == "None" ? shared_ptr<string>() : make_shared<string>(value));
+            std::string& value = i.second;
+            if (value == "None")
+            {
+                value = "";
+            }
         }
 
-        return ret;
+        return map;
     };
-    return blockable<map<string, shared_ptr<string>>>(innerfunc, dbName, blocking);
+    return blockable<std::map<std::string, std::string>>(innerfunc, dbName, blocking);
 }
 
 std::vector<std::string> DBInterface::keys(const std::string& dbName, const char *pattern, bool blocking)
diff --git a/common/dbinterface.h b/common/dbinterface.h
index ce4fc2987..a1fcf2a2b 100644
--- a/common/dbinterface.h
+++ b/common/dbinterface.h
@@ -40,7 +40,7 @@ class DBInterface
     bool exists(const std::string& dbName, const std::string& key);
     std::shared_ptr<std::string> get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false);
     bool hexists(const std::string& dbName, const std::string& hash, const std::string& key);
-    std::map<std::string, std::shared_ptr<std::string>> get_all(const std::string& dbName, const std::string& hash, bool blocking = false);
+    std::map<std::string, std::string> get_all(const std::string& dbName, const std::string& hash, bool blocking = false);
     std::vector<std::string> keys(const std::string& dbName, const char *pattern = "*", bool blocking = false);
     std::pair<int, std::vector<std::string>> scan(const std::string& db_name, int cursor, const char *match, uint32_t count);
     int64_t publish(const std::string& dbName, const std::string& channel, const std::string& message);
diff --git a/common/sonicv2connector.cpp b/common/sonicv2connector.cpp
index 13e8d3eab..59552cde2 100644
--- a/common/sonicv2connector.cpp
+++ b/common/sonicv2connector.cpp
@@ -84,7 +84,7 @@ bool SonicV2Connector_Native::hexists(const std::string& db_name, const std::str
     return m_dbintf.hexists(db_name, _hash, key);
 }
 
-std::map<std::string, std::shared_ptr<std::string>> SonicV2Connector_Native::get_all(const std::string& db_name, const std::string& _hash, bool blocking)
+std::map<std::string, std::string> SonicV2Connector_Native::get_all(const std::string& db_name, const std::string& _hash, bool blocking)
 {
     return m_dbintf.get_all(db_name, _hash, blocking);
 }
diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h
index 92ef4fb1d..541b5835f 100644
--- a/common/sonicv2connector.h
+++ b/common/sonicv2connector.h
@@ -39,7 +39,7 @@ class SonicV2Connector_Native
 
     bool hexists(const std::string& db_name, const std::string& _hash, const std::string& key);
 
-    std::map<std::string, std::shared_ptr<std::string>> get_all(const std::string& db_name, const std::string& _hash, bool blocking=false);
+    std::map<std::string, std::string> get_all(const std::string& db_name, const std::string& _hash, bool blocking=false);
 
     void hmset(const std::string& db_name, const std::string &key, const std::map<std::string, std::string> &values);
 
diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp
index 4125b3a8f..8999a14eb 100644
--- a/tests/redis_ut.cpp
+++ b/tests/redis_ut.cpp
@@ -325,7 +325,7 @@ TEST(DBConnector, DBInterface)
     auto fvs = db.get_all("TEST_DB", "key0");
     auto rc = fvs.find("field1");
     EXPECT_NE(rc, fvs.end());
-    EXPECT_EQ(*rc->second, "value2");
+    EXPECT_EQ(rc->second, "value2");
 }
 
 TEST(DBConnector, RedisClient)

From 903bb2eaff010c44a718e05108bfc838e2beaf9f Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 12:26:54 +0000
Subject: [PATCH 5/7] Add test cases

---
 tests/test_redis_ut.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py
index d1f63f4af..bde55445b 100644
--- a/tests/test_redis_ut.py
+++ b/tests/test_redis_ut.py
@@ -203,9 +203,18 @@ def test_DBInterface():
     db.connect("TEST_DB")
     redisclient = db.get_redis_client("TEST_DB")
     redisclient.flushdb()
+
     db.set("TEST_DB", "key0", "field1", "value2")
     val = db.get("TEST_DB", "key0", "field1")
     assert val == "value2"
+    db.set("TEST_DB", "kkk3", "field3", "")
+    val = db.get("TEST_DB", "kkk3", "field3")
+    assert val == ""
+    val = db.get("TEST_DB", "kkk3", "missing")
+    assert val == None
+    val = db.get("TEST_DB", "kkk_missing", "missing")
+    assert val == None
+
     fvs = db.get_all("TEST_DB", "key0")
     assert "field1" in fvs
     assert fvs["field1"] == "value2"

From bcd1d2f8f0e25f54e621ae0233db9dcf4fb85f08 Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Sat, 31 Jul 2021 12:32:40 +0000
Subject: [PATCH 6/7] Add test case

---
 tests/test_redis_ut.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py
index bde55445b..1bebd2e04 100644
--- a/tests/test_redis_ut.py
+++ b/tests/test_redis_ut.py
@@ -204,14 +204,22 @@ def test_DBInterface():
     redisclient = db.get_redis_client("TEST_DB")
     redisclient.flushdb()
 
+    # Case: hset and hget normally
     db.set("TEST_DB", "key0", "field1", "value2")
     val = db.get("TEST_DB", "key0", "field1")
     assert val == "value2"
+    # Case: hset an empty value
     db.set("TEST_DB", "kkk3", "field3", "")
     val = db.get("TEST_DB", "kkk3", "field3")
     assert val == ""
+    # Case: hset an "None" string value, hget will intepret it as true None (feature)
+    db.set("TEST_DB", "kkk3", "field3", "None")
+    val = db.get("TEST_DB", "kkk3", "field3")
+    assert val == None
+    # hget on an existing key but non-existing field
     val = db.get("TEST_DB", "kkk3", "missing")
     assert val == None
+    # hget on an non-existing key and non-existing field
     val = db.get("TEST_DB", "kkk_missing", "missing")
     assert val == None
 

From b26de20325e8abcc793d86990af91221f8b00d9c Mon Sep 17 00:00:00 2001
From: Qi Luo <qiluo-msft@users.noreply.github.com>
Date: Tue, 3 Aug 2021 02:57:46 +0000
Subject: [PATCH 7/7] Add namespace qualifier

---
 common/dbinterface.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp
index 73b3e5f4e..a088960e9 100644
--- a/common/dbinterface.cpp
+++ b/common/dbinterface.cpp
@@ -67,9 +67,9 @@ std::shared_ptr<std::string> DBInterface::get(const std::string& dbName, const s
             throw UnavailableDataError(message, hash);
         }
         const std::string& value = *pvalue;
-        return value == "None" ? shared_ptr<string>() : make_shared<string>(value);
+        return value == "None" ? std::shared_ptr<std::string>() : std::make_shared<std::string>(value);
     };
-    return blockable<shared_ptr<string>>(innerfunc, dbName, blocking);
+    return blockable<std::shared_ptr<std::string>>(innerfunc, dbName, blocking);
 }
 
 bool DBInterface::hexists(const std::string& dbName, const std::string& hash, const std::string& key)