372 storage multiple upload

[RawanAbdelkhalek]

SEMOSS/community#372
Closes SEMOSS/community#372

[github-actions]

@CodiumAI-Agent /describe

[github-actions]

@CodiumAI-Agent /review

[github-actions]

@CodiumAI-Agent /improve

[QodoAI-Agent]

PR Code Suggestions ✨

Latest suggestions up to 94fb986

Category	Suggestion	Impact
Possible issue	Skip nonexistent file uploads Avoid uploading files that were already marked as missing. Add a continue after logging missing files to prevent passing invalid paths to copyToStorage. This prevents potential exceptions and incorrect behavior. src/prerna/reactor/storage/PushToStorageReactor.java [47-57] for (String fileLocation : fileLocations) { fileLocation = Utility.normalizePath(fileLocation); if (!new File(fileLocation).exists()) { - failedFiles.add(fileLocation); - classLogger.error("Failed to upload file: " + fileLocation); + failedFiles.add(fileLocation); + classLogger.error("Failed to upload file (not found): " + fileLocation); + continue; } storage.copyToStorage(fileLocation, storageFolderPath, metadata); } Suggestion importance[1-10]: 8 __ Why: Correctly identifies that missing files are still passed to copyToStorage, proposing a continue to avoid errors and invalid uploads. This is accurate to the PR code and prevents potential exceptions, improving robustness.	Medium
	Use try-with-resources for streams Ensure streams are always closed even if an exception occurs during zipping by using try-with-resources. This prevents file handle leaks and partially written archives on error. src/prerna/reactor/util/ZipFilesReactor.java [82-95] -try { - FileOutputStream fos = new FileOutputStream(zipFile); - ZipOutputStream zos = new ZipOutputStream(fos); - +try (FileOutputStream fos = new FileOutputStream(zipFile); + ZipOutputStream zos = new ZipOutputStream(fos)) { for (File file : filesToZip) { ZipUtils.addToZipFile(file, zos); } - - zos.close(); - fos.close(); - } catch (IOException e) { throw new IllegalArgumentException("Unable to zip files. Detailed error = " + e.getMessage()); } Suggestion importance[1-10]: 7 __ Why: Switching to try-with-resources ensures streams close on exceptions, preventing leaks; this directly applies to the shown code block. It's a solid maintainability and reliability improvement, though not a critical bug fix.	Medium
	Reinstate folder prefix slash Restoring the trailing slash for folder prefixes avoids unintentionally matching similarly prefixed keys (e.g., "abc1" when listing "abc"). Re-enable appending "/" when prefix is non-empty to correctly scope listings to the desired folder. src/prerna/engine/impl/storage/AWSNativeBlobStorageEngine.java [383-387] if (!prefix.endsWith("/") && !prefix.isEmpty()) { -// prefix += "/"; + prefix += "/"; } -ListObjectsV2Request request = ListObjectsV2Request.builder().bucket(this.bucket).prefix(prefix) - .build(); +ListObjectsV2Request request = ListObjectsV2Request.builder() + .bucket(this.bucket) + .prefix(prefix) + .build(); Suggestion importance[1-10]: 6 __ Why: Re-adding the trailing slash can avoid overbroad S3 listings and aligns with typical folder prefix handling. However, the PR intentionally commented this out, so context may justify the change; impact is moderate but not definitively required.	Low

---

Previous suggestions

Suggestions up to commit 6d3fd84

Category	Suggestion	Impact
Possible issue	Skip and collect per-file upload failures Add a continue after recording a missing file to avoid attempting upload on a non-existent path. Also wrap each upload in a try/catch to continue processing remaining files and capture failures without aborting the whole batch. src/prerna/reactor/storage/PushToStorageReactor.java [47-57] for (String fileLocation : fileLocations) { fileLocation = Utility.normalizePath(fileLocation); - if (!new File(fileLocation).exists()) { - failedFiles.add(fileLocation); - classLogger.error("Failed to upload file: " + fileLocation); + File f = new File(fileLocation); + if (!f.exists()) { + failedFiles.add(fileLocation); + classLogger.error("Failed to upload file (not found): " + fileLocation); + continue; } - storage.copyToStorage(fileLocation, storageFolderPath, metadata); + try { + storage.copyToStorage(fileLocation, storageFolderPath, metadata); + } catch (Exception ex) { + failedFiles.add(fileLocation); + classLogger.error("Failed to upload file: " + fileLocation, ex); + } } Suggestion importance[1-10]: 8 __ Why: Correctly adds a continue to avoid uploading nonexistent files and wraps each upload in try/catch, improving robustness without changing behavior elsewhere. The 'existing_code' matches the new hunk loop and the improved_code accurately reflects the suggested change.	Medium
General	Ensure streams close reliably Use try-with-resources to ensure ZipOutputStream and FileOutputStream are always closed, even if an exception occurs while zipping. This prevents resource leaks and partial file locks. src/prerna/reactor/util/ZipFilesReactor.java [82-95] -try { - FileOutputStream fos = new FileOutputStream(zipFile); - ZipOutputStream zos = new ZipOutputStream(fos); - +try (FileOutputStream fos = new FileOutputStream(zipFile); + ZipOutputStream zos = new ZipOutputStream(fos)) { for (File file : filesToZip) { ZipUtils.addToZipFile(file, zos); } - - zos.close(); - fos.close(); - } catch (IOException e) { throw new IllegalArgumentException("Unable to zip files. Detailed error = " + e.getMessage()); } Suggestion importance[1-10]: 7 __ Why: Replacing manual close with try-with-resources prevents resource leaks and is a safe, direct improvement. The existing_code maps to the try block in the new hunk, and the improved_code correctly implements try-with-resources.	Medium
	Validate inputs and consistent return Validate that fileLocations is not empty before iterating, and return a clear error if so. Also return a boolean success when there are no failures to preserve previous contract, and include failures only when applicable. src/prerna/reactor/storage/PushToStorageReactor.java [48-58] +if (fileLocations == null || fileLocations.isEmpty()) { + throw new IllegalArgumentException("No files provided for upload"); +} try { - List<String> failedFiles = new ArrayList<>(); + List<String> failedFiles = new ArrayList<>(); for (String fileLocation : fileLocations) { - ... - storage.copyToStorage(fileLocation, storageFolderPath, metadata); + // ... per-file handling as above + } + if (failedFiles.isEmpty()) { + return new NounMetadata(true, PixelDataType.BOOLEAN); } return new NounMetadata(failedFiles, PixelDataType.VECTOR); } catch (Exception e) { classLogger.error(Constants.STACKTRACE, e); throw new IllegalArgumentException("Error occurred uploading local files to storage"); } Suggestion importance[1-10]: 6 __ Why: Input validation for an empty 'fileLocations' list is useful; adjusting return type to boolean on full success is reasonable but changes the current PR's API (which returns a vector), so impact is moderate. The code context matches, and the improved_code is coherent.	Low

[themaherkhalil]

these were not merged properly - getNoun is deprecated for getGenRowStruct. This exists in other files as well, but most prevalent in this one.

[RawanAbdelkhalek]

@themaherkhalil merge fixed