MB-70044: Add MutateWithMeta command by jimwwalker · Pull Request #2 · jimwwalker/kv_engine

jimwwalker · 2026-03-05T13:20:43Z

The MutateWithMeta command allows a user to store an item and have parts of the xattr segment being replaced with the actual value for CAS and seqno (just like subdoc allows for).

Subdoc cannot be used for this as there is a max number (it is however configurable) of paths one may operate on in the "multi" versions, but there is no upper limit on the number of xattr paths a document may have (which means that XDCR could encounter a document with more paths than the max configured subdoc path limit).

The command:

Must be negotiated via HELLO (feature MutateWithMeta)
Supports Add, Set, and Delete operations
Uses JSON-encoded metadata appended to the value (length in 4-byte extras)
Allows specifying byte offsets where CAS/seqno values should be written
Supports the same options as existing WithMeta commands

Change-Id: I47d70a1e7bb754eedde7a19ab3980eadccfa23f0

The MutateWithMeta command allows a user to store an item and have parts of the xattr segment being replaced with the actual value for CAS and seqno (just like subdoc allows for). Subdoc cannot be used for this as there is a max number (it is however configurable) of paths one may operate on in the "multi" versions, but there is no upper limit on the number of xattr paths a document may have (which means that XDCR could encounter a document with more paths than the max configured subdoc path limit). The command: - Must be negotiated via HELLO (feature MutateWithMeta) - Supports Add, Set, and Delete operations - Uses JSON-encoded metadata appended to the value (length in 4-byte extras) - Allows specifying byte offsets where CAS/seqno values should be written - Supports the same options as existing WithMeta commands Change-Id: I47d70a1e7bb754eedde7a19ab3980eadccfa23f0

Copilot

Pull request overview

This PR introduces a new MutateWithMeta command (opcode 0x4b) that allows storing items with metadata specified in JSON format, and supports CAS/seqno macro expansion by writing actual values at specified byte offsets within the xattr section. This is motivated by XDCR scenarios where subdoc's path limit could be exceeded.

Changes:

Adds the MutateWithMeta command with JSON-encoded metadata, supporting Add/Set/Delete operations with optional CAS and seqno macro expansion at specified offsets
Enables pre-link document processing in VBucket::setWithMeta to support the macro expansion feature
Adds comprehensive tests (unit tests for payload serialization, integration tests for the full command flow including error cases)

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`include/mcbp/protocol/opcode.h`	Adds `MutateWithMeta = 0x4b` opcode
`include/mcbp/protocol/feature.h`	Adds `MutateWithMeta = 0x25` feature flag
`include/mcbp/codec/mutate_with_meta_payload.h`	Defines `MutateWithMetaPayload` struct and JSON serialization
`include/mcbp/codec/with_meta_options.h`	Makes `encode()` const-correct
`include/xattr/blob.h`	Adds `has_user_keys()` helper
`include/memcached/engine.h`	Moves `set_with_meta`/`delete_with_meta` default impls out of header
`engines/utilities/engine.cc`	Provides default implementations for `set_with_meta`/`delete_with_meta`
`engines/ep/src/vbucket.cc`	Adds `PreLinkDocumentContext` to `setWithMeta` path
`protocol/mcbp/mutate_with_meta_payload.cc`	JSON serialization/deserialization for the payload
`protocol/mcbp/opcode.cc`	Registers opcode attributes
`protocol/mcbp/feature.cc`	Adds feature string formatting
`protocol/mcbp/feature_test.cc`	Adds feature to test blueprint
`protocol/mcbp/with_meta_options.cc`	Const-correctness fix for `encode()`
`protocol/mcbp/request.cc`	Adds opcode to `to_json` switch
`protocol/connection/client_connection.h`	Adds `mutateWithMeta` client method
`protocol/connection/client_connection.cc`	Implements `mutateWithMeta` client method
`protocol/connection/client_mcbp_commands.h`	Declares `BinprotMutateWithMetaCommand`
`protocol/connection/client_mcbp_commands.cc`	Implements command encoding
`daemon/protocol/mcbp/mutate_with_meta_context.h`	State machine header for the command
`daemon/protocol/mcbp/mutate_with_meta_context.cc`	State machine implementation with validation and store logic
`daemon/protocol/mcbp/hello_packet_executor.cc`	Registers feature in HELLO handling
`daemon/mcbp_validators.cc`	Adds validator for the new opcode
`daemon/mcbp_privileges.cc`	Registers privilege check
`daemon/mcbp_executors.cc`	Registers executor
`daemon/datatype_filter.cc`	Adds feature to non-datatype list
`utilities/json_utilities.h`	Adds `getValueFromJson` template helper
`tests/testapp/testapp.cc`	Enables feature in test connections
`tests/testapp/testapp_withmeta.cc`	Comprehensive integration tests
`tests/testapp_cluster/mutate_with_meta.cc`	Cluster-level integration tests
`protocol/mcbp/mutate_with_meta_payload_test.cc`	Unit tests for payload serialization
Various `CMakeLists.txt`	Adds new files to build

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T13:28:43Z

