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

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

@@ -1043,6 +1043,16 @@ public long getRecordCount(TreeName treeName) {
 			}
 		}
 	}
+	/**
+	 * 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 @@ private final class WriteableTransactionTransactionImpl extends ReadableTransact
 
 		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 @@ String getTableDialect() {
 		@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 @@ boolean isExistsIndex(String tableName, String indexName) throws SQLException {
 		}
 
 		public void clearTree(TreeName treeName) {
+			checkReadOnly();
 			try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName))){
 				execute(statement);
 				con.commit();
@@ -1168,6 +1187,7 @@ public void clearTree(TreeName treeName) {
 
 		@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 @@ public void deleteTree(TreeName treeName) {
 
 		@Override
 		public void put(TreeName treeName, ByteSequence key, ByteSequence value) {
+			checkReadOnly();
 			try {
 				upsert(treeName, key, value);
 			} catch (SQLException e) {
@@ -1250,6 +1271,9 @@ boolean update(TreeName treeName, ByteSequence key, ByteSequence value) throws S
 
 		@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 @@ public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) {
 
 		@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 @@ class VLVIndex extends AbstractTree implements ConfigurationChangeListener<Backe
   /** 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 @@ class VLVIndex extends AbstractTree implements ConfigurationChangeListener<Backe
     }
 
     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 @@ private SearchScope convertScope(final Scope cfgScope)
   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;
@@ -284,6 +285,86 @@ public void run(WriteableTransaction txn) throws Exception {
 		}
 	}
 
+	/**
+	 * 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 @@ public void run(WriteableTransaction txn) throws Exception
     }
   }
 
+  /**
+   * 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,