[#874] Grant the offline tools a read-only JDBC transaction instead of refu...

Valery Kharseko

2 days ago 97dbf50dd56ee7640411ac61e6aa288f208a4375

 opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

@@ -1043,6 +1043,16 @@
         }
      }
   }
   /**
    * A transaction able to write, unless the storage was opened read-only: then it may open an existing tree and
    * read it, and every mutating operation throws {@link ReadOnlyStorageException} instead.
    * <p>
    * The mode is checked per operation rather than refused here, because {@code RootContainer.open(AccessMode)}
    * asks for a write transaction even in read-only mode - that is where it opens the compressed schema and the
    * entry containers - so refusing to hand one out failed the offline {@code export-ldif}, {@code verify-index}
    * and {@code backendstat} before they read anything (#874). Both other storages of this server already have
    * this shape: {@code PDBStorage.ReadOnlyStorageImpl} and {@code CASStorage.TransactionImpl.checkReadOnly()}.
    */
   private final class WriteableTransactionTransactionImpl extends ReadableTransactionImpl implements WriteableTransaction {

      // Shared by every table this transaction stamps: opening a backend opens all its trees,
@@ -1052,10 +1062,17 @@

      public WriteableTransactionTransactionImpl(Connection con) {
         super(con);
         if (!accessMode.isWriteable()) {
         //captured once rather than read per operation: the access mode of the storage is mutable state -
         //ImporterImpl reopens the storage READ_WRITE under its caller - and a transaction has to keep the mode
         //it was created with. It also drives isReadOnly, so that a cursor this transaction opens refuses
         //delete() as well.
         isReadOnly = !accessMode.isWriteable();
      }

      void checkReadOnly() {
         if (isReadOnly) {
            throw new ReadOnlyStorageException();
         }
         isReadOnly = false;
      }

      boolean isExistsTable(TreeName treeName) {
@@ -1096,6 +1113,7 @@
      @Override
      public void openTree(TreeName treeName, boolean createOnDemand) {
         if (createOnDemand) {
            checkReadOnly();
            if (!isExistsTable(treeName)) {
               try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")){
                  execute(statement);
@@ -1158,6 +1176,7 @@
      }
      
      public void clearTree(TreeName treeName) {
         checkReadOnly();
         try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName))){
            execute(statement);
            con.commit();
@@ -1168,6 +1187,7 @@

      @Override
      public void deleteTree(TreeName treeName) {
         checkReadOnly();
         if (isExistsTable(treeName)) {
            try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) {
               execute(statement);
@@ -1183,6 +1203,7 @@

      @Override
      public void put(TreeName treeName, ByteSequence key, ByteSequence value) {
         checkReadOnly();
         try {
            upsert(treeName, key, value);
         } catch (SQLException e) {
@@ -1250,6 +1271,9 @@

      @Override
      public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) {
         //checked before the read, so that a read-only transaction reports the mode rather than the value it
         //computed being equal to the stored one
         checkReadOnly();
         final ByteString oldValue=read(treeName,key);
         final ByteSequence newValue=f.computeNewValue(oldValue);
         if (Objects.equals(newValue, oldValue))
@@ -1266,6 +1290,7 @@

      @Override
      public boolean delete(TreeName treeName, ByteSequence key) {
         checkReadOnly();
         try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName)+" where h="+hashParam(con)+" and k=?")){
            statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray())));
            statement.setBytes(2,real2db(key.toByteArray()));

 opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java

@@ -105,6 +105,7 @@
  /** The storage associated with this index. */
  private final Storage storage;
  private final State state;
  private final EntryContainer entryContainer;

  /**
   * A flag to indicate if this vlvIndex should be trusted to be consistent with the entries tree.
@@ -131,15 +132,8 @@
    }

    this.state = state;
    this.entryContainer = entryContainer;
    this.trusted = state.getIndexFlags(txn, getName()).contains(IndexFlag.TRUSTED);
    if (!trusted && entryContainer.getHighestEntryID(txn).longValue() == 0)
    {
      /*
       * If there are no entries in the entry container then there is no reason why this vlvIndex
       * can't be upgraded to trusted.
       */
      setTrusted(txn, true);
    }

    this.config.addChangeListener(this);
  }
@@ -163,6 +157,19 @@
  void afterOpen(final WriteableTransaction txn, boolean createOnDemand) throws StorageRuntimeException
  {
    counter.open(txn, createOnDemand);
    if (createOnDemand && !trusted && entryContainer.isEmpty(txn))
    {
      /*
       * If there are no entries in the entry container then there is no reason why this vlvIndex
       * can't be upgraded to trusted.
       *
       * Guarded by createOnDemand - which is accessMode.isWriteable() - and done here rather than in the
       * constructor, as DefaultIndex.afterOpen() does: the transaction a read-only container opens is not
       * allowed to write, so upgrading an untrusted index of an empty backend used to fail the offline tools
       * on it instead of leaving the flag alone (#874).
       */
      setTrusted(txn, true);
    }
  }

  @Override

 opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java

@@ -22,6 +22,7 @@
import org.opends.server.backends.pluggable.spi.AccessMode;
import org.opends.server.backends.pluggable.spi.Cursor;
import org.opends.server.backends.pluggable.spi.Importer;
import org.opends.server.backends.pluggable.spi.ReadOnlyStorageException;
import org.opends.server.backends.pluggable.spi.ReadOperation;
import org.opends.server.backends.pluggable.spi.ReadableTransaction;
import org.opends.server.backends.pluggable.spi.TreeName;
@@ -320,6 +321,86 @@
      }
   }

   /**
    * A storage opened READ_ONLY must still hand out the write transaction {@code RootContainer.open()} asks for
    * there - otherwise the offline export-ldif, verify-index and backendstat fail before reading anything - and
    * that transaction must serve exactly what the open needs and nothing more: opening an existing tree, reads,
    * cursors and record counts, while every mutation, including a delete through a cursor it opened, is
    * refused (#874).
    */
   @Test
   public void testReadOnlyTransactionReadsButRefusesWrites() throws Exception {
      final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null);
      final TreeName tree = new TreeName("testReadOnlyTransaction", "tree");
      final TreeName absent = new TreeName("testReadOnlyTransaction", "absent");
      try {
         storage.open(AccessMode.READ_WRITE);
         storage.write(new WriteOperation() {
            @Override
            public void run(WriteableTransaction txn) throws Exception {
               txn.openTree(tree, true);
               txn.put(tree, key(0), value(0));
               txn.put(tree, key(1), value(1));
            }
         });
         storage.close();

         storage.open(AccessMode.READ_ONLY);
         storage.write(new WriteOperation() {
            @Override
            public void run(WriteableTransaction txn) throws Exception {
               // what RootContainer.open() does through this transaction in read-only mode
               txn.openTree(tree, false);
               assertEquals(txn.read(tree, key(0)), value(0));
               assertEquals(txn.getRecordCount(tree), 2);
               try (final Cursor<ByteString, ByteString> cursor = txn.openCursor(tree)) {
                  assertTrue(cursor.next());
                  assertEquals(cursor.getKey(), key(0));
                  try {
                     cursor.delete();
                     fail("delete() through a cursor of a read-only transaction must fail");
                  } catch (UnsupportedOperationException expected) {}
               }

               assertReadOnly("openTree(createOnDemand)", () -> txn.openTree(absent, true));
               assertReadOnly("put", () -> txn.put(tree, key(2), value(2)));
               assertReadOnly("update", () -> txn.update(tree, key(0), old -> value(3)));
               assertReadOnly("delete", () -> txn.delete(tree, key(0)));
               assertReadOnly("deleteTree", () -> txn.deleteTree(tree));
            }
         });

         // nothing above reached the database
         storage.close();
         storage.open(AccessMode.READ_WRITE);
         storage.read(new ReadOperation<Void>() {
            @Override
            public Void run(ReadableTransaction txn) throws Exception {
               assertEquals(txn.getRecordCount(tree), 2);
               assertEquals(txn.read(tree, key(0)), value(0));
               return null;
            }
         });
      } finally {
         try {
            storage.write(new WriteOperation() {
               @Override
               public void run(WriteableTransaction txn) throws Exception {
                  txn.deleteTree(tree);
               }
            });
         } catch (Exception ignored) {}
         storage.close();
      }
   }

   private static void assertReadOnly(String operation, Runnable mutation) {
      try {
         mutation.run();
         fail(operation + " must fail on a read-only storage");
      } catch (ReadOnlyStorageException expected) {}
   }

   /** Buffer-served repositioning relies on the database collating keys in unsigned byte order. */
   @Test
   public void testCursorKeyOrderIsUnsigned() throws Exception {

 opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java

@@ -1251,6 +1251,40 @@
    }
  }

  /**
   * export-ldif, verify-index and backendstat open a root container of their own, in READ_ONLY mode, when the
   * backend is not already open - a stopped server or a disabled backend. RootContainer.open() asks the storage
   * for a write transaction even in that mode, since that is where it opens the compressed schema and the entry
   * containers, so a storage refusing to hand one out fails the three tools before they read anything (#874).
   * <p>
   * testReadOnly() above does not cover this: it expects a ReadOnlyStorageException and a storage that fails the
   * open throws one too, from RootContainer.open() rather than from the write it is meant to be checking.
   */
  @Test
  public void testOfflineToolsOpenBackendReadOnly() throws Exception
  {
    // Put the backend offline, so that the tools open a read-only root container of their own
    backend.finalizeBackend();
    try
    {
      final ByteArrayOutputStream exported = new ByteArrayOutputStream();
      try (final LDIFExportConfig exportConfig = new LDIFExportConfig(exported))
      {
        backend.exportLDIF(exportConfig);
      }
      assertThat(exported.toString(StandardCharsets.UTF_8.name())).contains(testBaseDN.toString());

      final VerifyConfig verifyConfig = new VerifyConfig();
      verifyConfig.setBaseDN(testBaseDN);
      verifyConfig.addCompleteIndex("dn2id");
      assertThat(backend.verifyBackend(verifyConfig)).isEqualTo(0);
    }
    finally
    {
      backend.openBackend();
    }
  }

  @Test
  public void test_issue_496() throws Exception {
    int resultCode = TestCaseUtils.applyModifications(true,