jbonofre commented on code in PR #2649:
URL: https://github.com/apache/karaf/pull/2649#discussion_r3559862790
##########
features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java:
##########
@@ -75,9 +71,8 @@ protected File download(Exception previousExceptionNotUsed)
throws Exception {
File tmpFile = Files.createTempFile(dir.toPath(), "download-",
null).toFile();
urlObj = new
URL(DownloadManagerHelper.stripStartLevel(urlObj.toString()));
- try (InputStream is = urlObj.openStream();
- OutputStream os = new FileOutputStream(tmpFile)) {
- StreamUtils.copy(is, os);
+ try (var is = urlObj.openStream()) {
+ Files.copy(is, tmpFile.toPath());
Review Comment:
I think this will always throws `FileAlreadyExistsException` because
`Files.creqteTempFile()` creates the file before the copy is attempted.
`REPLACE_EXISTING` is required here.
##########
features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java:
##########
@@ -112,10 +107,8 @@ protected File downloadBlueprintOrSpring() throws
Exception {
File dir = new File(System.getProperty("karaf.data"), "tmp");
dir.mkdirs();
File tmpFile = Files.createTempFile(dir.toPath(), "download-",
null).toFile();
- try (InputStream is = new URL(url).openStream();
- OutputStream os = new FileOutputStream(tmpFile))
- {
- StreamUtils.copy(is, os);
+ try (var is = new URL(url).openStream()) {
+ Files.copy(is, tmpFile.toPath());
Review Comment:
I think this will always throws `FileAlreadyExistsException` because
`Files.creqteTempFile()` creates the file before the copy is attempted.
`REPLACE_EXISTING` is required here.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java:
##########
@@ -298,21 +302,15 @@ private void installConfigurationFile(String
fileLocation, String finalname, boo
}
// TODO: use download manager to download the configuration
- try (
- InputStream is = new BufferedInputStream(new
URL(fileLocation).openStream())
- ) {
+ try (var is = new URL(fileLocation).openStream()) {
if (!file.exists()) {
File parentFile = file.getParentFile();
if (parentFile != null) {
parentFile.mkdirs();
}
file.createNewFile();
}
- try (
- FileOutputStream fop = new FileOutputStream(file)
- ) {
- StreamUtils.copy(is, fop);
- }
+ Files.copy(is, file.toPath());
Review Comment:
Without `REPLACE_EXISTING`, it throws when `override=true` and the config
file already exists. The old `FileOutputStream` silently overwrote. With
`Files.copy()` will always fail the override path. The same happens in
`InstallCommand` and `ConfigMBeanImpl`.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java:
##########
@@ -173,12 +181,8 @@ public void installFeatureConfigs(Feature feature) throws
IOException, InvalidSy
}
}
- private String loadConfiguration(final URL url) throws IOException {
- try (final InputStream inputStream = new
BufferedInputStream(url.openStream())) {
- final ByteArrayOutputStream outputStream = new
ByteArrayOutputStream();
- StreamUtils.copy(inputStream, outputStream);
- return new String(outputStream.toByteArray(),
StandardCharsets.UTF_8);
- }
+ private static String loadConfiguration(final URL url) throws IOException {
+ return new String(url.openStream().readAllBytes(),
StandardCharsets.UTF_8);
Review Comment:
Here, we potentially leaks the URL stream if `readAllBytes()` throws
mid-read. Previously, we used a proper try-with-resources approach.
##########
kar/src/main/java/org/apache/karaf/kar/internal/Kar.java:
##########
@@ -161,9 +160,7 @@ private static File extract(InputStream is, ZipEntry
zipEntry, File dest) throws
dest.mkdirs();
} else {
dest.getParentFile().mkdirs();
- FileOutputStream out = new FileOutputStream(dest);
- StreamUtils.copy(is, out);
- out.close();
+ Files.copy(is, dest.toPath());
Review Comment:
I think `REPLACE_EXISTING` is required here. On KAR re-install or upgrade,
previously extracted files exist, causing `FileAlreadyExistsException` and
leaving the KAR partially extracted.
--
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]
