kratos0718 opened a new pull request, #165:
URL: https://github.com/apache/maven-shared-jar/pull/165
While looking at #150 I noticed a separate defect in the sibling analyzer.
## Problem
`JarFileHashAnalyzer.computeHash()` reads the cached hash into a local, and
when that is `null` it computes the SHA-1 and stores it on `JarData` — but
never assigns it back to the local:
```java
String result = jarData.getFileHash();
if (result == null) {
try (InputStream inputStream =
Files.newInputStream(jarData.getFile().toPath())) {
jarData.setFileHash(DigestUtils.sha1Hex(inputStream)); // stored
on JarData
}
...
}
return result; // still null
```
So the **first** call returns `null` even when the hash was calculated
successfully. Only a later call — served from the `JarData` cache — returns the
real value.
`JarBytecodeHashAnalyzer`, immediately beside it, does this correctly:
```java
result = Hex.encodeHexString(sha1.digest());
jarData.setBytecodeHash(result);
```
This is reachable: `RepositorySearchExposer` calls
`fileHashAnalyzer.computeHash(jarAnalyzer)` to look up a jar in the repository,
so the first lookup behaves as though the file could not be hashed at all.
## Fix
Assign the computed hash to `result` before caching it — one line, matching
what the bytecode analyzer already does.
## Tests
The class had no test at all; `JarBytecodeHashAnalyzer` is the only one of
the two that was covered, which is likely why this survived. Adds
`JarFileHashAnalyzerTest` with three cases:
| test | before | after |
|---|---|---|
| `computeHashReturnsTheHashOnFirstCall` | FAIL — `expected: not <null>` |
pass |
| `computeHashIsStableAcrossCalls` | FAIL — `expected: <null> but was:
<d2c8151…>` | pass |
| `computeHashPopulatesJarData` | FAIL — `expected: <null> but was:
<d2c8151…>` | pass |
The second failure shows the bug precisely: the first call returned `null`
while the cached second call returned the real hash.
```
before fix: Tests run: 3, Failures: 3
after fix: Tests run: 3, Failures: 0
full suite: Tests run: 81, Failures: 0, Errors: 0
```
## Note
This does not address #150 itself — the `IOException` swallowing there needs
the deprecation and new throwing method you described, which is a wider API
change. Happy to take that separately if useful; I wanted to keep this one
narrow since it is an unambiguous bug with a one-line fix.
--
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]
