Fix: DBInterface::get() return nullable strings (#516)

Fix DBInterface::get() should return nullable strings, to align with redis-py behavior.
sonic-net · Aug 5, 2021 · e95a466 · e95a466
1 parent f89b2ac
commit e95a466
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 6 deletions.
diff --git 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" ? std::shared_ptr<std::string>() : std::make_shared<std::string>(value);
     };
-    return blockable<std::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)

diff --git 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,7 +38,7 @@ 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::vector<std::string> keys(const std::string& dbName, const char *pattern = "*", bool blocking = false);

diff --git 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);
 }

diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h
@@ -35,7 +35,7 @@ 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);
 

diff --git 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>
@@ -88,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:

diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py
@@ -203,7 +203,26 @@ def test_DBInterface():
     db.connect("TEST_DB")
     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
+
     fvs = db.get_all("TEST_DB", "key0")
     assert "field1" in fvs
     assert fvs["field1"] == "value2"