bitflicker64 opened a new pull request, #897:
URL: https://github.com/apache/hugegraph-toolchain/pull/897
## Purpose of the PR
No linked issue. These came out of a read through `hugegraph-tools`, looking
for small bugs and code that can move to Java 17 syntax without changing
behaviour.
Three of them replace the real error with a different one:
- `BackupManager` decides whether a failed shard was a timeout or a "Too
many records" limit with `getMessage().contains(...)`. When the server's error
body has no `message`, `getMessage()` is null, so the backup fails with
`NullPointerException` and the `ServerException` from the server is lost.
- `ToolUtil.printExceptionStackIfNeeded` asks "Type y(yes) to print
exception stack" and calls `Scanner.nextLine()`. When stdin is closed or empty
(cron, CI, `< /dev/null`), that throws `NoSuchElementException` from inside the
error handler.
- `HdfsDirectory.fileSystem()` passes the `IOException` to
`ClientException(String, Object...)` as a format argument, so the cause is
dropped.
## Main Changes
Bug fixes (each covered by a new test that fails without the fix):
- `BackupManager.isTimeoutException` and `isLimitExceedException` use
`StringUtils.contains`, which is null-safe.
- `ToolUtil.printExceptionStackIfNeeded` treats no input as the default
answer, `n`. A failing command run without stdin now exits with
`EXIT_CODE_ERROR` (255), like any other failure, instead of also printing a
`NoSuchElementException` trace and exiting with 1.
- `HdfsDirectory.fileSystem()` passes the `IOException` as the cause. The
message text is unchanged.
- `UrlValidator` accepted ports up to 59999 but its last alternative,
`6[0-5]{2}[0-3][0-5]`, rejected most of 60000 to 65535, for example 60080 and
61080. It now accepts 0 to 65535 and still rejects 65536.
- `AuthHugeTypeConverter` parses `--types` case-insensitively, but checked
the "belong needs user and group" and "access needs group and target" rules
against lower-case names only. `--types BELONG` passed the check that `--types
belong` fails. The names are now lower-cased before the check.
- `PositiveValidator` used `Integer.parseInt`, but it also validates the
long options `--limit` and `--task-id`, so `--limit 3000000000` threw
`NumberFormatException`. It now uses `Long.parseLong`.
Cleanup, no behaviour change:
- Remove `catch (Throwable e) { throw e; }` from `BackupManager.backup`,
`RestoreManager.restore` and `DumpGraphManager.dump`. The `finally` blocks stay.
- Remove the private `BackupManager.readShards` and
`HugeGraphCommand.mode()`, which have no callers, and the imports only they
used.
- Pattern matching for `instanceof` in `ToolUtil.printOrThrow` and
`BackupManager.exceptionStackTrace`; an enhanced `for` in `ComputeSign`.
No option name, default or output text changes, apart from the errors above.
Tests: a new `ToolsCheckTest`, registered in `FuncTestSuite`, covers the
validators, the converter, the stdin case and the HDFS cause without a server.
`BackupFailureTest` gets one more case on its in-JVM fake server: a page
request that fails with an error body that has no `message`.
## 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:
- `mvn -ntp -pl hugegraph-tools -am test
-Dtest='org/apache/hugegraph/test/functional/FuncTestSuite'
-Dtools.test.disposable=true -Dsurefire.failIfNoSpecifiedTests=false` at
877763d1 on JDK 17 (Linux), against a HugeGraph 1.8.0 server with auth on, set
up the way `start-hugegraph-servers.sh` does it: 66 tests, 0 failures, 0
skipped.
- With only `hugegraph-tools/src/main` reverted to master, all 6 new
tests fail: `testAuthTypeDependenciesIgnoreCase` (no exception for `BELONG`),
`testUrlValidatorAcceptsHighPorts` (`ParameterException` for port 60080),
`testPositiveValidatorAcceptsLongValues` (`NumberFormatException`),
`testHdfsDirectoryKeepsCause` (null cause),
`testPrintExceptionStackWithEmptyStdin` (`NoSuchElementException`) and
`testPageFailureWithoutMessageFailsBackup` (`NullPointerException` from
`getMessage().contains`).
## Does this PR potentially affect the following parts?
- [x] Nope
- [ ] Dependencies (add/update license info)
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
## Documentation Status
- [ ] `Doc - TODO`
- [ ] `Doc - Done`
- [x] `Doc - No Need`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
