[ntuple] fix auto schema evolution of base classes

Forbid changing the number of base classes except for adding base
classes where there were previously none and for removing all of the
base classes.

Author: jblomer

tree/ntuple/src/RFieldMeta.cxx

@@ -446,6 +446,9 @@ std::unique_ptr<ROOT::RFieldBase> ROOT::RClassField::BeforeConnectPageSource(ROO
    // On-disk members that are not targeted by an I/O rule; all other sub fields of the in-memory class
    // will be marked as artificial (added member in a new class version or member set by rule).
    std::unordered_set<std::string> regularSubfields;
+   // We generally don't support changing the number of base classes, with the exception of changing from/to zero
+   // base classes. The variable stores the number of on-disk base classes.
+   int nOnDiskBaseClasses = 0;
 
    if (GetOnDiskId() == kInvalidDescriptorId) {
       // This can happen for added base classes or added members of class type
@@ -468,6 +471,8 @@ std::unique_ptr<ROOT::RFieldBase> ROOT::RClassField::BeforeConnectPageSource(ROO
       for (auto linkId : fieldDesc.GetLinkIds()) {
          const auto &subFieldDesc = desc.GetFieldDescriptor(linkId);
          regularSubfields.insert(subFieldDesc.GetFieldName());
+         if (!subFieldDesc.GetFieldName().empty() && subFieldDesc.GetFieldName()[0] == ':')
+            nOnDiskBaseClasses++;
       }
 
       rules = FindRules(&fieldDesc);
@@ -510,10 +515,23 @@ std::unique_ptr<ROOT::RFieldBase> ROOT::RClassField::BeforeConnectPageSource(ROO
    }
 
    // Iterate over all sub fields in memory and mark those as missing that are not in the descriptor.
+   int nInMemoryBaseClasses = 0;
    for (auto &field : fSubfields) {
-      if (regularSubfields.count(field->GetFieldName()) == 0) {
+      const auto &fieldName = field->GetFieldName();
+      if (regularSubfields.count(fieldName) == 0) {
          CallSetArtificialOn(*field);
       }
+      if (!fieldName.empty() && fieldName[0] == ':')
+         nInMemoryBaseClasses++;
+   }
+
+   if (nInMemoryBaseClasses != 0 && nOnDiskBaseClasses != 0 && nInMemoryBaseClasses != nOnDiskBaseClasses) {
+      throw RException(R__FAIL(std::string("incompatible number of base classes for field ") + GetFieldName() + ": " +
+                               GetTypeName() + ", " + std::to_string(nInMemoryBaseClasses) +
+                               " base classes in memory "
+                               " vs. " +
+                               std::to_string(nOnDiskBaseClasses) + " base classes on-disk\n" +
+                               Internal::GetTypeTraceReport(*this, pageSource.GetSharedDescriptorGuard().GetRef())));
    }
 
    return nullptr;

tree/ntuple/test/ntuple_evolution_shape.cxx

@@ -593,20 +593,12 @@ struct AddedSecondBaseDerived : public AddedSecondBaseFirst, public AddedSecondB
 )"));
 
    auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());
-   ASSERT_EQ(2, reader->GetNEntries());
-
-   void *ptr = reader->GetModel().GetDefaultEntry().GetPtr<void>("f").get();
-   DeclarePointer("AddedSecondBaseDerived", "ptrAddedSecondBaseDerived", ptr);
-
-   reader->LoadEntry(0);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fFirstBase", 1);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fSecondBase", 2);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fDerived", 3);
-
-   reader->LoadEntry(1);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fFirstBase", 71);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fSecondBase", 2);
-   EXPECT_EVALUATE_EQ("ptrAddedSecondBaseDerived->fDerived", 93);
+   try {
+      reader->GetModel();
+      FAIL() << "model reconstruction should fail";
+   } catch (const ROOT::RException &err) {
+      EXPECT_THAT(err.what(), testing::HasSubstr("incompatible number of base classes"));
+   }
 }
 
 TEST(RNTupleEvolution, PrependSecondBaseClass)
@@ -662,7 +654,7 @@ struct PrependSecondBaseDerived : public PrependSecondBaseSecond, public Prepend
       reader->GetModel();
       FAIL() << "model reconstruction should fail";
    } catch (const ROOT::RException &err) {
-      EXPECT_THAT(err.what(), testing::HasSubstr("incompatible type name for field"));
+      EXPECT_THAT(err.what(), testing::HasSubstr("incompatible number of base classes"));
    }
 }
 
@@ -844,6 +836,61 @@ struct RemovedIntermediateDerived : public RemovedIntermediateBase {
    }
 }
 
+TEST(RNTupleEvolution, RemovedSecondBaseClass)
+{
+   FileRaii fileGuard("test_ntuple_evolution_removed_second_base_class.root");
+
+   ExecInFork([&] {
+      // The child process writes the file and exits, but the file must be preserved to be read by the parent.
+      fileGuard.PreserveFile();
+
+      ASSERT_TRUE(gInterpreter->Declare(R"(
+struct RemovedSecondBaseFirst {
+   int fFirstBase = 1;
+};
+struct RemovedSecondBaseSecond {
+   int fSecondBase = 2;
+};
+struct RemovedSecondBaseDerived : public RemovedSecondBaseFirst, public RemovedSecondBaseSecond {
+   int fDerived = 3;
+};
+)"));
+
+      auto model = RNTupleModel::Create();
+      model->AddField(RFieldBase::Create("f", "RemovedSecondBaseDerived").Unwrap());
+
+      auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());
+      writer->Fill();
+
+      // Reset / close the writer and flush the file.
+      {
+         // TStreamerInfo::Build will report a warning for interpreted classes (but only for base classes).
+         // See also https://github.com/root-project/root/issues/9371
+         ROOT::TestSupport::CheckDiagsRAII diagRAII;
+         diagRAII.optionalDiag(kWarning, "TStreamerInfo::Build", "has no streamer or dictionary",
+                               /*matchFullMessage=*/false);
+         writer.reset();
+      }
+   });
+
+   ASSERT_TRUE(gInterpreter->Declare(R"(
+struct RemovedSecondBaseFirst {
+   int fFirstBase = 1;
+};
+struct RemovedSecondBaseDerived : public RemovedSecondBaseFirst {
+   int fDerived = 3;
+};
+)"));
+
+   auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());
+   try {
+      reader->GetModel();
+      FAIL() << "model reconstruction should fail";
+   } catch (const ROOT::RException &err) {
+      EXPECT_THAT(err.what(), testing::HasSubstr("incompatible number of base classes"));
+   }
+}
+
 TEST(RNTupleEvolution, RenamedBaseClass)
 {
    // RNTuple currently does not support automatic schema evolution when a class is renamed.