You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "kratos0718 (via GitHub)" <gi...@apache.org> on 2026/08/14 14:47:05 UTC
[PR] Return the file hash that was just computed instead of null [maven-shared-jar]
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return the file hash that was just computed instead of null [maven-shared-jar]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz merged PR #165:
URL: https://github.com/apache/maven-shared-jar/pull/165
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return the file hash that was just computed instead of null [maven-shared-jar]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #165:
URL: https://github.com/apache/maven-shared-jar/pull/165#discussion_r3839701449
##########
src/test/java/org/apache/maven/shared/jar/identification/hash/JarFileHashAnalyzerTest.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.maven.shared.jar.identification.hash;
+
+import java.io.InputStream;
+import java.nio.file.Files;
+
+import org.apache.commons.codec.digest.DigestUtils;
+import org.apache.maven.shared.jar.AbstractJarAnalyzerTestCase;
+import org.apache.maven.shared.jar.JarAnalyzer;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+class JarFileHashAnalyzerTest extends AbstractJarAnalyzerTestCase {
+
+ private final JarFileHashAnalyzer analyzer = new JarFileHashAnalyzer();
+
+ @Test
+ void computeHashReturnsTheHashOnFirstCall() throws Exception {
+ JarAnalyzer jarAnalyzer = new JarAnalyzer(getSampleJar("codec.jar"));
+ try {
+ String expected;
Review Comment:
don';t split variable declaration and initialization; move this into the try block
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return the file hash that was just computed instead of null [maven-shared-jar]
Posted by "kratos0718 (via GitHub)" <gi...@apache.org>.
kratos0718 commented on PR #165:
URL: https://github.com/apache/maven-shared-jar/pull/165#issuecomment-5390321742
Done. The outer try was only there to hold the split declaration, so folding the stream into the existing try-with-resources removes a nesting level as well. `mvn test` is 81 passing.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return the file hash that was just computed instead of null [maven-shared-jar]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #165:
URL: https://github.com/apache/maven-shared-jar/pull/165#discussion_r3843115246
##########
src/main/java/org/apache/maven/shared/jar/identification/hash/JarFileHashAnalyzer.java:
##########
@@ -48,7 +48,8 @@ public String computeHash(JarAnalyzer jarAnalyzer) {
try {
Review Comment:
you only need one try block 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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org