+ *     supported by the Add/Set/DeleteWithMeta commands and the motivation
+ *     for creating the new command.
+ *
+ *.For both CAS and sequence number replacement, the client provides offsets


Formatting issue: *.For should be * For (missing space after * and the period seems accidental).

Suggested change

*.For both CAS and sequence number replacement, the client provides offsets

* For both CAS and sequence number replacement, the client provides offsets

Copilot · 2026-03-05T13:28:43Z

            nullptr /* No pre link step needed */,
            {} /*overwritingPrepareSeqno*/,
            cHandle.getCanDeduplicate(),
            enforceMemCheck};
+    PreLinkDocumentContext preLinkDocumentContext(engine, cookie, &itm);
+    queueItmCtx.preLinkDocumentContext = &preLinkDocumentContext;


The comment /* No pre link step needed */ on line 2252 is now stale/misleading since lines 2256-2257 immediately override the nullptr with a valid PreLinkDocumentContext. The comment should be updated or removed. Alternatively, pass &preLinkDocumentContext directly in the constructor instead of first setting nullptr and then overriding.

Copilot · 2026-03-05T13:28:43Z

+        if (message.starts_with(prefix)) {
+            message = message.substr(prefix.size());
+        } else {
+            message = "Failed to parse extended attributes as JSON";


The error message "Failed to parse extended attributes as JSON" is misleading. The code is parsing the metadata payload (command, options, offsets, etc.), not extended attributes (xattrs). It should say something like "Failed to parse metadata as JSON".

Suggested change

message = "Failed to parse extended attributes as JSON";

message = "Failed to parse metadata as JSON";

Copilot · 2026-03-05T13:28:44Z

+
+    if (!cb::mcbp::datatype::is_xattr(datatype)) {
+        cookie.setErrorContext(
+                "Cannot use pattern macros documents without xattr");


Grammar issue: "Cannot use pattern macros documents without xattr" is missing the preposition "on". Should be "Cannot use pattern macros on documents without xattr".

Suggested change

"Cannot use pattern macros documents without xattr");

"Cannot use pattern macros on documents without xattr");

Copilot · 2026-03-05T13:28:44Z

+    try {
+        MutateWithMetaPayload payload = json;
+    } catch (const std::exception& e) {
+        EXPECT_STREQ(
+                e.what(),
+                "cb::getValueFromJson(): Missing required field: expiration");
+    }
+}


This test will silently pass if MutateWithMetaPayload payload = json; does not throw an exception. There is no FAIL() after the statement that is expected to throw. The test should add a FAIL() << "Expected exception for missing required field"; after line 187 (before the catch block) to properly verify the exception is thrown, similar to how other tests in this file handle it (e.g., MutateWithMetaCommandTest::to_json_invalid at line 34-39).

Copilot · 2026-03-05T13:28:44Z

+            const cb::mcbp::request::MutateWithMetaPayload& payload);
+
+    /// Validate that the if a pattern is provided the document isn't
+    /// compressed and contains an XATTR sectiion


Typo: "sectiion" should be "section".

Suggested change

/// compressed and contains an XATTR sectiion

/// compressed and contains an XATTR section

