bitflicker64 commented on code in PR #3309: URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4248564546
########## hugegraph-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigValidationTest.java: ########## @@ -0,0 +1,239 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package org.apache.hugegraph.unit.config; + +import static org.apache.hugegraph.config.OptionChecker.disallowEmpty; +import static org.apache.hugegraph.config.OptionChecker.rangeInt; + +import java.util.HashMap; +import java.util.Map; + +import org.apache.commons.configuration2.Configuration; +import org.apache.hugegraph.config.ConfigException; +import org.apache.hugegraph.config.ConfigListOption; +import org.apache.hugegraph.config.ConfigOption; +import org.apache.hugegraph.config.HugeConfig; +import org.apache.hugegraph.config.OptionHolder; +import org.apache.hugegraph.config.OptionSpace; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.unit.BaseUnitTest; +import org.junit.BeforeClass; +import org.junit.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + +public class HugeConfigValidationTest extends BaseUnitTest { + + @BeforeClass + public static void init() { + OptionSpace.register("validation-test", ValidationOptions.class.getName()); Review Comment: Keeping it. Registering a name again does not throw: `OptionSpace.register` logs a warning and stores the same holder, so a re-run in the same JVM gives the same result. The `validation-test` name and the `validation.*` option keys are used only by this class, so no other test can register something that conflicts with them. ########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/rocksdb/RocksDBStdSessionsLifecycleTest.java: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.unit.rocksdb; + +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.commons.configuration2.PropertiesConfiguration; +import org.apache.hugegraph.backend.store.BackendEntry.BackendColumnIterator; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBOptions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions.Session; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBStdSessions; +import org.apache.hugegraph.config.HugeConfig; +import org.apache.hugegraph.config.OptionSpace; +import org.apache.hugegraph.exception.BackendException; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.unit.BaseUnitTest; +import org.apache.hugegraph.unit.FakeObjects; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.rocksdb.RocksDBException; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; + +/** + * Lifecycle and negative-path tests for {@link RocksDBStdSessions}. Every test + * uses its own temporary directory so it never shares the fixed path used by + * {@link BaseRocksDBUnitTest}. + */ +public class RocksDBStdSessionsLifecycleTest extends BaseUnitTest { + + private static final String TABLE = "lifecycle-table"; + + @Rule + public TemporaryFolder temporary = new TemporaryFolder(); + + private final List<RocksDBSessions> opened = new ArrayList<>(); + + @BeforeClass + public static void registerRocksDBOptions() { + // Typed values in HugeConfig require the RocksDB options to be known + if (!OptionSpace.containKey(RocksDBOptions.NUM_LEVELS.name())) { + OptionSpace.register("rocksdb", RocksDBOptions.class.getName()); + } + } + + @After + public void closeOpenedSessions() { + for (RocksDBSessions sessions : this.opened) { + try { + // Closes this thread's session and, when it is the last one, the DB + sessions.close(); + } finally { + if (sessions.databaseOpened()) { + sessions.forceCloseRocksDB(); + } + } + } + this.opened.clear(); + } + + @Test + public void testOperationsOnUnopenedTableAreRejected() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("unopened")); + Session session = sessions.session(); + byte[] key = getBytes("k"); + + Assert.assertThrows(BackendException.class, () -> { + session.put("missing", key, getBytes("v")); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertFalse(session.hasChanges()); + + Assert.assertThrows(BackendException.class, () -> { + session.get("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.scan("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.delete("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.keyRange("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + + // The opened table is still fully usable after the rejected calls + session.put(TABLE, key, getBytes("v")); + Assert.assertEquals(1, (int) session.commit()); + Assert.assertEquals("v", getString(session.get(TABLE, key))); + } + + @Test + public void testCreateTableTwiceKeepsDataAndDropMissingTableIsNoop() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("idempotent")); + Session session = sessions.session(); + session.put(TABLE, getBytes("k"), getBytes("v")); + session.commit(); + + // Re-creating an existing table must not truncate or replace it + sessions.createTable(TABLE); + Assert.assertEquals("v", getString(session.get(TABLE, getBytes("k")))); + + // Dropping a table that never existed leaves the opened tables intact + sessions.dropTable("never-created"); + Assert.assertTrue(sessions.existsTable(TABLE)); + Assert.assertFalse(sessions.existsTable("never-created")); + + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.existsTable(TABLE)); + // Dropping again after the table is gone is still a no-op + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.openedTables().contains(TABLE)); + } + + @Test + public void testRollbackDiscardsPendingWrites() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("rollback")); + Session session = sessions.session(); + + Assert.assertEquals(0, (int) session.commit()); + session.put(TABLE, getBytes("k2"), getBytes("v2")); + Assert.assertEquals(1, (int) session.commit()); + + session.put(TABLE, getBytes("k1"), getBytes("v1")); + session.delete(TABLE, getBytes("k2")); + Assert.assertTrue(session.hasChanges()); + + session.rollback(); + Assert.assertFalse(session.hasChanges()); + Assert.assertEquals(0, (int) session.commit()); + // Neither the pending put nor the pending delete reached the DB + Assert.assertNull(session.get(TABLE, getBytes("k1"))); + Assert.assertEquals("v2", getString(session.get(TABLE, getBytes("k2")))); + } + + @Test + public void testCommittedDataSurvivesReopenButPendingWritesDoNot() throws Exception { + String path = this.newPath("reopen"); + RocksDBSessions sessions = this.open(path); + Session session = sessions.session(); + session.put(TABLE, getBytes("committed"), getBytes("v1")); + session.commit(); + session.put(TABLE, getBytes("pending"), getBytes("v2")); + Assert.assertTrue(session.hasChanges()); + + Assert.assertTrue(sessions.close()); + Assert.assertFalse(sessions.databaseOpened()); + + // Reopen without createTable(): existing column families are reloaded + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertTrue(reopened.existsTable(TABLE)); + Session session2 = reopened.session(); + Assert.assertFalse(session2.hasChanges()); + Assert.assertEquals("v1", getString(session2.get(TABLE, getBytes("committed")))); + Assert.assertNull(session2.get(TABLE, getBytes("pending"))); + } + + @Test + public void testLiveDatabaseCannotBeOpenedTwiceUntilClosed() throws Exception { + String path = this.newPath("exclusive"); + RocksDBSessions sessions = this.open(path); + sessions.session().put(TABLE, getBytes("k"), getBytes("v")); + sessions.session().commit(); + + Assert.assertThrows(RocksDBException.class, () -> { + this.openWithoutTable(path); + }, e -> { + Assert.assertContains("lock", e.getMessage()); + }); + // The failed open must not disturb the live instance + Assert.assertTrue(sessions.databaseOpened()); + Assert.assertEquals("v", getString(sessions.session().get(TABLE, getBytes("k")))); + + Assert.assertTrue(sessions.close()); + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertEquals("v", getString(reopened.session().get(TABLE, getBytes("k")))); + } + + @Test + public void testClosedDatabaseRejectsTableAndSessionAccess() throws Exception { + String path = this.newPath("closed"); + RocksDBSessions sessions = this.open(path); + sessions.forceCloseRocksDB(); + Assert.assertFalse(sessions.databaseOpened()); + + Assert.assertThrows(IllegalStateException.class, () -> { + sessions.createTable("another-table"); + }, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, () -> { + sessions.dropTable(TABLE); + }, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, sessions::compactRange, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, sessions::session, e -> { + Assert.assertContains("session pool is closed", e.getMessage()); + }); + + // Force close must release the database lock for a later owner + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertTrue(reopened.databaseOpened()); + Assert.assertEquals(ImmutableSet.of("default", TABLE), reopened.openedTables()); + } + + @Test + public void testScanIteratorCloseIsIdempotentAndStopsIteration() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("scan-close")); + Session session = sessions.session(); + session.put(TABLE, getBytes("a1"), getBytes("1")); + session.put(TABLE, getBytes("a2"), getBytes("2")); + session.put(TABLE, getBytes("b1"), getBytes("3")); + session.commit(); + + BackendColumnIterator iter = session.scan(TABLE, getBytes("a")); + Assert.assertTrue(iter.hasNext()); + Assert.assertEquals("a1", getString(iter.next().name)); + Assert.assertEquals("a1", getString(iter.position())); + + // Closing early releases the native iterator; further reads end cleanly + iter.close(); + iter.close(); + Assert.assertFalse(iter.hasNext()); + Assert.assertThrows(NoSuchElementException.class, iter::next); + + // A fully consumed iterator clears its paging position + BackendColumnIterator all = session.scan(TABLE, getBytes("a")); + Assert.assertEquals("a1", getString(all.next().name)); + Assert.assertEquals("a2", getString(all.next().name)); + Assert.assertFalse(all.hasNext()); + Assert.assertNull(all.position()); + all.close(); + } + + @Test + public void testMismatchedLevelCompressionsRejectedAndLockReleased() throws Exception { + String path = this.newPath("bad-options"); + PropertiesConfiguration conf = new PropertiesConfiguration(); + conf.setProperty(RocksDBOptions.NUM_LEVELS.name(), "3"); + conf.setProperty(RocksDBOptions.LEVELS_COMPRESSIONS.name(), "[none, snappy]"); + HugeConfig badConfig = new HugeConfig(conf); + + Assert.assertThrows(IllegalArgumentException.class, () -> { + new RocksDBStdSessions(badConfig, "db", "store", path, path); + }, e -> { + Assert.assertContains(RocksDBOptions.LEVELS_COMPRESSIONS.name(), e.getMessage()); + Assert.assertContains("but got 2 != 3", e.getMessage()); + }); + + // The failed open must not leave the database lock behind + RocksDBSessions sessions = this.open(path); + Assert.assertTrue(sessions.databaseOpened()); + } + + @Test + public void testBuildSnapshotPathRequiresExistingSnapshot() throws Exception { + File parent = this.temporary.newFolder("graph"); + File dataDir = new File(parent, "rocksdb-data/g"); + Assert.assertTrue(dataDir.getParentFile().mkdirs()); + String dataPath = dataDir.getAbsolutePath(); + RocksDBSessions sessions = this.open(dataPath); + + File expected = new File(parent, "snapshot_rocksdb-data/g"); + Assert.assertThrows(IllegalArgumentException.class, () -> { + sessions.buildSnapshotPath("snapshot"); + }, e -> { + Assert.assertContains("doesn't exist", e.getMessage()); + Assert.assertContains(expected.getPath(), e.getMessage()); + }); + + Assert.assertTrue(expected.mkdirs()); + Assert.assertEquals(expected.getPath(), sessions.buildSnapshotPath("snapshot")); + } + + private String newPath(String name) throws Exception { + return new File(this.temporary.newFolder(name), "db").getAbsolutePath(); + } + + private RocksDBSessions open(String path) throws RocksDBException { + RocksDBSessions sessions = this.openWithoutTable(path); + sessions.createTable(TABLE); + return sessions; + } + + private RocksDBSessions openWithoutTable(String path) throws RocksDBException { + // Passing no CF names still reopens every column family found on disk + RocksDBSessions sessions = new RocksDBStdSessions(FakeObjects.newConfig(), "db", + "store", path, path, + ImmutableList.of()); + this.opened.add(sessions); + return sessions; + } + + private static byte[] getBytes(String str) { + return str.getBytes(); + } + + private static String getString(byte[] bytes) { + return bytes == null ? null : new String(bytes); + } Review Comment: Same point as the earlier thread on this line (https://github.com/apache/hugegraph/pull/3309#discussion_r4246727950): the helpers copy `BaseRocksDBUnitTest`, and every value passed through them is an ASCII literal, so the platform charset does not change any result. ########## hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/ZipUtilsPathSafetyTest.java: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.store.rocksdb; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.zip.CRC32; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +import org.apache.hugegraph.rocksdb.access.util.CRC64; +import org.apache.hugegraph.rocksdb.access.util.ZipUtils; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** + * Snapshot archives are produced and unpacked by two ZipUtils copies: the RocksDB one used for + * partition snapshots and the core one. Both must keep extracted files inside the target dir. + */ +public class ZipUtilsPathSafetyTest { + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + private static void write(File file, String content) throws IOException { + Files.createDirectories(file.getParentFile().toPath()); + Files.write(file.toPath(), content.getBytes(StandardCharsets.UTF_8)); + } + + private static String read(File file) throws IOException { + return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8); + } + + private File maliciousZip(String... entryNames) throws IOException { + File zip = this.folder.newFile("malicious.zip"); + try (ZipOutputStream zos = new ZipOutputStream(new FileOutputStream(zip))) { + zos.putNextEntry(new ZipEntry("snapshot/ok.sst")); + zos.write("ok".getBytes(StandardCharsets.UTF_8)); + zos.closeEntry(); + for (String name : entryNames) { + zos.putNextEntry(new ZipEntry(name)); + zos.write("pwned".getBytes(StandardCharsets.UTF_8)); + zos.closeEntry(); + } + } + return zip; + } + + @Test + public void testRoundTripKeepsNestedLayoutAndChecksumsAgree() throws IOException { + File root = this.folder.newFolder("root"); + write(new File(root, "snapshot/CURRENT"), "MANIFEST-000001"); + write(new File(root, "snapshot/sub/dir/000004.sst"), "sst-bytes"); + Files.createDirectories(new File(root, "snapshot/empty").toPath()); + File zip = new File(this.folder.getRoot(), "snapshot.zip"); + File out = this.folder.newFolder("out"); + + CRC64 written = new CRC64(); + ZipUtils.compress(root.getAbsolutePath(), "snapshot", zip.getAbsolutePath(), written); + CRC64 read = new CRC64(); + ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), read); + + assertEquals("MANIFEST-000001", read(new File(out, "snapshot/CURRENT"))); + assertEquals("sst-bytes", read(new File(out, "snapshot/sub/dir/000004.sst"))); + // The checksum covers the whole archive on both sides, so it detects transfer damage + assertTrue(written.getValue() != 0L); + assertEquals(written.getValue(), read.getValue()); + + File coreOut = this.folder.newFolder("core-out"); + CRC32 coreRead = new CRC32(); + org.apache.hugegraph.store.util.ZipUtils.decompress(zip.getAbsolutePath(), + coreOut.getAbsolutePath(), coreRead); + assertArrayEquals(Files.readAllBytes(new File(out, "snapshot/sub/dir/000004.sst").toPath()), + Files.readAllBytes(new File(coreOut, "snapshot/sub/dir/000004.sst").toPath())); + } + + @Test + public void testRocksDbDecompressSkipsEntriesEscapingTheTargetDir() throws IOException { + File out = this.folder.newFolder("out"); + File zip = maliciousZip("../escaped.txt", "snapshot/../../escaped2.txt", "../out-evil/x.txt", + new File(this.folder.getRoot(), "absolute.txt").getAbsolutePath()); Review Comment: Keeping it. On the Linux runners that run the Store suites, `getAbsolutePath()` gives a real `/...` entry name, which is exactly the absolute-path entry the decompress check has to skip. The store tests do not run on Windows (RocksDB native setup and the fixed `/tmp` paths in other store tests already assume a POSIX host), so a Windows variant would not run anywhere. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
