This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat-jakartaee-migration.git
The following commit(s) were added to refs/heads/main by this push:
new 19fb951 Additional minor fixes from code review
19fb951 is described below
commit 19fb9517a37afa4c6171cf9604db4a1fa0ebcffa
Author: remm <[email protected]>
AuthorDate: Mon Jun 8 11:31:24 2026 +0200
Additional minor fixes from code review
Done using OpenCode.
---
CHANGES.md | 1 +
.../org/apache/tomcat/jakartaee/AntHandler.java | 9 +++-
.../org/apache/tomcat/jakartaee/CacheEntry.java | 7 ++-
.../apache/tomcat/jakartaee/ManifestConverter.java | 17 +++++--
.../org/apache/tomcat/jakartaee/Migration.java | 58 ++++++++++++++++------
.../apache/tomcat/jakartaee/MigrationCache.java | 6 ++-
.../org/apache/tomcat/jakartaee/MigrationTask.java | 11 ++--
.../tomcat/jakartaee/LocalStrings.properties | 7 +++
8 files changed, 90 insertions(+), 26 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index a509782..452d1b9 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -3,6 +3,7 @@
## 1.0.13
- Properly reconstruct the original pool in the ClassConverter transformer if
a class is found to be available. (remm)
- Do not buffer very large STORED zip entries when processing them in
streaming mode. (remm)
+- Various minor fixes from code review. (remm)
## 1.0.12
- Add Maven Wrapper Plugin to manage the Maven wrapper. (markt)
diff --git a/src/main/java/org/apache/tomcat/jakartaee/AntHandler.java
b/src/main/java/org/apache/tomcat/jakartaee/AntHandler.java
index 5946226..13fd21a 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/AntHandler.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/AntHandler.java
@@ -37,7 +37,14 @@ class AntHandler extends Handler {
@Override
public void publish(LogRecord record) {
- task.log(record.getMessage(), record.getThrown(),
toAntLevel(record.getLevel()));
+ String message = record.getMessage();
+ if (message == null && record.getThrown() != null) {
+ message = record.getThrown().toString();
+ }
+ if (message == null) {
+ message = "";
+ }
+ task.log(message, record.getThrown(), toAntLevel(record.getLevel()));
}
@Override
diff --git a/src/main/java/org/apache/tomcat/jakartaee/CacheEntry.java
b/src/main/java/org/apache/tomcat/jakartaee/CacheEntry.java
index 525f5ae..244db38 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/CacheEntry.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/CacheEntry.java
@@ -21,6 +21,8 @@ import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
+import java.util.logging.Level;
+import java.util.logging.Logger;
import org.apache.commons.io.IOUtils;
@@ -30,6 +32,7 @@ import org.apache.commons.io.IOUtils;
*/
class CacheEntry {
+ private static final Logger logger =
Logger.getLogger(CacheEntry.class.getCanonicalName());
private static final StringManager sm =
StringManager.getManager(CacheEntry.class);
private final String hash;
@@ -127,7 +130,9 @@ class CacheEntry {
}
}
if (tempFile.exists()) {
- tempFile.delete();
+ if (!tempFile.delete()) {
+ logger.log(Level.WARNING,
sm.getString("cacheEntry.rollbackDeleteFailed", tempFile));
+ }
}
}
}
diff --git a/src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java
b/src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java
index b316c41..9196c98 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java
@@ -125,9 +125,13 @@ public class ManifestConverter implements Converter {
boolean converted = false;
// Update version info
if (attributes.containsKey(Attributes.Name.IMPLEMENTATION_VERSION)) {
- String newValue =
attributes.get(Attributes.Name.IMPLEMENTATION_VERSION) + "-" +
Info.getVersion();
- attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, newValue);
- logger.log(Level.FINE,
sm.getString("manifestConverter.updatedVersion", newValue));
+ String currentVersion = (String)
attributes.get(Attributes.Name.IMPLEMENTATION_VERSION);
+ String migrationSuffix = "-" + Info.getVersion();
+ if (!currentVersion.endsWith(migrationSuffix)) {
+ String newValue = currentVersion + migrationSuffix;
+ attributes.put(Attributes.Name.IMPLEMENTATION_VERSION,
newValue);
+ logger.log(Level.FINE,
sm.getString("manifestConverter.updatedVersion", newValue));
+ }
// Purposefully avoid setting result
}
// Update package names in values
@@ -178,7 +182,12 @@ public class ManifestConverter implements Converter {
if (element.getValue().startsWith(JAKARTA_SERVLET)) {
String oldVersion =
element.getAttribute(Constants.VERSION_ATTRIBUTE);
if (oldVersion != null) {
- packages.add(element.toString().replace(oldVersion,
replacement));
+ // Only replace the version attribute, not occurrences of
the version
+ // string in other attributes or directives
+ String escaped = Pattern.quote(oldVersion);
+ String result =
element.toString().replaceFirst("(;version=\\\")" + escaped + "(\\\")",
+ "$1" + replacement + "$2");
+ packages.add(result);
} else {
packages.add(element.toString());
}
diff --git a/src/main/java/org/apache/tomcat/jakartaee/Migration.java
b/src/main/java/org/apache/tomcat/jakartaee/Migration.java
index e29c7ca..2ffdcb3 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/Migration.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/Migration.java
@@ -57,6 +57,7 @@ public class Migration {
private static final Set<String> DEFAULT_EXCLUDES = new HashSet<>();
+ private static final long TEMP_FILE_THRESHOLD = 10L * 1024 * 1024;
private static final ZipShort EXTRA_FIELD_ZIP64 = new ZipShort(1);
private static final long ZIP64_THRESHOLD_LENGTH = 0xFFFFFFFFL;
@@ -260,7 +261,7 @@ public class Migration {
throw new IOException(sm.getString("migration.mkdirError",
destination.getAbsolutePath()));
}
} else {
- // Single file`
+ // Single file
File parentDestination =
destination.getAbsoluteFile().getParentFile();
if (parentDestination.exists() || parentDestination.mkdirs()) {
migrateFile(source, destination);
@@ -272,7 +273,7 @@ public class Migration {
// Finalize cache operations (save metadata and prune expired entries)
if (cache != null) {
- cache.finalizeCacheOperations();
+ cache.pruneCache();
}
logger.log(Level.INFO, sm.getString("migration.done",
@@ -299,18 +300,36 @@ public class Migration {
private void migrateFile(File src, File dest) throws IOException {
if (src.equals(dest)) {
- ByteArrayOutputStream buffer = new
ByteArrayOutputStream(Math.toIntExact((long) (src.length() * 1.05)));
+ if (src.length() > TEMP_FILE_THRESHOLD) {
+ // For very large files, use a temp file instead of memory
+ File tempFile = File.createTempFile("jakartaee-migration-",
".tmp");
+ tempFile.deleteOnExit();
+ try (InputStream is = new FileInputStream(src); OutputStream
os = new FileOutputStream(tempFile)) {
+ if (migrateStream(src.getAbsolutePath(), is, os)) {
+ converted = true;
+ try (InputStream tempIs = new
FileInputStream(tempFile); OutputStream destOs = new FileOutputStream(dest)) {
+ Util.copy(tempIs, destOs);
+ }
+ } else {
+ return;
+ }
+ } finally {
+ tempFile.delete();
+ }
+ } else {
+ ByteArrayOutputStream buffer = new
ByteArrayOutputStream(Math.toIntExact((long) (src.length() * 1.05)));
- try (InputStream is = new FileInputStream(src)) {
- if (migrateStream(src.getAbsolutePath(), is, buffer)) {
- converted = true;
- } else {
- return;
+ try (InputStream is = new FileInputStream(src)) {
+ if (migrateStream(src.getAbsolutePath(), is, buffer)) {
+ converted = true;
+ } else {
+ return;
+ }
}
- }
- try (OutputStream os = new FileOutputStream(dest)) {
- os.write(buffer.toByteArray());
+ try (OutputStream os = new FileOutputStream(dest)) {
+ os.write(buffer.toByteArray());
+ }
}
} else {
try (InputStream is = new FileInputStream(src);
@@ -432,7 +451,7 @@ public class Migration {
logger.log(Level.INFO, sm.getString("migration.skip", name));
} else if (isArchive(name)) {
// Only cache nested archives (e.g., JARs inside WARs), not
top-level files
- // Top-level files will have absolute paths starting with "/"
+ // Top-level files will have absolute paths starting with a path
separator
boolean isNestedArchive = !name.startsWith("/") &&
!name.startsWith("\\");
CacheEntry cacheEntry = null;
@@ -536,15 +555,20 @@ public class Migration {
}
}
+ /**
+ * Output stream that tracks the CRC32 checksum and byte count of written
data.
+ * For data exceeding TEMP_FILE_THRESHOLD, automatically switches from an
in-memory
+ * buffer to a temporary file to avoid excessive memory usage. Used for
computing
+ * CRC and size of STORED zip entries during streaming migration.
+ */
private static class CrcSizeTrackingOutputStream extends OutputStream {
- private static final long TEMP_FILE_THRESHOLD = 10L * 1024 * 1024;
-
private final CRC32 crc = new CRC32();
private long size;
private ByteArrayOutputStream buffer = new ByteArrayOutputStream();
private FileOutputStream fileOutput;
private File tempFile;
+ private volatile boolean tempFileDeleted = false;
@Override
public void write(int b) throws IOException {
@@ -596,6 +620,10 @@ public class Migration {
try (FileInputStream fis = new FileInputStream(tempFile)) {
IOUtils.copy(fis, out);
}
+ if (tempFile != null && tempFile.exists()) {
+ tempFile.delete();
+ tempFileDeleted = true;
+ }
} else if (buffer != null) {
buffer.writeTo(out);
}
@@ -604,7 +632,7 @@ public class Migration {
public void close() throws IOException {
if (fileOutput != null) {
fileOutput.close();
- if (tempFile != null && tempFile.exists()) {
+ if (!tempFileDeleted && tempFile != null && tempFile.exists())
{
tempFile.delete();
}
}
diff --git a/src/main/java/org/apache/tomcat/jakartaee/MigrationCache.java
b/src/main/java/org/apache/tomcat/jakartaee/MigrationCache.java
index 1af6070..46d8a58 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/MigrationCache.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/MigrationCache.java
@@ -311,7 +311,10 @@ public class MigrationCache {
*/
public void clear() throws IOException {
deleteDirectory(cacheDir);
- cacheDir.mkdirs();
+ cacheMetadata.clear();
+ if (!cacheDir.mkdirs()) {
+ throw new IOException(sm.getString("cache.cannotCreate",
cacheDir.getAbsolutePath()));
+ }
logger.log(Level.INFO, sm.getString("cache.cleared"));
}
@@ -422,6 +425,7 @@ public class MigrationCache {
*
* @throws IOException if an I/O error occurs
*/
+ @Deprecated
public void finalizeCacheOperations() throws IOException {
// Save updated metadata
saveMetadata();
diff --git a/src/main/java/org/apache/tomcat/jakartaee/MigrationTask.java
b/src/main/java/org/apache/tomcat/jakartaee/MigrationTask.java
index c39ce09..02ecec7 100644
--- a/src/main/java/org/apache/tomcat/jakartaee/MigrationTask.java
+++ b/src/main/java/org/apache/tomcat/jakartaee/MigrationTask.java
@@ -18,6 +18,7 @@ package org.apache.tomcat.jakartaee;
import java.io.File;
import java.io.IOException;
+import java.util.Locale;
import java.util.logging.Handler;
import java.util.logging.Logger;
@@ -29,6 +30,8 @@ import org.apache.tools.ant.Task;
*/
public class MigrationTask extends Task {
+ private static final StringManager sm =
StringManager.getManager(MigrationTask.class);
+
/**
* Default constructor.
*/
@@ -104,15 +107,15 @@ public class MigrationTask extends Task {
// check the parameters
EESpecProfile profile = null;
try {
- profile = EESpecProfiles.valueOf(this.profile.toUpperCase());
+ profile =
EESpecProfiles.valueOf(this.profile.toUpperCase(Locale.ENGLISH));
} catch (IllegalArgumentException e) {
- throw new BuildException("Invalid profile specified: " +
this.profile, getLocation()); // todo i18n
+ throw new
BuildException(sm.getString("migrationTask.invalidProfile", this.profile),
getLocation());
}
if (src == null || !src.exists() || !src.canRead()) {
- throw new BuildException("Invalid or missing source parameter: " +
src); // todo i18n
+ throw new BuildException(sm.getString("migrationTask.noSource",
src));
}
if (dest == null) {
- throw new BuildException("Missing destination parameter"); // todo
i18n
+ throw new BuildException(sm.getString("migrationTask.noDest"));
}
Migration migration = new Migration();
diff --git
a/src/main/resources/org/apache/tomcat/jakartaee/LocalStrings.properties
b/src/main/resources/org/apache/tomcat/jakartaee/LocalStrings.properties
index e87a484..91c8059 100644
--- a/src/main/resources/org/apache/tomcat/jakartaee/LocalStrings.properties
+++ b/src/main/resources/org/apache/tomcat/jakartaee/LocalStrings.properties
@@ -13,6 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+cacheEntry.rollbackDeleteFailed=Failed to delete temporary cache file [{0}]
during rollback
+
classConverter.converted=Migrated class [{0}]
classConverter.noConversion=No conversion necessary for [{0}]
classConverter.skipName=Skip conversion of class usage from the [{0}]
namespace to [{1}] as it is not accessible to the classloader
@@ -69,6 +71,10 @@ where options includes:\n\
migration.warnSignatureRemoval=Removed cryptographic signature from JAR file
+migrationTask.invalidProfile=Specified profile [{0}] is invalid
+migrationTask.noDest=No destination parameter specified
+migrationTask.noSource=Invalid or missing source [{0}] specified
+
passThroughConverter.noConversion=No conversion necessary for [{0}]
textConverter.converted=Migrated text file [{0}]
@@ -81,6 +87,7 @@ manifestConverter.removeSignature=Remove cryptographic
signature for [{0}]
manifestConverter.noConversion=No manifest conversion necessary for [{0}]
cache.cannotCreate=Cannot create cache directory [{0}]
+cache.deleteFailed=Failed to delete [{0}]
cache.notDirectory=[{0}] is not a directory
cache.nullDirectory=The cache storage directory may not be null
cache.enabled=Migration cache enabled at [{0}] with {1} day retention period
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