Copilot · 2026-03-05T13:28:45Z

+        if (cas_offset + cas_hex.size() <= view.size()) {
+            // This should be done as part of the validation step
+            Expects(cas_offset + cas_hex.size() <= view.size());
+            std::copy_n(
+                    cas_hex.data(), cas_hex.size(), view.data() + cas_offset);
+        }
+    }
+
+    // Write seqno value at all specified offsets
+    const auto seqno_hex = cb::to_hex(info.seqno);
+    for (auto seqno_offset : seqno_offsets) {
+        if (seqno_offset + seqno_hex.size() <= view.size()) {
+            // This should be done as part of the validation step
+            Expects(seqno_offset + seqno_hex.size() <= view.size());
+            std::copy_n(seqno_hex.data(),
+                        seqno_hex.size(),
+                        view.data() + seqno_offset);
+        }


The Expects assertion on line 67 is unreachable as a failing assertion because it's guarded by the same condition on line 65 (if (cas_offset + cas_hex.size() <= view.size())). The Expects will always be true when reached. This makes the assertion useless.

More importantly, if an offset is out of bounds, the write is silently skipped (the if guard prevents the write). Since validation should already ensure offsets are valid (as the comment says), a cleaner approach would be to remove the if guard and keep only the Expects, or simply assert/write unconditionally since the validation step guarantees bounds. The same issue applies to the seqno loop at lines 76-78.

Suggested change

if (cas_offset + cas_hex.size() <= view.size()) {

// This should be done as part of the validation step

Expects(cas_offset + cas_hex.size() <= view.size());

std::copy_n(

cas_hex.data(), cas_hex.size(), view.data() + cas_offset);

}

}

// Write seqno value at all specified offsets

const auto seqno_hex = cb::to_hex(info.seqno);

for (auto seqno_offset : seqno_offsets) {

if (seqno_offset + seqno_hex.size() <= view.size()) {

// This should be done as part of the validation step

Expects(seqno_offset + seqno_hex.size() <= view.size());

std::copy_n(seqno_hex.data(),

seqno_hex.size(),

view.data() + seqno_offset);

}

// This should be done as part of the validation step

Expects(cas_offset + cas_hex.size() <= view.size());

std::copy_n(

cas_hex.data(), cas_hex.size(), view.data() + cas_offset);

}

// Write seqno value at all specified offsets

const auto seqno_hex = cb::to_hex(info.seqno);

for (auto seqno_offset : seqno_offsets) {

// This should be done as part of the validation step

Expects(seqno_offset + seqno_hex.size() <= view.size());

std::copy_n(seqno_hex.data(),

seqno_hex.size(),

view.data() + seqno_offset);

Copilot · 2026-03-05T13:28:45Z

+        if (value.starts_with("0x")) {
+            return cb::from_hex(value);
+        }


The from_hex path does not validate that the parsed value fits in T. cb::from_hex() returns uint64_t, but when T is uint32_t (as used in from_json for flags, expiration, and options), a hex string like "0x1ffffffff" would be silently truncated. The from_chars path (for decimal strings) correctly detects std::errc::result_out_of_range, but this hex path lacks equivalent overflow checking. Consider adding a range check after from_hex when T is narrower than uint64_t, and throwing std::out_of_range if the value doesn't fit.

jimwwalker requested a review from Copilot March 5, 2026 13:20

Copilot started reviewing on behalf of jimwwalker March 5, 2026 13:21 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MB-70044: Add MutateWithMeta command#2

MB-70044: Add MutateWithMeta command#2
jimwwalker wants to merge 1 commit into
masterfrom
MutateWithMetaTest

jimwwalker commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	*.For both CAS and sequence number replacement, the client provides offsets
	* For both CAS and sequence number replacement, the client provides offsets

	message = "Failed to parse extended attributes as JSON";
	message = "Failed to parse metadata as JSON";

	"Cannot use pattern macros documents without xattr");
	"Cannot use pattern macros on documents without xattr");

	/// compressed and contains an XATTR sectiion
	/// compressed and contains an XATTR section

Conversation

jimwwalker commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants