[ntuple] Prevent late model extension with Streamer Fields

Also add [[nodiscard]] to RResultBase::ForwardError to prevent erroneous
use of R__FORWARD_ERROR

Author: silverweed

core/foundation/inc/ROOT/RError.hxx

@@ -125,6 +125,7 @@ public:
    void Throw();
 
    /// Used by R__FORWARD_ERROR in order to keep track of the stack trace.
+   [[nodiscard]]
    static RError ForwardError(RResultBase &&result, RError::RLocation &&sourceLocation)
    {
       if (!result.fError) {

tree/ntuple/inc/ROOT/RPageStorage.hxx

@@ -320,8 +320,8 @@ public:
       if (fIsInitialized) {
          throw RException(R__FAIL("already initialized"));
       }
-      fIsInitialized = true;
       InitImpl(model);
+      fIsInitialized = true;
    }
 
 protected:

tree/ntuple/src/RNTupleMerger.cxx

@@ -586,8 +586,10 @@ CompareDescriptorStructure(const ROOT::RNTupleDescriptor &dst, const ROOT::RNTup
 }
 
 // Applies late model extension to `destination`, adding all `newFields` to it.
-static void ExtendDestinationModel(std::span<const ROOT::RFieldDescriptor *> newFields, ROOT::RNTupleModel &dstModel,
-                                   RNTupleMergeData &mergeData, std::vector<RCommonField> &commonFields)
+[[nodiscard]]
+static ROOT::RResult<void>
+ExtendDestinationModel(std::span<const ROOT::RFieldDescriptor *> newFields, ROOT::RNTupleModel &dstModel,
+                       RNTupleMergeData &mergeData, std::vector<RCommonField> &commonFields)
 {
    assert(newFields.size() > 0); // no point in calling this with 0 new cols
 
@@ -636,14 +638,20 @@ static void ExtendDestinationModel(std::span<const ROOT::RFieldDescriptor *> new
       ROOT::Internal::GetProjectedFieldsOfModel(dstModel).Add(std::move(field), fieldMap);
    }
    dstModel.Freeze();
-   mergeData.fDestination.UpdateSchema(changeset, mergeData.fNumDstEntries);
+   try {
+      mergeData.fDestination.UpdateSchema(changeset, mergeData.fNumDstEntries);
+   } catch (const ROOT::RException &ex) {
+      return R__FAIL(ex.GetError().GetReport());
+   }
 
    commonFields.reserve(commonFields.size() + newFields.size());
    for (const auto *field : newFields) {
       const auto newFieldInDstId = mergeData.fDstDescriptor.FindFieldId(field->GetFieldName());
       const auto &newFieldInDst = mergeData.fDstDescriptor.GetFieldDescriptor(newFieldInDstId);
       commonFields.emplace_back(*field, newFieldInDst);
    }
+
+   return ROOT::RResult<void>::Success();
 }
 
 // Generates default (zero) values for the given columns
@@ -1184,7 +1192,9 @@ ROOT::RResult<void> RNTupleMerger::Merge(std::span<RPageSource *> sources, const
       if (descCmp.fExtraSrcFields.size()) {
          if (mergeOpts.fMergingMode == ENTupleMergingMode::kUnion) {
             // late model extension for all fExtraSrcFields in Union mode
-            ExtendDestinationModel(descCmp.fExtraSrcFields, *fModel, mergeData, descCmp.fCommonFields);
+            auto res = ExtendDestinationModel(descCmp.fExtraSrcFields, *fModel, mergeData, descCmp.fCommonFields);
+            if (!res)
+               return R__FORWARD_ERROR(res);
          } else if (mergeOpts.fMergingMode == ENTupleMergingMode::kStrict) {
             // If the current source has extra fields and we're in Strict mode, error
             std::string msg = "Source RNTuple has extra fields that the destination RNTuple doesn't have:";

tree/ntuple/src/RPageStorage.cxx

@@ -830,6 +830,11 @@ ROOT::Internal::RPagePersistentSink::AddColumn(ROOT::DescriptorId_t fieldId, RCo
 void ROOT::Internal::RPagePersistentSink::UpdateSchema(const ROOT::Internal::RNTupleModelChangeset &changeset,
                                                        ROOT::NTupleSize_t firstEntry)
 {
+   if (fIsInitialized)
+      for (const auto &field : changeset.fAddedFields)
+         if (field->GetStructure() == ENTupleStructure::kStreamer)
+            throw ROOT::RException(R__FAIL("a Model cannot be extended with Streamer fields"));
+
    const auto &descriptor = fDescriptorBuilder.GetDescriptor();
 
    if (descriptor.GetNLogicalColumns() > descriptor.GetNPhysicalColumns()) {

tree/ntuple/test/ntuple_merger.cxx

@@ -3561,6 +3561,7 @@ TEST(RNTupleMerger, MergeStreamerFields)
 
       // Now merge the inputs
       for (const auto mmode : {ENTupleMergingMode::kFilter, ENTupleMergingMode::kStrict, ENTupleMergingMode::kUnion}) {
+         SCOPED_TRACE(std::string("with merging mode = ") + ToString(mmode));
          FileRaii fileGuardOut("test_ntuple_merge_streamer_out.root");
          {
             auto destination = std::make_unique<RPageSinkFile>("ntuple", fileGuardOut.GetPath(), RNTupleWriteOptions());
@@ -3575,6 +3576,8 @@ TEST(RNTupleMerger, MergeStreamerFields)
          EXPECT_EQ(reader->GetNEntries(), 20);
          auto pFoo = reader->GetModel().GetDefaultEntry().GetPtr<CustomStruct>("foo");
          auto pBar = reader->GetModel().GetDefaultEntry().GetPtr<CustomStruct>("bar");
+         EXPECT_EQ(reader->GetModel().GetConstField("foo").GetStructure(), ROOT::ENTupleStructure::kStreamer);
+         EXPECT_EQ(reader->GetModel().GetConstField("bar").GetStructure(), ROOT::ENTupleStructure::kStreamer);
          for (auto idx : reader->GetEntryRange()) {
             reader->LoadEntry(idx);
             ASSERT_EQ(pFoo->v1.size(), 1);
@@ -3588,7 +3591,7 @@ TEST(RNTupleMerger, MergeStreamerFields)
 
 TEST(RNTupleMerger, MergeStreamerFieldsFirstMissing)
 {
-   // Merge two files where the second has an additional streamer field - should succeed.
+   // Merge two files where the second has an additional streamer field - should fail except in Filter mode.
    FileRaii fileGuard1("test_ntuple_merge_streamer_firstmissing1.root");
    {
       auto model = RNTupleModel::Create();
@@ -3634,14 +3637,15 @@ TEST(RNTupleMerger, MergeStreamerFieldsFirstMissing)
 
       // Now merge the inputs
       for (const auto mmode : {ENTupleMergingMode::kFilter, ENTupleMergingMode::kStrict, ENTupleMergingMode::kUnion}) {
+         SCOPED_TRACE(std::string("with merging mode = ") + ToString(mmode));
          FileRaii fileGuardOut("test_ntuple_merge_streamer_firstmissing_out.root");
          {
             auto destination = std::make_unique<RPageSinkFile>("ntuple", fileGuardOut.GetPath(), RNTupleWriteOptions());
             RNTupleMerger merger{std::move(destination)};
             RNTupleMergeOptions opts;
             opts.fMergingMode = mmode;
             auto res = merger.Merge(sourcePtrs, opts);
-            if (mmode != ENTupleMergingMode::kStrict)
+            if (mmode == ENTupleMergingMode::kFilter)
                EXPECT_TRUE(bool(res));
             else
                EXPECT_FALSE(bool(res));