From 371675daf34342ccff99b45524a0d789acd9d89c Mon Sep 17 00:00:00 2001 From: JasMehta08 Date: Sat, 18 Jul 2026 11:38:22 +0530 Subject: [PATCH 1/2] [ntuple] Add checksum to S3 JSON anchor --- tree/ntuple/inc/ROOT/RPageStorageS3.hxx | 5 ++ tree/ntuple/src/RPageStorageS3.cxx | 50 +++++++++++++-- tree/ntuple/test/ntuple_storage_s3.cxx | 85 +++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 4 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorageS3.hxx b/tree/ntuple/inc/ROOT/RPageStorageS3.hxx index 84eb4f11df0d9..4189fa80c9081 100644 --- a/tree/ntuple/inc/ROOT/RPageStorageS3.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorageS3.hxx @@ -70,6 +70,11 @@ struct RNTupleAnchorS3 { std::string ToJSON() const; /// Deserialize the anchor from a JSON string. Returns an error on malformed or incompatible input. static RResult CreateFromJSON(const std::string &json); + +private: + /// Serialize all data fields into a canonical compact JSON string with sorted keys. + /// Used by both ToJSON and CreateFromJSON to compute/verify the checksum. + static std::string CanonicalPayload(const RNTupleAnchorS3 &anchor); }; /// \brief Translate an ntpl+s3 URI into its plain HTTP(S) equivalent. diff --git a/tree/ntuple/src/RPageStorageS3.cxx b/tree/ntuple/src/RPageStorageS3.cxx index 763516c3df5ae..009b52f283637 100644 --- a/tree/ntuple/src/RPageStorageS3.cxx +++ b/tree/ntuple/src/RPageStorageS3.cxx @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -31,8 +32,29 @@ using ROOT::Internal::MakeUninitArray; using ROOT::Internal::RNTupleCompressor; -/// Field-by-field equality check across all 14 anchor members. -/// Used to verify round-trip correctness in tests. +/// Serialize all anchor data fields into a compact JSON string with sorted keys. Returns the canonical form +/// used by ToJSON and CreateFromJSON to compute and verify the XXH3 checksum. +std::string ROOT::Experimental::Internal::RNTupleAnchorS3::CanonicalPayload(const RNTupleAnchorS3 &anchor) +{ + nlohmann::json payload; + payload["anchorVersion"] = anchor.fVersionAnchor; + payload["footerObjId"] = anchor.fFooterObjId; + payload["footerOffset"] = anchor.fFooterOffset; + payload["formatVersionEpoch"] = anchor.fVersionEpoch; + payload["formatVersionMajor"] = anchor.fVersionMajor; + payload["formatVersionMinor"] = anchor.fVersionMinor; + payload["formatVersionPatch"] = anchor.fVersionPatch; + payload["headerObjId"] = anchor.fHeaderObjId; + payload["headerOffset"] = anchor.fHeaderOffset; + payload["lenFooter"] = anchor.fLenFooter; + payload["lenHeader"] = anchor.fLenHeader; + payload["nBytesFooter"] = anchor.fNBytesFooter; + payload["nBytesHeader"] = anchor.fNBytesHeader; + payload["urlTemplate"] = anchor.fUrlTemplate; + return payload.dump(-1); +} + +/// Field-by-field equality check across all data members. bool ROOT::Experimental::Internal::RNTupleAnchorS3::operator==(const RNTupleAnchorS3 &other) const { return fVersionAnchor == other.fVersionAnchor && fVersionEpoch == other.fVersionEpoch && @@ -45,10 +67,13 @@ bool ROOT::Experimental::Internal::RNTupleAnchorS3::operator==(const RNTupleAnch } /// Serialize the anchor to a pretty-printed JSON string (2-space indent). -/// nlohmann/json handles type conversion, string escaping, and uint64 precision. -/// The output is suitable for direct upload to S3 as the anchor object. +/// The checksum is computed over the compact canonical form produced by CanonicalPayload; +/// the stored JSON uses pretty-printing for readability. std::string ROOT::Experimental::Internal::RNTupleAnchorS3::ToJSON() const { + auto canonical = CanonicalPayload(*this); + auto checksum = XXH3_64bits(canonical.data(), canonical.size()); + nlohmann::json jsonAnchor; jsonAnchor["anchorVersion"] = fVersionAnchor; jsonAnchor["formatVersionEpoch"] = fVersionEpoch; @@ -64,6 +89,7 @@ std::string ROOT::Experimental::Internal::RNTupleAnchorS3::ToJSON() const jsonAnchor["footerOffset"] = fFooterOffset; jsonAnchor["nBytesFooter"] = fNBytesFooter; jsonAnchor["lenFooter"] = fLenFooter; + jsonAnchor["checksum"] = checksum; return jsonAnchor.dump(2); } @@ -110,6 +136,22 @@ ROOT::Experimental::Internal::RNTupleAnchorS3::CreateFromJSON(const std::string return R__FAIL("missing or invalid field in S3 anchor: " + std::string(e.what())); } + if (!jsonAnchor.contains("checksum")) + return R__FAIL("missing 'checksum' field in S3 anchor"); + + std::uint64_t storedChecksum; + try { + storedChecksum = jsonAnchor.at("checksum").get(); + } catch (const nlohmann::json::exception &e) { + return R__FAIL("invalid 'checksum' field in S3 anchor: " + std::string(e.what())); + } + + auto canonical = CanonicalPayload(anchor); + auto computedChecksum = XXH3_64bits(canonical.data(), canonical.size()); + + if (storedChecksum != computedChecksum) + return R__FAIL("S3 anchor checksum mismatch"); + return anchor; } diff --git a/tree/ntuple/test/ntuple_storage_s3.cxx b/tree/ntuple/test/ntuple_storage_s3.cxx index a7359f0764138..c1d2f577d80e4 100644 --- a/tree/ntuple/test/ntuple_storage_s3.cxx +++ b/tree/ntuple/test/ntuple_storage_s3.cxx @@ -368,6 +368,90 @@ TEST(RNTupleAnchorS3, MaxUint64Values) EXPECT_EQ(UINT64_MAX, parsed.fLenFooter); } +// ==================== Checksum Tests ==================== + +TEST(RNTupleAnchorS3, ChecksumMismatch) +{ + RNTupleAnchorS3 anchor; + anchor.fHeaderObjId = 42; + anchor.fNBytesHeader = 100; + anchor.fLenHeader = 200; + + auto json = anchor.ToJSON(); + + // Corrupt a data field while keeping the old checksum + auto pos = json.find("\"nBytesHeader\": 100"); + ASSERT_NE(std::string::npos, pos); + json.replace(pos, std::strlen("\"nBytesHeader\": 100"), "\"nBytesHeader\": 999"); + + auto result = RNTupleAnchorS3::CreateFromJSON(json); + EXPECT_FALSE(bool(result)); +} + +TEST(RNTupleAnchorS3, MissingChecksumRejected) +{ + // An anchor without a checksum field must be rejected. + std::string json = R"({ + "anchorVersion": 0, + "formatVersionEpoch": 0, + "formatVersionMajor": 1, + "formatVersionMinor": 0, + "formatVersionPatch": 0, + "urlTemplate": "${baseurl}/${objid}", + "headerObjId": 0, + "headerOffset": 0, + "nBytesHeader": 0, + "lenHeader": 0, + "footerObjId": 0, + "footerOffset": 0, + "nBytesFooter": 0, + "lenFooter": 0 + })"; + + auto result = RNTupleAnchorS3::CreateFromJSON(json); + EXPECT_FALSE(bool(result)); +} + +TEST(RNTupleAnchorS3, ChecksumDeterministic) +{ + RNTupleAnchorS3 anchor; + anchor.fHeaderObjId = 1; + anchor.fFooterObjId = 5; + anchor.fNBytesHeader = 80; + anchor.fLenHeader = 100; + anchor.fNBytesFooter = 150; + anchor.fLenFooter = 200; + + auto json1 = anchor.ToJSON(); + auto json2 = anchor.ToJSON(); + EXPECT_EQ(json1, json2) << "ToJSON must produce identical output for the same data"; + + auto result = RNTupleAnchorS3::CreateFromJSON(json1); + ASSERT_TRUE(bool(result)) << result.GetError()->GetReport(); + auto json3 = result.Inspect().ToJSON(); + EXPECT_EQ(json1, json3); +} + +TEST(RNTupleAnchorS3, WrongChecksumType) +{ + RNTupleAnchorS3 anchor; + auto json = anchor.ToJSON(); + + // Replace the numeric checksum value with a string + auto pos = json.find("\"checksum\":"); + ASSERT_NE(std::string::npos, pos); + auto valStart = json.find(':', pos) + 1; + while (valStart < json.size() && json[valStart] == ' ') + ++valStart; + auto valEnd = valStart; + while (valEnd < json.size() && json[valEnd] != '\n' && json[valEnd] != ',') + ++valEnd; + json.replace(valStart, valEnd - valStart, " \"not_a_number\""); + + auto result = RNTupleAnchorS3::CreateFromJSON(json); + EXPECT_FALSE(bool(result)); +} + // ==================== ParseS3Url Tests ==================== using ROOT::Experimental::Internal::ParseS3Url; @@ -576,6 +660,7 @@ TEST(RPageSinkS3Wire, WriteIssuesExpectedPuts) // The anchor body is the JSON document the reader bootstraps from. EXPECT_NE(std::string::npos, requests.back().fBody.find("\"footerObjId\"")); EXPECT_NE(std::string::npos, requests.back().fBody.find("\"urlTemplate\"")); + EXPECT_NE(std::string::npos, requests.back().fBody.find("\"checksum\"")); } TEST(RPageSinkS3Wire, PutErrorThrows) From 5945e7236a0c9389469a9ff12d9b303b46e2e15b Mon Sep 17 00:00:00 2001 From: JasMehta08 Date: Sat, 18 Jul 2026 11:38:45 +0530 Subject: [PATCH 2/2] [ntuple] Replace hardcoded clone path with file-local constant --- tree/ntuple/src/RPageStorageS3.cxx | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tree/ntuple/src/RPageStorageS3.cxx b/tree/ntuple/src/RPageStorageS3.cxx index 009b52f283637..2f2a9bbd5cdc4 100644 --- a/tree/ntuple/src/RPageStorageS3.cxx +++ b/tree/ntuple/src/RPageStorageS3.cxx @@ -32,6 +32,11 @@ using ROOT::Internal::MakeUninitArray; using ROOT::Internal::RNTupleCompressor; +namespace { +/// Default URL pattern for hidden (attribute-set) ntuples created by CloneAsHidden. +const std::string kDefaultCloneTemplate = "${baseurl}/_clone/${name}"; +} // anonymous namespace + /// Serialize all anchor data fields into a compact JSON string with sorted keys. Returns the canonical form /// used by ToJSON and CreateFromJSON to compute and verify the XXH3 checksum. std::string ROOT::Experimental::Internal::RNTupleAnchorS3::CanonicalPayload(const RNTupleAnchorS3 &anchor) @@ -342,8 +347,17 @@ std::unique_ptr ROOT::Experimental::Internal::RPageSinkS3::CloneAsHidden(std::string_view name, const ROOT::RNTupleWriteOptions &opts) const { - // The hidden (attribute-set) ntuple is stored under a reserved "_clone" sub-prefix so its objects and - // anchor can never collide with the main ntuple's numeric object keys ($baseurl/0, $baseurl/1, ...). - std::string cloneBaseUrl = fBaseUrl + "/_clone/" + std::string(name); + // Resolve the default clone template so the hidden ntuple's objects and anchor live under a reserved + // sub-prefix that cannot collide with the main ntuple's numeric object keys. + std::string cloneBaseUrl = kDefaultCloneTemplate; + + auto pos = cloneBaseUrl.find("${baseurl}"); + if (pos != std::string::npos) + cloneBaseUrl.replace(pos, std::strlen("${baseurl}"), fBaseUrl); + + pos = cloneBaseUrl.find("${name}"); + if (pos != std::string::npos) + cloneBaseUrl.replace(pos, std::strlen("${name}"), name); + return std::unique_ptr(new RPageSinkS3(name, cloneBaseUrl, opts, RFromBaseUrl{})); }