You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "bitflicker64 (via GitHub)" <gi...@apache.org> on 2026/10/10 23:06:09 UTC
[PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
bitflicker64 opened a new pull request, #3309:
URL: https://github.com/apache/hugegraph/pull/3309
## Purpose of the PR
- Related to the 1.8.0 release work in #3242. No issue is closed by this PR.
Before 1.8.0 we wanted more executable tests module by module, with attention to the parts that changed recently: the Java 17 migration, the new `hugegraph-struct` module, and the RocksDB and JRaft upgrades. Several test classes in the repository also compiled but never ran, because no suite or Surefire include picked them up.
This PR only touches test classes and suite registration. There are no production, dependency, configuration, CI or documentation changes. Each module is a separate commit so it can be reviewed on its own.
## Main Changes
| Commit | Lines | What it adds |
|---|---|---|
| `test(struct)` | +451 | Struct-side `BytesBufferTest`: varint rejection, unsigned limits, length-prefixed bytes, string endings, edge and index id encoding, partition and OLAP id parsing, tagged collections, buffer growth. `EdgeIdTest`: text form, parse errors, binary id generation. |
| `test(commons)` | +899 | `TypedOptionTest` (option parsing and checkers), `HugeConfigValidationTest` (URL normalization, input validation), `HostNameVerifierTest` (negative paths after #3307: SAN matching, wildcard scope, IP vs DNS, no CN fallback), rpc `RpcConfigTest` (self URL exclusion, invalid options, provider and consumer config, service cleanup on destroy). |
| `test(server)` | +677 | `RocksDBStdSessionsLifecycleTest` (unopened tables, rollback, reopen, lock release after force close and bad options, iterator double close, snapshot path), `RocksDBIteratorPoolTest`, `HstoreTableRoutingTest`. Registers `ConsumersTest`, `MemoryAllocateTest` and `MemoryManageTest`, which existed but never ran. |
| `test(pd)` | +692 | `KVOperationTest` (encoding, null checks, truncated input, whitelist decoding), `RaftKVStoreTest` (writes go through raft, leader check, raft failure, state machine replay), `PeerUtilTest`, `ZipUtilsTest` (snapshot checksums, zip-slip entries). Registers `RaftReflectionUtilTest`. |
| `test(store)` | +1029 | `Base58EncoderTest`, `KeyUtilTest`, `CRC64Test`, `ZipUtilsPathSafetyTest` (both ZipUtils copies), `SessionOperatorTest` (writes, ranges, iterator release, snapshots), `PartitionMetaCodecTest`, `HgBufferProxyTest`. Registers `HgStoreStateMachineTest`, `FutureClosureTest`, `HgAssertTest` and `HgPairTest`. The `reuseForks=false` setting of the core execution is unchanged. |
| `test(cluster)` | +164 | `SimpleClusterGraphTest`: creates schema, vertices and an edge through the server, reads them back by id and by adjacency through the HStore backend, and checks that invalid vertex requests and unknown ids are rejected. |
New RocksDB tests use `TemporaryFolder` instead of fixed paths. No new test binds a fixed port.
## Verifying these changes
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [x] Need tests and can be verified as follows:
- Run locally on JDK 17.0.20 and Maven 3.8.7, on 2026-10-11, at this PR head. Base is `36bc1c64b`. The figures are run/failed/errors/skipped.
| Command (from the repository root) | Before | After |
|---|---|---|
| `mvn test -pl hugegraph-struct` | 110/0/0/0 | 129/0/0/0 |
| `mvn test -pl hugegraph-commons/hugegraph-common -Dtest=UnitTestSuite -DskipCommonsTests=false` | 366/2/4/0 | 389/2/4/0 |
| `mvn test -pl hugegraph-commons/hugegraph-rpc -Dtest=UnitTestSuite -DskipCommonsTests=false` | 25/0/0/0 | 31/0/0/0 |
| `mvn test -pl hugegraph-server/hugegraph-test -P unit-test` | 996/0/0/8 | 1031/0/0/8 |
| `mvn test -pl hugegraph-pd/hg-pd-test -P pd-core-test` | 113/0/0/2 | 149/0/0/2 |
| `mvn test -pl hugegraph-store/hg-store-test -P store-common-test` | 2/0/0/0 | 14/0/0/0 |
| `... -P store-rocksdb-test` | 3/0/0/0 | 19/0/0/0 |
| `... -P store-raftcore-test` | 16/0/0/0 | 27/0/0/0 |
| `... -P store-client-test` | 67/0/0/0 | 90/0/0/0 |
| `... -P store-core-test` | 93 run, 1 failure (see below) | 100/0/0/0 |
| `mvn test -pl hugegraph-cluster-test/hugegraph-clustertest-test -P simple-cluster-test` | 6/0/0/0 | 8/0/0/0 |
- The 6 common failures and errors happen both before and after this change, and they come from the test host. Two `RestClientTest` HTTPS tests time out reaching an external endpoint, and four `MachineInfoTest` tests find no MAC or IP address on that machine.
- The one store-core failure before the change was `HeartbeatShutdownTest`, in a checkout where `hg-store-node` had not been compiled. With a full build it passes.
- Every new class was also run on its own at least twice. For each module, one expectation was broken on purpose to confirm that the tests fail.
- `mvn editorconfig:check` passes on every touched test module.
## Does this PR potentially affect the following parts?
- [ ] Dependencies ([add/update license](https://hugegraph.apache.org/docs/contribution-guidelines/contribute/#321-check-licenses) info & [regenerate_known_dependencies.sh](../install-dist/scripts/dependency/regenerate_known_dependencies.sh))
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
- [x] Nope
## Documentation Status
- [ ] `Doc - TODO`: required documentation is pending; complete it before merging.
- [ ] `Doc - Done`: documentation is included here or linked below.
- [x] `Doc - No Need`: no user-visible documentation is affected.
Documentation files in this PR or paired hugegraph-doc PR:
## Known limitations
These tests were left out or left unregistered on purpose.
- `MemoryConsumerTest` passes on its own but fails inside `UnitTestSuite`. An earlier test terminates the shared `EventHub` executor, and its `clearBackend()` calls then get `RejectedExecutionException`. It is left unregistered and unchanged.
- `PDWatchTest` has no assertions and needs a live PD. `PDPulseTest` is marked `@Useless` and waits forever against a live PD without stores. Neither is registered.
- The store client tests that need a running cluster are not registered. Seven classes in the `CoreSuiteTest` TODO block pass on their own; they are left for a follow-up.
- The cluster harness cannot stop and restart single nodes yet, so this PR has no node-failure scenarios. `multi-cluster-test` was not run locally.
Some new tests exposed behaviour that looks wrong in production code. Those tests are not part of this PR; the findings will be reported separately:
- In struct `IdUtil`, an id part that ends with a backtick does not survive escaping and unescaping. `SplicingIdGenerator.split(concat("a`", "b"))` returns `["a>b"]`.
- In struct `BytesBuffer.readVInt` and `readVLong`, the `leading == 0x80` check compares a signed byte with 128, so it is never true. `wrap({0x80, 1}).readVInt()` returns 1.
- `HugeConfig.validateOption` skips the option checker when a value is already of the target Java type. A port of 70000 passed as an `Integer` is accepted.
- `RocksDBStdSessions.scan()` opens a native iterator before the scan arguments are checked, and never closes it when the check fails.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4245451487
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/SessionOperatorTest.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * 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.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.rocksdb.access.DBStoreException;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession.BackendColumn;
+import org.apache.hugegraph.rocksdb.access.ScanIterator;
+import org.apache.hugegraph.rocksdb.access.SessionOperator;
+import org.apache.hugegraph.rocksdb.access.SessionOperatorImpl;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class SessionOperatorTest extends BaseRocksDbTest {
+
+ private static final String TABLE = "tbl";
+ private static final String GRAPH = "op-graph";
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private ExecutorService prober;
+ private String dataPath;
+ private RocksDBSession session;
+ private int probes;
+
+ private static byte[] b(String s) {
+ return s.getBytes(StandardCharsets.UTF_8);
+ }
+
+ private static List<String> keys(ScanIterator it) {
+ List<String> keys = new ArrayList<>();
+ try {
+ while (it.hasNext()) {
+ BackendColumn col = it.next();
+ keys.add(new String(col.name, StandardCharsets.UTF_8));
+ }
+ } finally {
+ it.close();
+ }
+ return keys;
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ this.prober = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "cf-lock-prober");
+ t.setDaemon(true);
+ return t;
+ });
+ this.dataPath = this.folder.newFolder("data").getAbsolutePath();
+ this.session = new RocksDBSession(hConfig, this.dataPath, GRAPH, 0);
+ this.session.checkTable(TABLE);
+ }
+
+ @After
+ public void tearDown() {
+ if (this.session != null) {
+ this.session.close();
Review Comment:
Important: `tearDown()` hangs instead of reporting a failure whenever a test ends with the CF read lock still held by the test thread. `prepare()` takes `cfHandleLock.readLock()` on the calling thread (`SessionOperatorImpl.java:219`). `session.close()` on this line reaches `RocksDBSession.shutdown()`, which calls `cfHandleLock.writeLock().lock()` (`RocksDBSession.java:626`) on the same thread. `ReentrantReadWriteLock` does not upgrade a read hold, so that call parks forever, `prober.shutdownNow()` below is never reached, and no test in this class has a timeout.
A lock-leak bug in production code is not needed to hit this. Any assertion that fails between `prepare()` and `commit()` leaves the read hold in place, for example the `assertNull` on line 138. A timeout in `assertCfLockReleased()` (line 118), which is the case the probe exists to report, ends the same way.
Measured at 03ffdf0bc on Linux, JDK 17.0.20, Maven 3.8.7: I changed line 138 to an assertion that fails and ran
```
mvn test -pl hugegraph-store/hg-store-test -am -P store-rocksdb-test \
-Dtest=SessionOperatorTest#testCommitAppliesTheBatchAndReturnsItsSize
```
The fork logged `shutdown db op-graph` and then nothing. After 65 s `jstack` showed `main` parked in `ReentrantReadWriteLock$WriteLock.lock` called from `RocksDBSession.shutdown(RocksDBSession.java:626)`, `RocksDBSession.close(RocksDBSession.java:1016)`, `SessionOperatorTest.tearDown(SessionOperatorTest.java:93)`. Surefire never printed a test result. In CI the `store` job would sit until its 45 minute `timeout-minutes` and name no failing test, so the first regression these tests catch shows up as a job timeout.
Please make the teardown safe before the class is registered in `RocksDbSuiteTest`:
- release the read holds the test thread still owns before `close()`. `getCfHandleReadLock()` returns a `ReentrantReadWriteLock.ReadLock`, and its `unlock()` throws `IllegalMonitorStateException` once the thread has no hold left, so a loop that unlocks until that exception is enough;
- add a JUnit `Timeout` rule as a backstop, so a stuck lock is reported as a failed test. The rule runs `@Before`, the test and `@After` on one thread, so the unlock loop still sees the test's holds. `@Test(timeout = ...)` does not help here: it moves only the test method to another thread and leaves `tearDown()` outside the timeout.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#issuecomment-6103168125
## [Codecov](https://app.codecov.io/gh/apache/hugegraph/pull/3309?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 96.95%. Comparing base ([`36bc1c6`](https://app.codecov.io/gh/apache/hugegraph/commit/36bc1c64ba2db23c40965ad952f3942cca31b1a7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a46e822`](https://app.codecov.io/gh/apache/hugegraph/commit/a46e8226c2902ed298477935d55d435a23008c2c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3309 +/- ##
=============================================
+ Coverage 41.57% 96.95% +55.38%
+ Complexity 7025 67 -6958
=============================================
Files 762 8 -754
Lines 67060 263 -66797
Branches 9021 21 -9000
=============================================
- Hits 27880 255 -27625
+ Misses 35969 4 -35965
+ Partials 3211 4 -3207
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hugegraph/pull/3309?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4246874333
##########
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:
The “absolute path” zip entry name is built using `File#getAbsolutePath()`, which is OS-dependent (e.g., Windows drive letters and backslashes). That can make this security test non-portable or fail to exercise the intended “absolute path zip-slip” branch consistently. Consider generating an absolute-style entry name in a platform-agnostic way (or branching the assertion per OS), and ensure the entry name uses zip-standard `/` separators.
##########
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:
`OptionSpace.register(...)` is called unconditionally. If other tests (or re-runs in the same JVM) register the same option space name, this can cause cross-test interference and failures that depend on execution order. Guard the registration (similar to how RocksDB options registration is guarded elsewhere) or use a unique namespace per test class run.
##########
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:
`String.getBytes()` / `new String(bytes)` use the platform default charset, which can make these tests fail on systems not using UTF-8 (or behave inconsistently across CI agents). Use an explicit charset (e.g., UTF_8) for both encoding and decoding to keep the test deterministic.
##########
hugegraph-commons/hugegraph-rpc/src/test/java/org/apache/hugegraph/unit/RpcConfigTest.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * 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;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hugegraph.config.ConfigException;
+import org.apache.hugegraph.config.HugeConfig;
+import org.apache.hugegraph.config.RpcOptions;
+import org.apache.hugegraph.rpc.RpcClientProvider;
+import org.apache.hugegraph.rpc.RpcConsumerConfig;
+import org.apache.hugegraph.rpc.RpcProviderConfig;
+import org.apache.hugegraph.rpc.RpcServer;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.testutil.Whitebox;
+import org.apache.hugegraph.unit.ServerClientTest.HelloService;
+import org.apache.hugegraph.unit.ServerClientTest.HelloServiceImpl;
+import org.junit.Test;
+
+import com.alipay.sofa.rpc.config.ConsumerConfig;
+import com.alipay.sofa.rpc.config.ProviderConfig;
+
+public class RpcConfigTest extends BaseUnitTest {
+
+ private static final String HELLO = HelloService.class.getName();
+
+ @Test
+ public void testExcludeSelfUrl() {
+ Assert.assertEquals("127.0.0.1:8092,127.0.0.1:8093",
+ excludeSelfUrl("127.0.0.1:8091,127.0.0.1:8092;127.0.0.1:8093",
+ "127.0.0.1:8091"));
+ // Order is kept and duplicates are collapsed
+ Assert.assertEquals("b:2,a:1",
+ excludeSelfUrl("b:2,a:1,b:2,self:0", "self:0"));
+ Assert.assertEquals("a:1", excludeSelfUrl("a:1", "self:0"));
+ Assert.assertEquals("", excludeSelfUrl("self:0,self:0", "self:0"));
+ Assert.assertEquals("", excludeSelfUrl("", "self:0"));
+ }
+
+ @Test
+ public void testClientDisabledWhenOnlySelfIsRemote() {
+ Map<String, Object> options = new HashMap<>();
+ options.put(RpcOptions.RPC_SERVER_HOST.name(), "127.0.0.1");
+ options.put(RpcOptions.RPC_SERVER_PORT.name(), "8091");
+ options.put(RpcOptions.RPC_REMOTE_URL.name(), "127.0.0.1:8091");
+ RpcClientProvider selfOnly = new RpcClientProvider(new HugeConfig(options));
+ Assert.assertFalse(selfOnly.enabled());
+ Assert.assertThrows(IllegalArgumentException.class, selfOnly::config, e -> {
+ Assert.assertContains("add an address other than self service", e.getMessage());
+ });
+ // Both are no-ops when the client is disabled
+ selfOnly.unreferAll();
+ selfOnly.destroy();
+
+ options.put(RpcOptions.RPC_REMOTE_URL.name(), "127.0.0.1:8091,127.0.0.1:8092");
+ RpcClientProvider withPeer = new RpcClientProvider(new HugeConfig(options));
+ try {
+ Assert.assertTrue(withPeer.enabled());
+ Assert.assertEquals("127.0.0.1:8092",
+ Whitebox.getInternalState(withPeer.config(), "remoteUrls"));
Review Comment:
This assertion relies on a private field name (`remoteUrls`) via reflection, which makes the test brittle to refactors that don’t change behavior. Prefer asserting through public behavior (e.g., the resulting consumer config’s direct URL, or a public accessor) so the test remains stable even if internal fields are renamed or reorganized.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4241798930
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -244,12 +250,18 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryManageTest.class,
Review Comment:
The description was corrected along with 03ffdf0bc: `MemoryAllocateTest` is intentionally left out of `UnitTestSuite` (two of its tests force an unreleased 1 GiB allocation), and the PR body now lists it under Known limitations with the server unit count at 1027/0/0/8. The "Registers" line names only `ConsumersTest` and `MemoryManageTest`. No code change needed.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4245718635
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/SessionOperatorTest.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * 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.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.rocksdb.access.DBStoreException;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession.BackendColumn;
+import org.apache.hugegraph.rocksdb.access.ScanIterator;
+import org.apache.hugegraph.rocksdb.access.SessionOperator;
+import org.apache.hugegraph.rocksdb.access.SessionOperatorImpl;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class SessionOperatorTest extends BaseRocksDbTest {
+
+ private static final String TABLE = "tbl";
+ private static final String GRAPH = "op-graph";
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private ExecutorService prober;
+ private String dataPath;
+ private RocksDBSession session;
+ private int probes;
+
+ private static byte[] b(String s) {
+ return s.getBytes(StandardCharsets.UTF_8);
+ }
+
+ private static List<String> keys(ScanIterator it) {
+ List<String> keys = new ArrayList<>();
+ try {
+ while (it.hasNext()) {
+ BackendColumn col = it.next();
+ keys.add(new String(col.name, StandardCharsets.UTF_8));
+ }
+ } finally {
+ it.close();
+ }
+ return keys;
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ this.prober = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "cf-lock-prober");
+ t.setDaemon(true);
+ return t;
+ });
+ this.dataPath = this.folder.newFolder("data").getAbsolutePath();
+ this.session = new RocksDBSession(hConfig, this.dataPath, GRAPH, 0);
+ this.session.checkTable(TABLE);
+ }
+
+ @After
+ public void tearDown() {
+ if (this.session != null) {
+ this.session.close();
Review Comment:
Fixed in 1c0a9a0ee. `tearDown()` now unlocks the read holds the test thread still owns (looping until `IllegalMonitorStateException`) before `session.close()`, and a `Timeout.seconds(60)` rule backs it up. Checked with your repro: with line 138 forced to fail, the test now reports that assertion in 8 s. With the unlock loop removed as well, the rule fails `tearDown` with `TestTimedOutException` after 60 s instead of hanging. `SessionOperatorTest` alone passed twice (8/0), and `store-rocksdb-test` 19/0.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4251126151
##########
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());
Review Comment:
The lock regression now checks the existing native IOError status instead of localized text. It still verifies rejection of the second open, the live owner and its data, and a successful reopen after close. The complete current Maven reactor passed the lifecycle test class.
##########
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:
The fixture helpers now use explicit UTF-8 for encoding and decoding. A native close/reopen regression verifies a Chinese key and a value containing a surrogate pair; the old helpers fail the non-UTF-8 control. The current Maven reactor passed.
##########
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());
+
+ ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32());
+
+ assertEquals("ok", read(new File(out, "snapshot/ok.sst")));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "escaped2.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertFalse(new File(this.folder.getRoot(), "absolute.txt").exists());
+ }
+
+ @Test
+ public void testCoreDecompressRejectsEntriesEscapingTheTargetDir() throws IOException {
+ File out = this.folder.newFolder("out");
+ // "../out-evil" shares the textual prefix of the target dir and must still be rejected
+ for (String evil : new String[]{"../escaped.txt", "snapshot/../../escaped.txt",
+ "../out-evil/x.txt"}) {
+ File zip = maliciousZip(evil);
+ IOException e = assertThrows(IOException.class, () ->
+ org.apache.hugegraph.store.util.ZipUtils.decompress(
+ zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32()));
+ assertTrue(e.getMessage(), e.getMessage().contains("outside of the target dir"));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertTrue(zip.delete());
Review Comment:
Each attack now receives a distinct archive owned by TemporaryFolder, so the containment checks no longer depend on deleting and reusing a fixed filename. All rejection and outside-target assertions remain intact. Both ZIP implementations passed the current Maven tests.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4246727331
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBIteratorPoolTest.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.backend.store.rocksdb;
+
+import java.nio.charset.StandardCharsets;
+
+import org.apache.hugegraph.backend.store.rocksdb.RocksDBIteratorPool.ReusedRocksIterator;
+import org.apache.hugegraph.testutil.Assert;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.rocksdb.ColumnFamilyDescriptor;
+import org.rocksdb.ColumnFamilyHandle;
+import org.rocksdb.Options;
+import org.rocksdb.RocksDB;
+import org.rocksdb.RocksIterator;
+
+public class RocksDBIteratorPoolTest {
+
+ private static final String CF = "iter-pool-cf";
+
+ @Rule
+ public TemporaryFolder temporary = new TemporaryFolder();
+
+ private Options options;
+ private RocksDB rocksdb;
+ private ColumnFamilyHandle cfh;
+ private RocksDBIteratorPool pool;
+
+ @Before
+ public void setup() throws Exception {
+ this.options = new Options().setCreateIfMissing(true);
+ this.rocksdb = RocksDB.open(this.options, this.temporary.newFolder().getAbsolutePath());
+ this.cfh = this.rocksdb.createColumnFamily(new ColumnFamilyDescriptor(bytes(CF)));
+ this.rocksdb.put(this.cfh, bytes("k1"), bytes("v1"));
+ this.rocksdb.put(this.cfh, bytes("k2"), bytes("v2"));
+ this.pool = new RocksDBIteratorPool(this.rocksdb, this.cfh);
+ }
+
+ @After
+ public void teardown() {
+ // Native objects must be released child-first: pool, CF, DB, options
+ try {
+ this.pool.close();
+ } finally {
+ this.cfh.close();
+ this.rocksdb.close();
+ this.options.close();
Review Comment:
Fixed in 4d989c05d: `teardown()` now null-checks the pool, CF handle, DB and options before closing each, still child-first. With a failure injected into `setup()`, the run reports that `IllegalStateException` and nothing else. `RocksDBIteratorPoolTest` passed alone twice (3/0); `unit-test` 1027/0/0/8.
##########
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());
Review Comment:
Keeping this one. The error comes from RocksDB's file-lock path, and every port includes "lock" in it: here it is "lock hold by current process" because the second open is in the same JVM. The check is what tells lock contention apart from any other open failure, which is the behaviour the test protects. Matching only the exception type would also accept unrelated `RocksDBException`s.
##########
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:
Keeping the helpers as they are. They copy `BaseRocksDBUnitTest.getBytes/getString`, which the other RocksDB unit tests use, and every value passed through them is an ASCII literal ("k", "v1", "committed" and so on), so any platform charset gives the same bytes. Changing only this class would make it differ from its neighbours without changing 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());
+
+ ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32());
+
+ assertEquals("ok", read(new File(out, "snapshot/ok.sst")));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "escaped2.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertFalse(new File(this.folder.getRoot(), "absolute.txt").exists());
+ }
+
+ @Test
+ public void testCoreDecompressRejectsEntriesEscapingTheTargetDir() throws IOException {
+ File out = this.folder.newFolder("out");
+ // "../out-evil" shares the textual prefix of the target dir and must still be rejected
+ for (String evil : new String[]{"../escaped.txt", "snapshot/../../escaped.txt",
+ "../out-evil/x.txt"}) {
+ File zip = maliciousZip(evil);
+ IOException e = assertThrows(IOException.class, () ->
+ org.apache.hugegraph.store.util.ZipUtils.decompress(
+ zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32()));
+ assertTrue(e.getMessage(), e.getMessage().contains("outside of the target dir"));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertTrue(zip.delete());
Review Comment:
Keeping it. The delete is needed because `maliciousZip()` reuses one file name and `TemporaryFolder.newFile` fails if it already exists. The archive is written with try-with-resources, and `decompress` closes its handle. If the delete ever failed, it would point to a leaked handle, which is worth reporting. Store CI runs on Linux only.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4251128003
##########
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:
No guard is needed for the existing OptionSpace contract: register() warns and replaces the holder/options; it does not throw on the same namespace. The unchanged complete validation class passed twice in one JVM. Adding a conditional registration would hide replacement behavior rather than repair a reproduced failure.
##########
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:
Covered by the same explicit UTF-8 helper correction and native Chinese/surrogate-pair reopen regression. The complete current Maven reactor passed; the remaining lifecycle assertions are unchanged.
##########
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:
The absolute ZIP entry now uses ZIP-standard slash separators, with an explicit assertion that its resolved path remains outside the output directory. Relative traversal attacks and all containment assertions remain. Current Maven tests passed; a Windows runner was not executed locally.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4241431444
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/store/RaftKVStoreTest.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.pd.core.store;
+
+import java.nio.ByteBuffer;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.pd.common.PDException;
+import org.apache.hugegraph.pd.config.PDConfig;
+import org.apache.hugegraph.pd.grpc.Pdpb;
+import org.apache.hugegraph.pd.raft.KVOperation;
+import org.apache.hugegraph.pd.raft.RaftEngine;
+import org.apache.hugegraph.pd.raft.RaftStateMachine;
+import org.apache.hugegraph.pd.store.HgKVStore;
+import org.apache.hugegraph.pd.store.KV;
+import org.apache.hugegraph.pd.store.RaftKVStore;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import com.alipay.sofa.jraft.Iterator;
+import com.alipay.sofa.jraft.Status;
+import com.alipay.sofa.jraft.entity.Task;
+import com.alipay.sofa.jraft.error.RaftError;
+
+/**
+ * Covers RaftKVStore as both sides of a raft round trip: writes become serialized raft tasks,
+ * and the decoded operations are dispatched back onto the local store by invoke().
+ */
+public class RaftKVStoreTest {
+
+ private static final byte[] KEY = "key".getBytes();
+ private static final byte[] VALUE = "value".getBytes();
+
+ private HgKVStore store;
+ private RaftEngine engine;
+ private RaftKVStore raftStore;
+
+ @Before
+ public void setUp() {
+ this.store = Mockito.mock(HgKVStore.class);
+ this.engine = Mockito.mock(RaftEngine.class);
+ this.raftStore = new RaftKVStore(this.engine, this.store);
+ }
+
+ /**
+ * Makes the mocked engine behave like a single-node leader: the task payload is decoded
+ * exactly as a follower would decode the log entry, applied, then completed with status.
+ */
+ private void applyTasksAs(Status status) {
+ Mockito.doAnswer(invocation -> {
+ Task task = invocation.getArgument(0);
+ if (status.isOk()) {
+ ByteBuffer data = task.getData();
+ byte[] bytes = new byte[data.remaining()];
+ data.get(bytes);
+ this.raftStore.invoke(KVOperation.fromByteArray(bytes), null);
+ }
+ task.getDone().run(status);
+ return null;
+ }).when(this.engine).addTask(Mockito.any(Task.class));
+ }
+
+ @Test
+ public void testInitRegistersAsRaftTaskHandler() {
+ PDConfig config = new PDConfig();
+
+ this.raftStore.init(config);
+
+ Mockito.verify(this.store).init(config);
+ Mockito.verify(this.engine).addTaskHandler(this.raftStore);
+ }
+
+ @Test
+ public void testReadsAreServedLocallyWithoutRaft() throws PDException {
+ List<KV> kvs = Collections.singletonList(new KV(KEY, VALUE));
+ Mockito.when(this.store.get(KEY)).thenReturn(VALUE);
+ Mockito.when(this.store.getWithTTL(KEY)).thenReturn(VALUE);
+ Mockito.when(this.store.getListWithTTL(KEY)).thenReturn(Collections.singletonList(VALUE));
+ Mockito.when(this.store.scanPrefix(KEY)).thenReturn(kvs);
+ Mockito.when(this.store.scanRange(KEY, VALUE)).thenReturn(kvs);
+
+ Assert.assertArrayEquals(VALUE, this.raftStore.get(KEY));
+ Assert.assertArrayEquals(VALUE, this.raftStore.getWithTTL(KEY));
+ Assert.assertEquals(1, this.raftStore.getListWithTTL(KEY).size());
+ Assert.assertSame(kvs, this.raftStore.scanPrefix(KEY));
+ Assert.assertSame(kvs, this.raftStore.scanRange(KEY, VALUE));
+
+ Mockito.verify(this.engine, Mockito.never()).addTask(Mockito.any());
+ }
+
+ @Test
+ public void testWritesReachStoreOnlyThroughRaft() throws PDException {
+ applyTasksAs(Status.OK());
+
+ this.raftStore.put(KEY, VALUE);
+ this.raftStore.putWithTTL(KEY, VALUE, 60L);
+ this.raftStore.putWithTTL(KEY, VALUE, 5L, TimeUnit.MINUTES);
+ this.raftStore.remove(KEY);
+ this.raftStore.removeByPrefix(KEY);
+ this.raftStore.removeWithTTL(KEY);
+ this.raftStore.clear();
+
+ Mockito.verify(this.engine, Mockito.times(7)).addTask(Mockito.any(Task.class));
+ Mockito.verify(this.store).put(KEY, VALUE);
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 60L);
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 5L, TimeUnit.MINUTES);
+ Mockito.verify(this.store).remove(KEY);
+ Mockito.verify(this.store).removeByPrefix(KEY);
+ Mockito.verify(this.store).removeWithTTL(KEY);
+ Mockito.verify(this.store).clear();
+ Mockito.verifyNoMoreInteractions(this.store);
+ }
+
+ @Test
+ public void testWriteOnNonLeaderIsRejectedAndNotApplied() throws PDException {
+ // A RaftEngine that has not joined a group is never leader
+ RaftKVStore notLeader = new RaftKVStore(new RaftEngine(), this.store);
+
+ PDException e = Assert.assertThrows(PDException.class, () -> notLeader.put(KEY, VALUE));
+
+ Assert.assertTrue(e.getMessage(),
+ e.getMessage().contains("Error code = " +
+ Pdpb.ErrorType.NOT_LEADER_VALUE));
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testFailedRaftStatusFailsEveryWrite() {
+ applyTasksAs(new Status(RaftError.EINTERNAL, "injected raft failure"));
+
+ assertWriteFails(() -> this.raftStore.put(KEY, VALUE));
+ assertWriteFails(() -> this.raftStore.putWithTTL(KEY, VALUE, 1L));
+ assertWriteFails(() -> this.raftStore.putWithTTL(KEY, VALUE, 1L, TimeUnit.SECONDS));
+ assertWriteFails(() -> this.raftStore.remove(KEY));
+ assertWriteFails(() -> this.raftStore.removeByPrefix(KEY));
+ assertWriteFails(() -> this.raftStore.removeWithTTL(KEY));
+ assertWriteFails(() -> this.raftStore.clear());
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testEngineExceptionFailsWrite() {
+ Mockito.doThrow(new IllegalStateException("engine closed"))
+ .when(this.engine).addTask(Mockito.any(Task.class));
+
+ PDException e = Assert.assertThrows(PDException.class,
+ () -> this.raftStore.put(KEY, VALUE));
+ Assert.assertTrue(e.getMessage(), e.getMessage().contains("engine closed"));
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testInvokeDispatchesSnapshotPathFromAttach() throws PDException {
+ this.raftStore.invoke(KVOperation.createSaveSnapshot("/snap/save"), null);
+ this.raftStore.invoke(KVOperation.createLoadSnapshot("/snap/load"), null);
+
+ Mockito.verify(this.store).saveSnapshot("/snap/save");
+ Mockito.verify(this.store).loadSnapshot("/snap/load");
+ }
+
+ @Test
+ public void testInvokeIgnoresGetAndUnknownOperations() throws PDException {
+ Assert.assertFalse(this.raftStore.invoke(KVOperation.createGet(KEY), null));
+ Assert.assertFalse(this.raftStore.invoke(new KVOperation(KEY, VALUE, null, (byte) 0x7F),
+ null));
+
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testFollowerReplayAppliesTtlWithUnit() throws Exception {
+ RaftStateMachine stateMachine = new RaftStateMachine();
+ stateMachine.addTaskHandler(this.raftStore);
+ byte[] entry = KVOperation.createPutWithTTL(KEY, VALUE, 3L, TimeUnit.HOURS)
+ .toByteArray();
+ Iterator iter = Mockito.mock(Iterator.class);
+ Mockito.when(iter.hasNext()).thenReturn(true, false);
+ // A follower has no closure for the entry, so it must decode the log data
+ Mockito.when(iter.done()).thenReturn(null);
+ Mockito.when(iter.getData()).thenReturn(ByteBuffer.wrap(entry));
+
+ stateMachine.onApply(iter);
+
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 3L, TimeUnit.HOURS);
+ Mockito.verify(iter).next();
+ Mockito.verify(iter, Mockito.never()).setErrorAndRollback(Mockito.anyLong(),
+ Mockito.any());
+ }
+
+ @Test
+ public void testFollowerReplayStopsOnStoreFailure() throws Exception {
+ RaftStateMachine stateMachine = new RaftStateMachine();
+ stateMachine.addTaskHandler(this.raftStore);
+ Mockito.doThrow(new PDException(Pdpb.ErrorType.ROCKSDB_WRITE_ERROR_VALUE, "disk full"))
+ .when(this.store).put(KEY, VALUE);
+ Iterator iter = Mockito.mock(Iterator.class);
+ Mockito.when(iter.hasNext()).thenReturn(true);
Review Comment:
Fixed in 03ffdf0bc: `iter.hasNext()` now returns `true, false`, so a missing early return makes the loop call `next()` once and the `never()` check fails instead of the test running until the heap fills. `RaftKVStoreTest` passed alone twice (10/0) and `pd-core-test` passed with 149/0/0/2.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4240715483
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/store/RaftKVStoreTest.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.pd.core.store;
+
+import java.nio.ByteBuffer;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.pd.common.PDException;
+import org.apache.hugegraph.pd.config.PDConfig;
+import org.apache.hugegraph.pd.grpc.Pdpb;
+import org.apache.hugegraph.pd.raft.KVOperation;
+import org.apache.hugegraph.pd.raft.RaftEngine;
+import org.apache.hugegraph.pd.raft.RaftStateMachine;
+import org.apache.hugegraph.pd.store.HgKVStore;
+import org.apache.hugegraph.pd.store.KV;
+import org.apache.hugegraph.pd.store.RaftKVStore;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import com.alipay.sofa.jraft.Iterator;
+import com.alipay.sofa.jraft.Status;
+import com.alipay.sofa.jraft.entity.Task;
+import com.alipay.sofa.jraft.error.RaftError;
+
+/**
+ * Covers RaftKVStore as both sides of a raft round trip: writes become serialized raft tasks,
+ * and the decoded operations are dispatched back onto the local store by invoke().
+ */
+public class RaftKVStoreTest {
+
+ private static final byte[] KEY = "key".getBytes();
+ private static final byte[] VALUE = "value".getBytes();
+
+ private HgKVStore store;
+ private RaftEngine engine;
+ private RaftKVStore raftStore;
+
+ @Before
+ public void setUp() {
+ this.store = Mockito.mock(HgKVStore.class);
+ this.engine = Mockito.mock(RaftEngine.class);
+ this.raftStore = new RaftKVStore(this.engine, this.store);
+ }
+
+ /**
+ * Makes the mocked engine behave like a single-node leader: the task payload is decoded
+ * exactly as a follower would decode the log entry, applied, then completed with status.
+ */
+ private void applyTasksAs(Status status) {
+ Mockito.doAnswer(invocation -> {
+ Task task = invocation.getArgument(0);
+ if (status.isOk()) {
+ ByteBuffer data = task.getData();
+ byte[] bytes = new byte[data.remaining()];
+ data.get(bytes);
+ this.raftStore.invoke(KVOperation.fromByteArray(bytes), null);
+ }
+ task.getDone().run(status);
+ return null;
+ }).when(this.engine).addTask(Mockito.any(Task.class));
+ }
+
+ @Test
+ public void testInitRegistersAsRaftTaskHandler() {
+ PDConfig config = new PDConfig();
+
+ this.raftStore.init(config);
+
+ Mockito.verify(this.store).init(config);
+ Mockito.verify(this.engine).addTaskHandler(this.raftStore);
+ }
+
+ @Test
+ public void testReadsAreServedLocallyWithoutRaft() throws PDException {
+ List<KV> kvs = Collections.singletonList(new KV(KEY, VALUE));
+ Mockito.when(this.store.get(KEY)).thenReturn(VALUE);
+ Mockito.when(this.store.getWithTTL(KEY)).thenReturn(VALUE);
+ Mockito.when(this.store.getListWithTTL(KEY)).thenReturn(Collections.singletonList(VALUE));
+ Mockito.when(this.store.scanPrefix(KEY)).thenReturn(kvs);
+ Mockito.when(this.store.scanRange(KEY, VALUE)).thenReturn(kvs);
+
+ Assert.assertArrayEquals(VALUE, this.raftStore.get(KEY));
+ Assert.assertArrayEquals(VALUE, this.raftStore.getWithTTL(KEY));
+ Assert.assertEquals(1, this.raftStore.getListWithTTL(KEY).size());
+ Assert.assertSame(kvs, this.raftStore.scanPrefix(KEY));
+ Assert.assertSame(kvs, this.raftStore.scanRange(KEY, VALUE));
+
+ Mockito.verify(this.engine, Mockito.never()).addTask(Mockito.any());
+ }
+
+ @Test
+ public void testWritesReachStoreOnlyThroughRaft() throws PDException {
+ applyTasksAs(Status.OK());
+
+ this.raftStore.put(KEY, VALUE);
+ this.raftStore.putWithTTL(KEY, VALUE, 60L);
+ this.raftStore.putWithTTL(KEY, VALUE, 5L, TimeUnit.MINUTES);
+ this.raftStore.remove(KEY);
+ this.raftStore.removeByPrefix(KEY);
+ this.raftStore.removeWithTTL(KEY);
+ this.raftStore.clear();
+
+ Mockito.verify(this.engine, Mockito.times(7)).addTask(Mockito.any(Task.class));
+ Mockito.verify(this.store).put(KEY, VALUE);
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 60L);
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 5L, TimeUnit.MINUTES);
+ Mockito.verify(this.store).remove(KEY);
+ Mockito.verify(this.store).removeByPrefix(KEY);
+ Mockito.verify(this.store).removeWithTTL(KEY);
+ Mockito.verify(this.store).clear();
+ Mockito.verifyNoMoreInteractions(this.store);
+ }
+
+ @Test
+ public void testWriteOnNonLeaderIsRejectedAndNotApplied() throws PDException {
+ // A RaftEngine that has not joined a group is never leader
+ RaftKVStore notLeader = new RaftKVStore(new RaftEngine(), this.store);
+
+ PDException e = Assert.assertThrows(PDException.class, () -> notLeader.put(KEY, VALUE));
+
+ Assert.assertTrue(e.getMessage(),
+ e.getMessage().contains("Error code = " +
+ Pdpb.ErrorType.NOT_LEADER_VALUE));
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testFailedRaftStatusFailsEveryWrite() {
+ applyTasksAs(new Status(RaftError.EINTERNAL, "injected raft failure"));
+
+ assertWriteFails(() -> this.raftStore.put(KEY, VALUE));
+ assertWriteFails(() -> this.raftStore.putWithTTL(KEY, VALUE, 1L));
+ assertWriteFails(() -> this.raftStore.putWithTTL(KEY, VALUE, 1L, TimeUnit.SECONDS));
+ assertWriteFails(() -> this.raftStore.remove(KEY));
+ assertWriteFails(() -> this.raftStore.removeByPrefix(KEY));
+ assertWriteFails(() -> this.raftStore.removeWithTTL(KEY));
+ assertWriteFails(() -> this.raftStore.clear());
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testEngineExceptionFailsWrite() {
+ Mockito.doThrow(new IllegalStateException("engine closed"))
+ .when(this.engine).addTask(Mockito.any(Task.class));
+
+ PDException e = Assert.assertThrows(PDException.class,
+ () -> this.raftStore.put(KEY, VALUE));
+ Assert.assertTrue(e.getMessage(), e.getMessage().contains("engine closed"));
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testInvokeDispatchesSnapshotPathFromAttach() throws PDException {
+ this.raftStore.invoke(KVOperation.createSaveSnapshot("/snap/save"), null);
+ this.raftStore.invoke(KVOperation.createLoadSnapshot("/snap/load"), null);
+
+ Mockito.verify(this.store).saveSnapshot("/snap/save");
+ Mockito.verify(this.store).loadSnapshot("/snap/load");
+ }
+
+ @Test
+ public void testInvokeIgnoresGetAndUnknownOperations() throws PDException {
+ Assert.assertFalse(this.raftStore.invoke(KVOperation.createGet(KEY), null));
+ Assert.assertFalse(this.raftStore.invoke(new KVOperation(KEY, VALUE, null, (byte) 0x7F),
+ null));
+
+ Mockito.verifyNoInteractions(this.store);
+ }
+
+ @Test
+ public void testFollowerReplayAppliesTtlWithUnit() throws Exception {
+ RaftStateMachine stateMachine = new RaftStateMachine();
+ stateMachine.addTaskHandler(this.raftStore);
+ byte[] entry = KVOperation.createPutWithTTL(KEY, VALUE, 3L, TimeUnit.HOURS)
+ .toByteArray();
+ Iterator iter = Mockito.mock(Iterator.class);
+ Mockito.when(iter.hasNext()).thenReturn(true, false);
+ // A follower has no closure for the entry, so it must decode the log data
+ Mockito.when(iter.done()).thenReturn(null);
+ Mockito.when(iter.getData()).thenReturn(ByteBuffer.wrap(entry));
+
+ stateMachine.onApply(iter);
+
+ Mockito.verify(this.store).putWithTTL(KEY, VALUE, 3L, TimeUnit.HOURS);
+ Mockito.verify(iter).next();
+ Mockito.verify(iter, Mockito.never()).setErrorAndRollback(Mockito.anyLong(),
+ Mockito.any());
+ }
+
+ @Test
+ public void testFollowerReplayStopsOnStoreFailure() throws Exception {
+ RaftStateMachine stateMachine = new RaftStateMachine();
+ stateMachine.addTaskHandler(this.raftStore);
+ Mockito.doThrow(new PDException(Pdpb.ErrorType.ROCKSDB_WRITE_ERROR_VALUE, "disk full"))
+ .when(this.store).put(KEY, VALUE);
+ Iterator iter = Mockito.mock(Iterator.class);
+ Mockito.when(iter.hasNext()).thenReturn(true);
Review Comment:
Minor: `iter.hasNext()` returns `true` on every call, so this test only ends through the `return` after `setErrorAndRollback` in `RaftStateMachine.onApply` (`RaftStateMachine.java:133-134`), which is the behaviour it protects. If that `return` is dropped, the loop calls `iter.next()`, decodes the same entry, fails on `store.put` again and never ends on its own. It runs until the heap fills with recorded mock invocations or the job times out, so the regression shows up as an `OutOfMemoryError` or a stuck `pd-core-test` run, and `verify(iter, never()).next()` on line 225 is never reached.
Please bound the iterator as `testFollowerReplayAppliesTtlWithUnit` does on line 196:
```suggestion
Mockito.when(iter.hasNext()).thenReturn(true, false);
```
Then a missing `return` makes the loop call `iter.next()` once and stop, and the `never()` check fails with a clear message.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4253755513
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -247,13 +252,19 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryManageTest.class,
/* utils */
ConsumersTest.class,
VersionTest.class,
JsonUtilTest.class,
StringEncodingTest.class,
CompressUtilTest.class,
+ ConsumersTest.class,
Review Comment:
Removed the second ConsumersTest entry and retained the original registration. The real JUnit suite description now selects each unique method once; targeted Maven tests, formatting and reactor compilation pass, with independent fixture review completed. Test implementations and resource assertions are unchanged.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4241440418
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -244,12 +250,18 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryManageTest.class,
Review Comment:
The PR description says `MemoryAllocateTest` is newly registered, but this suite adds only `MemoryManageTest`; the unit-test Surefire execution includes only `UnitTestSuite`, so `MemoryAllocateTest` still will not run. Please either add `MemoryAllocateTest` to this suite (and import it) or correct the PR description and verification claims.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4250773459
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -244,12 +251,19 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryAllocateTest.class,
Review Comment:
Verified in the current source: MemoryAllocateTest is excluded from the default unit suite, and the PR description explicitly identifies it as an unregistered allocation stress test. The ordinary suite retains MemoryManageTest and ConsumersTest.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/SessionOperatorTest.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * 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.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.rocksdb.access.DBStoreException;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession.BackendColumn;
+import org.apache.hugegraph.rocksdb.access.ScanIterator;
+import org.apache.hugegraph.rocksdb.access.SessionOperator;
+import org.apache.hugegraph.rocksdb.access.SessionOperatorImpl;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class SessionOperatorTest extends BaseRocksDbTest {
+
+ private static final String TABLE = "tbl";
+ private static final String GRAPH = "op-graph";
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private ExecutorService prober;
+ private String dataPath;
+ private RocksDBSession session;
+ private int probes;
+
+ private static byte[] b(String s) {
+ return s.getBytes(StandardCharsets.UTF_8);
+ }
+
+ private static List<String> keys(ScanIterator it) {
+ List<String> keys = new ArrayList<>();
+ try {
+ while (it.hasNext()) {
+ BackendColumn col = it.next();
+ keys.add(new String(col.name, StandardCharsets.UTF_8));
+ }
+ } finally {
+ it.close();
+ }
+ return keys;
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ this.prober = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "cf-lock-prober");
+ t.setDaemon(true);
+ return t;
+ });
+ this.dataPath = this.folder.newFolder("data").getAbsolutePath();
+ this.session = new RocksDBSession(hConfig, this.dataPath, GRAPH, 0);
+ this.session.checkTable(TABLE);
+ }
+
+ @After
+ public void tearDown() {
+ if (this.session != null) {
+ this.session.close();
+ }
+ this.prober.shutdownNow();
Review Comment:
Verified in the current source: teardown releases the read-lock holds owned by the test thread before closing the session, and the class has a 60-second timeout. This addresses the lock-upgrade hang described here.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4248266101
##########
hugegraph-commons/hugegraph-rpc/src/test/java/org/apache/hugegraph/unit/RpcConfigTest.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * 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;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hugegraph.config.ConfigException;
+import org.apache.hugegraph.config.HugeConfig;
+import org.apache.hugegraph.config.RpcOptions;
+import org.apache.hugegraph.rpc.RpcClientProvider;
+import org.apache.hugegraph.rpc.RpcConsumerConfig;
+import org.apache.hugegraph.rpc.RpcProviderConfig;
+import org.apache.hugegraph.rpc.RpcServer;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.testutil.Whitebox;
+import org.apache.hugegraph.unit.ServerClientTest.HelloService;
+import org.apache.hugegraph.unit.ServerClientTest.HelloServiceImpl;
+import org.junit.Test;
+
+import com.alipay.sofa.rpc.config.ConsumerConfig;
+import com.alipay.sofa.rpc.config.ProviderConfig;
+
+public class RpcConfigTest extends BaseUnitTest {
+
+ private static final String HELLO = HelloService.class.getName();
+
+ @Test
+ public void testExcludeSelfUrl() {
+ Assert.assertEquals("127.0.0.1:8092,127.0.0.1:8093",
+ excludeSelfUrl("127.0.0.1:8091,127.0.0.1:8092;127.0.0.1:8093",
+ "127.0.0.1:8091"));
+ // Order is kept and duplicates are collapsed
+ Assert.assertEquals("b:2,a:1",
+ excludeSelfUrl("b:2,a:1,b:2,self:0", "self:0"));
+ Assert.assertEquals("a:1", excludeSelfUrl("a:1", "self:0"));
+ Assert.assertEquals("", excludeSelfUrl("self:0,self:0", "self:0"));
+ Assert.assertEquals("", excludeSelfUrl("", "self:0"));
+ }
+
+ @Test
+ public void testClientDisabledWhenOnlySelfIsRemote() {
+ Map<String, Object> options = new HashMap<>();
+ options.put(RpcOptions.RPC_SERVER_HOST.name(), "127.0.0.1");
+ options.put(RpcOptions.RPC_SERVER_PORT.name(), "8091");
+ options.put(RpcOptions.RPC_REMOTE_URL.name(), "127.0.0.1:8091");
+ RpcClientProvider selfOnly = new RpcClientProvider(new HugeConfig(options));
+ Assert.assertFalse(selfOnly.enabled());
+ Assert.assertThrows(IllegalArgumentException.class, selfOnly::config, e -> {
+ Assert.assertContains("add an address other than self service", e.getMessage());
+ });
+ // Both are no-ops when the client is disabled
+ selfOnly.unreferAll();
+ selfOnly.destroy();
+
+ options.put(RpcOptions.RPC_REMOTE_URL.name(), "127.0.0.1:8091,127.0.0.1:8092");
+ RpcClientProvider withPeer = new RpcClientProvider(new HugeConfig(options));
+ try {
+ Assert.assertTrue(withPeer.enabled());
+ Assert.assertEquals("127.0.0.1:8092",
+ Whitebox.getInternalState(withPeer.config(), "remoteUrls"));
Review Comment:
Changed in 7aa5b789e: the test now checks `getDirectUrl()` on the consumer config that `RpcConsumerConfig` builds for SOFA, rather than reading the private `remoteUrls` field. That method is private, so it still goes through the module's `Whitebox` helper, the same way the other tests in this class do, but it asserts the value the client actually uses. `RpcConfigTest` passed alone twice (6/0) and the rpc `UnitTestSuite` 31/0. An injected wrong expectation fails at line 77.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4240895963
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -244,12 +251,19 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryAllocateTest.class,
Review Comment:
[P2] Scale down and clean up allocation tests before registering them
This registration makes the ordinary unit suite run testNettyAllocate(), which requests a real 1-GiB direct ByteBuf and never releases its three buffers, and testOnHeapAllocate(), which creates a real 1-GiB byte array. The forceAllocate implementations call directBuffer((int) size) and new byte[(int) size], so this introduces a >1-GiB heap requirement and a 1-GiB direct-memory request even when the allocator is correct. A unit-test fork capped at 1 GiB or less cannot pass the heap allocation. Please use a small temporary manager limit and small boundary allocations, restore the prior settings, and release every ByteBuf in finally before enabling this class.
Reviewed head: a46e8226c2902ed298477935d55d435a23008c2c. This is source-level analysis; I did not independently execute this scenario.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/SessionOperatorTest.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * 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.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hugegraph.rocksdb.access.DBStoreException;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession;
+import org.apache.hugegraph.rocksdb.access.RocksDBSession.BackendColumn;
+import org.apache.hugegraph.rocksdb.access.ScanIterator;
+import org.apache.hugegraph.rocksdb.access.SessionOperator;
+import org.apache.hugegraph.rocksdb.access.SessionOperatorImpl;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class SessionOperatorTest extends BaseRocksDbTest {
+
+ private static final String TABLE = "tbl";
+ private static final String GRAPH = "op-graph";
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private ExecutorService prober;
+ private String dataPath;
+ private RocksDBSession session;
+ private int probes;
+
+ private static byte[] b(String s) {
+ return s.getBytes(StandardCharsets.UTF_8);
+ }
+
+ private static List<String> keys(ScanIterator it) {
+ List<String> keys = new ArrayList<>();
+ try {
+ while (it.hasNext()) {
+ BackendColumn col = it.next();
+ keys.add(new String(col.name, StandardCharsets.UTF_8));
+ }
+ } finally {
+ it.close();
+ }
+ return keys;
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ this.prober = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "cf-lock-prober");
+ t.setDaemon(true);
+ return t;
+ });
+ this.dataPath = this.folder.newFolder("data").getAbsolutePath();
+ this.session = new RocksDBSession(hConfig, this.dataPath, GRAPH, 0);
+ this.session.checkTable(TABLE);
+ }
+
+ @After
+ public void tearDown() {
+ if (this.session != null) {
+ this.session.close();
+ }
+ this.prober.shutdownNow();
Review Comment:
[P2] Make teardown safe when the read-lock probe times out
If commit/rollback leaves the read lock held, which assertCfLockReleased() is specifically intended to detect, its future.get() times out but JUnit still calls this teardown. RocksDBSession.close() then reaches shutdown(), whose writeLock().lock() runs on the test thread while that thread still owns the read lock acquired by prepare(). ReentrantReadWriteLock cannot perform that upgrade, so the test hangs instead of reporting the 10-second failure, and shutdownNow() is never reached. Release any outstanding read-lock holds owned by the test thread and let the probe finish before closing the session; merely moving shutdownNow() will not interrupt writeLock().lock().
Reviewed head: a46e8226c2902ed298477935d55d435a23008c2c. This is source-level analysis; I did not independently execute this scenario.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4245841851
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBIteratorPoolTest.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.backend.store.rocksdb;
+
+import java.nio.charset.StandardCharsets;
+
+import org.apache.hugegraph.backend.store.rocksdb.RocksDBIteratorPool.ReusedRocksIterator;
+import org.apache.hugegraph.testutil.Assert;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.rocksdb.ColumnFamilyDescriptor;
+import org.rocksdb.ColumnFamilyHandle;
+import org.rocksdb.Options;
+import org.rocksdb.RocksDB;
+import org.rocksdb.RocksIterator;
+
+public class RocksDBIteratorPoolTest {
+
+ private static final String CF = "iter-pool-cf";
+
+ @Rule
+ public TemporaryFolder temporary = new TemporaryFolder();
+
+ private Options options;
+ private RocksDB rocksdb;
+ private ColumnFamilyHandle cfh;
+ private RocksDBIteratorPool pool;
+
+ @Before
+ public void setup() throws Exception {
+ this.options = new Options().setCreateIfMissing(true);
+ this.rocksdb = RocksDB.open(this.options, this.temporary.newFolder().getAbsolutePath());
+ this.cfh = this.rocksdb.createColumnFamily(new ColumnFamilyDescriptor(bytes(CF)));
+ this.rocksdb.put(this.cfh, bytes("k1"), bytes("v1"));
+ this.rocksdb.put(this.cfh, bytes("k2"), bytes("v2"));
+ this.pool = new RocksDBIteratorPool(this.rocksdb, this.cfh);
+ }
+
+ @After
+ public void teardown() {
+ // Native objects must be released child-first: pool, CF, DB, options
+ try {
+ this.pool.close();
+ } finally {
+ this.cfh.close();
+ this.rocksdb.close();
+ this.options.close();
Review Comment:
`teardown()` assumes all native resources were created successfully in `setup()`. If `setup()` throws (e.g., `RocksDB.open(...)` fails), JUnit may still run `@After`, and these `close()` calls can throw `NullPointerException`, masking the root cause. Make the cleanup resilient by null-checking each resource (and closing in reverse creation order) so failures report the real setup error.
##########
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());
+
+ ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32());
+
+ assertEquals("ok", read(new File(out, "snapshot/ok.sst")));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "escaped2.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertFalse(new File(this.folder.getRoot(), "absolute.txt").exists());
+ }
+
+ @Test
+ public void testCoreDecompressRejectsEntriesEscapingTheTargetDir() throws IOException {
+ File out = this.folder.newFolder("out");
+ // "../out-evil" shares the textual prefix of the target dir and must still be rejected
+ for (String evil : new String[]{"../escaped.txt", "snapshot/../../escaped.txt",
+ "../out-evil/x.txt"}) {
+ File zip = maliciousZip(evil);
+ IOException e = assertThrows(IOException.class, () ->
+ org.apache.hugegraph.store.util.ZipUtils.decompress(
+ zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32()));
+ assertTrue(e.getMessage(), e.getMessage().contains("outside of the target dir"));
+ assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists());
+ assertFalse(new File(this.folder.getRoot(), "out-evil").exists());
+ assertTrue(zip.delete());
Review Comment:
Asserting `zip.delete()` can be flaky on some platforms/filesystems (notably Windows) where deletion may fail transiently due to file locks/AV scanning. Since `TemporaryFolder` already cleans up, consider removing this assertion or replacing it with a tolerant cleanup (e.g., `Files.deleteIfExists(...)` without failing the test) to avoid CI flakiness.
##########
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());
Review Comment:
Asserting the exception message contains `\"lock\"` is brittle: RocksDB error strings can vary by version, OS, and locale. To reduce test flakiness, prefer asserting on the exception type only, or validate a stable observable (e.g., the presence of the lock file / error code if available) rather than string-matching the message.
##########
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:
These helpers use the platform default charset (`String#getBytes()` / `new String(byte[])`), which makes the tests environment-dependent. Use an explicit charset (e.g., UTF-8) to keep assertions deterministic across OS/JVM locales.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] test: add coverage across struct, commons, server, pd, store and cluster [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3309:
URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4253616863
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java:
##########
@@ -247,13 +252,19 @@
RocksDBSessionTest.class,
RocksDBCountersTest.class,
RocksDBTableQueryByIdsTest.class,
+ RocksDBStdSessionsLifecycleTest.class,
+ RocksDBIteratorPoolTest.class,
+
+ /* memory */
+ MemoryManageTest.class,
/* utils */
ConsumersTest.class,
VersionTest.class,
JsonUtilTest.class,
StringEncodingTest.class,
CompressUtilTest.class,
+ ConsumersTest.class,
Review Comment:
[P3] Avoid registering ConsumersTest twice
ConsumersTest is already present at line262, inherited from the current base; this added entry registers it a second time. JUnit Suite creates a runner for each class-array entry without deduplicating. The class has12 active @Test methods, so this schedules24 executions for12 unique cases, adding runtime and inflating the suite total without adding coverage. Please remove this second entry and keep the existing registration. This persists in the current PR but is not introduced by the latest two-file map-fixture commit. Source: https://junit.org/junit4/xref/org/junit/runners/model/RunnerBuilder.html#98 .
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org