MB-35079: Allow mix of multiple virtual xattrs

It should be possible to mix the order of the virtual xattrs
(and request multiple of them)

Change-Id: I2473d8151053c15b6fe1c8f0fef2455657031ca5
Reviewed-on: http://review.couchbase.org/111996
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Trond Norbye <[email protected]>

Author: trondn

daemon/subdocument.cc

@@ -251,7 +251,10 @@ static void create_multi_path_context(SubdocCmdContext& context,
         if (xattr) {
             size_t xattr_keylen;
             is_valid_xattr_key({path.data(), path.size()}, xattr_keylen);
-            context.set_xattr_key({path.data(), xattr_keylen});
+            cb::const_char_buffer key{path.data(), xattr_keylen};
+            if (!cb::xattr::is_vattr(key)) {
+                context.set_xattr_key(key);
+            }
         }
 
         const SubdocCmdContext::Phase phase = xattr ?
@@ -568,7 +571,7 @@ static cb::mcbp::Status subdoc_operate_one_path(
     }
 
     if (context.getCurrentPhase() == SubdocCmdContext::Phase::XATTR &&
-        spec.path.buf[0] == '$') {
+        cb::xattr::is_vattr(spec.path)) {
         if (spec.path.buf[1] == 'd') {
             // This is a call to the "$document" (the validator stopped all of
             // the other ones), so replace the document with
@@ -772,11 +775,9 @@ static bool operate_single_doc(SubdocCmdContext& context,
     return true;
 }
 
-static ENGINE_ERROR_CODE validate_vattr_privilege(SubdocCmdContext& context) {
-    auto key = context.get_xattr_key();
-
+static ENGINE_ERROR_CODE validate_vattr_privilege(
+        SubdocCmdContext& context, const cb::const_char_buffer key) {
     // The $document vattr doesn't require any xattr permissions.
-
     if (key.buf[1] == 'X') {
         // In the xtoc case we want to see which privileges the connection has
         // to determine which XATTRs we tell the user about
@@ -823,15 +824,22 @@ static ENGINE_ERROR_CODE validate_vattr_privilege(SubdocCmdContext& context) {
 }
 
 static ENGINE_ERROR_CODE validate_xattr_privilege(SubdocCmdContext& context) {
+    // Look at all of the operations we've got in there:
+    for (const auto& op :
+         context.getOperations(SubdocCmdContext::Phase::XATTR)) {
+        if (cb::xattr::is_vattr(op.path)) {
+            auto ret = validate_vattr_privilege(context, op.path);
+            if (ret != ENGINE_SUCCESS) {
+                return ret;
+            }
+        }
+    }
+
     auto key = context.get_xattr_key();
     if (key.empty()) {
         return ENGINE_SUCCESS;
     }
 
-    if (cb::xattr::is_vattr(key)) {
-        return validate_vattr_privilege(context);
-    }
-
     cb::rbac::Privilege privilege;
     // We've got an XATTR..
     if (context.traits.is_mutator) {

include/xattr/utils.h

@@ -75,11 +75,12 @@ static inline bool is_system_xattr(cb::const_char_buffer& attr) {
 /**
  * Check if the attribute is a virtual xattr or not
  *
- * @param attr the attribute to check (CAN'T BE EMPTY!)
+ * @param attr the attribute to check
  */
-static inline bool is_vattr(cb::const_char_buffer& attr) {
-    return *attr.data() == '$';
+static inline bool is_vattr(cb::const_char_buffer attr) {
+    return !attr.empty() && *attr.data() == '$';
 }
+
 namespace macros {
 struct macro {
     cb::const_char_buffer name;

tests/testapp/testapp_xattr.cc

@@ -1536,3 +1536,101 @@ TEST_P(XattrTest, MB_28524_TestReplaceWithXattrUncompressed) {
 TEST_P(XattrTest, MB_28524_TestReplaceWithXattrCompressed) {
     doReplaceWithXattrTest(true);
 }
+
+/**
+ * If the client tries to fetch a mix of multiple virtual xattrs it might not
+ * work as expected.  Ex:
+ *
+ * $document
+ * tnx
+ * will work, but
+ *
+ * tnx
+ * $document
+ * will not work. In addition
+ *
+ * $XTOC
+ * tnx
+ * returns "null" for the toc
+ */
+TEST_P(XattrTest, MB35079_virtual_xattr_mix) {
+    // Store the document we want to operate on
+    {
+        BinprotSubdocMultiMutationCommand cmd;
+        cmd.setKey(name);
+        cmd.addMutation(cb::mcbp::ClientOpcode::SubdocDictUpsert,
+                        SUBDOC_FLAG_XATTR_PATH,
+                        "tnx",
+                        "{\"id\":666}");
+        cmd.addMutation(cb::mcbp::ClientOpcode::SubdocDictUpsert,
+                        SUBDOC_FLAG_MKDIR_P,
+                        "value",
+                        R"("value")");
+        auto& conn = getConnection();
+        ASSERT_EQ(cb::mcbp::Status::Success, conn.execute(cmd).getStatus());
+    }
+
+    // Problem 1 : The order of the virtual xattr and key matters:
+    {
+        BinprotSubdocMultiLookupCommand cmd;
+        cmd.setKey(name);
+        cmd.addGet("tnx", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("$document.CAS", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("value", SUBDOC_FLAG_NONE);
+
+        auto& conn = getConnection();
+        BinprotSubdocMultiLookupResponse resp{conn.execute(cmd)};
+        ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
+
+        auto& results = resp.getResults();
+        EXPECT_EQ(cb::mcbp::Status::Success, results[0].status);
+        EXPECT_EQ(R"({"id":666})", results[0].value);
+        EXPECT_EQ(cb::mcbp::Status::Success, results[1].status);
+        // We can't really check the CAS, but it should be a hex value
+        EXPECT_EQ(0, results[1].value.find("\"0x"));
+        EXPECT_EQ(cb::mcbp::Status::Success, results[2].status);
+        EXPECT_EQ(R"("value")", results[2].value);
+    }
+    // Try it with cas first and then the xattr
+    {
+        BinprotSubdocMultiLookupCommand cmd;
+        cmd.setKey(name);
+        cmd.addGet("$document.CAS", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("tnx", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("value", SUBDOC_FLAG_NONE);
+
+        auto& conn = getConnection();
+        BinprotSubdocMultiLookupResponse resp{conn.execute(cmd)};
+        ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
+
+        auto& results = resp.getResults();
+        EXPECT_EQ(cb::mcbp::Status::Success, results[0].status);
+        // We can't really check the CAS, but it should be a hex value
+        EXPECT_EQ(0, results[0].value.find("\"0x"));
+        EXPECT_EQ(cb::mcbp::Status::Success, results[1].status);
+        EXPECT_EQ(R"({"id":666})", results[1].value);
+        EXPECT_EQ(cb::mcbp::Status::Success, results[2].status);
+        EXPECT_EQ(R"("value")", results[2].value);
+    }
+
+    // Problem 2 : Requesting XTOC with another xattr fails to populate XTOC
+    {
+        BinprotSubdocMultiLookupCommand cmd;
+        cmd.setKey(name);
+        cmd.addGet("$XTOC", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("tnx", SUBDOC_FLAG_XATTR_PATH);
+        cmd.addGet("value", SUBDOC_FLAG_NONE);
+
+        auto& conn = getConnection();
+        BinprotSubdocMultiLookupResponse resp{conn.execute(cmd)};
+        ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
+
+        auto& results = resp.getResults();
+        EXPECT_EQ(cb::mcbp::Status::Success, results[0].status);
+        EXPECT_EQ(R"(["tnx"])", results[0].value);
+        EXPECT_EQ(cb::mcbp::Status::Success, results[1].status);
+        EXPECT_EQ(R"({"id":666})", results[1].value);
+        EXPECT_EQ(cb::mcbp::Status::Success, results[2].status);
+        EXPECT_EQ(R"("value")", results[2].value);
+    }
+}