feat: Add dynamic file type support for catalog uploads (CSV, JSON, JSONL) additional changes

[esezen]

No description provided.

[Copilot]

Pull request overview

This PR adds support for multiple file formats (CSV, JSON, JSONL) for catalog uploads in the Constructor.io client. Previously, only CSV format was supported; now the client dynamically detects and validates file extensions to accept JSON and JSONL formats as well.

Key Changes

• File extension validation for catalog uploads now supports .csv, .json, and .jsonl extensions
• Test utilities were added to generate temporary JSON/JSONL files for comprehensive test coverage
• Updated catalog operation methods with improved documentation reflecting multi-format support

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
Utils.java	Added helper methods to create temporary JSON/JSONL test files and invalid files for validation testing
ConstructorIOCatalogTest.java	Added extensive test coverage for JSON/JSONL formats, validation edge cases, and mixed file type scenarios
ConstructorIO.java	Implemented file extension validation logic and updated catalog methods to support multiple formats

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test method has the same name as the test at line 509 (UpdateCatalogWithNoExtensionShouldError), but its implementation mirrors the test at line 492 (ReplaceCatalogWithInvalidExtensionShouldError). The test should be renamed to clearly indicate it tests updateCatalog with invalid extension, differentiating it from the other tests.

[constructor-claude-bedrock]

Code Review Summary

This PR adds dynamic file type support for catalog uploads (CSV, JSON, JSONL) to the Constructor.io Java client. The implementation is solid and well-tested, with comprehensive test coverage for various scenarios. The code demonstrates good defensive programming practices with proper validation and error handling.

Detailed Feedback

Strengths

✅ Excellent validation logic - The getValidatedFileExtension method properly validates null files, missing extensions, and invalid file types with clear error messages.

✅ Comprehensive test coverage - 18 new test cases covering:

• Happy paths for JSON and JSONL formats
• Invalid extensions and edge cases
• Null file handling
• Mixed file type scenarios

✅ Backward compatibility - CSV files continue to work as before, with JSON/JSONL added seamlessly.

✅ Clear documentation - Javadoc comments properly document the new functionality and supported formats.

---

Issues and Recommendations

[File: constructorio-client/src/test/java/io/constructor/client/Utils.java Line: L271]

🔴 Critical: Incomplete code - Missing method body

The createNoExtensionFile() method appears to be truncated in the diff. The method body is incomplete:

public static File createNoExtensionFile() throws IOException {
    File file = File.createTempFile("items", "");
    file.deleteOnExit();

    try (FileWriter writer = new FileWriter(file)) {
        writer.write("This is a file with no extension for catalog upload testing.");
    }
    return file;
}

Action required: Verify the complete implementation was committed.

---

[File: constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java Line: L37-L38]

💡 Minor: Consider Arrays.asList vs explicit LinkedHashSet

The use of LinkedHashSet initialized from Arrays.asList is technically unnecessary here since order doesn't matter for validation and Sets don't preserve insertion order for iteration in this context.

Current:

private static final Set<String> VALID_CATALOG_EXTENSIONS =
        new LinkedHashSet<>(Arrays.asList(".csv", ".json", ".jsonl"));

Simpler alternative:

private static final Set<String> VALID_CATALOG_EXTENSIONS =
        new HashSet<>(Arrays.asList(".csv", ".json", ".jsonl"));

Or if ordering in error messages is important (which it appears to be for consistency), the current approach is fine. Just note that LinkedHashSet preserves insertion order, which is useful for predictable error messages.

---

[File: constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java Line: L2082]

⚠️ Medium: Case sensitivity consideration

The extension is converted to lowercase for comparison:

String extension = actualFileName.substring(lastDotIndex).toLowerCase();

This is good for case-insensitive matching, but consider documenting this behavior in the Javadoc so users know that .CSV, .Json, and .JSONL are all valid.

Suggestion: Add to the method Javadoc:

* Note: Extension matching is case-insensitive (.CSV, .csv, .Csv are all valid)

---

[File: constructorio-client/src/test/java/io/constructor/client/Utils.java Line: L121]

📝 Code style: Diamond operator

Minor style improvement - use diamond operator for better readability:

Current:

List<Map<String, Object>> items = new ArrayList<Map<String, Object>>();

Recommended:

List<Map<String, Object>> items = new ArrayList<>();

This applies to multiple locations in the file (lines 121, 138, 161, 184, 208, etc.).

---

[File: constructorio-client/src/test/java/io/constructor/client/ConstructorIOCatalogTest.java Line: L62-L79]

💡 Test cleanup: Potential resource leak prevention

The cleanup logic is good, but consider using a more defensive approach that won't fail if any file is null:

@After
public void teardown() throws Exception {
    deleteFileIfExists(itemsFile);
    deleteFileIfExists(variationsFile);
    deleteFileIfExists(itemGroupsFile);
    deleteFileIfExists(jsonItemsFile);
    deleteFileIfExists(jsonVariationsFile);
    deleteFileIfExists(jsonlItemsFile);
    deleteFileIfExists(jsonlVariationsFile);
    deleteFileIfExists(jsonlItemGroupsFile);
    deleteFileIfExists(invalidExtensionFile);
    deleteFileIfExists(noExtensionFile);
    deleteFileIfExists(csvFolder);
}

private void deleteFileIfExists(File file) {
    if (file != null && file.exists()) {
        file.delete();
    }
}

This is more robust if any test setup fails partway through.

---

[General]

📊 Test coverage suggestion

Consider adding tests for:

1. Files with multiple dots in the name (e.g., items.backup.csv)
2. Files with uppercase extensions (e.g., items.CSV) to explicitly verify case-insensitive handling
3. Edge case: Empty files with valid extensions

These would further strengthen the validation logic.

---

[General]

📖 Documentation opportunity

Consider updating the README or adding a code example showing how to use the new JSON/JSONL format support. This would help users understand the feature without diving into the code.

Conclusion

This is a well-implemented feature with strong validation logic and comprehensive test coverage. The code is production-ready with only minor improvements suggested. The most critical item is verifying that the createNoExtensionFile() method is complete.

Recommendation: Approve with minor fixes ✅

Great work on the thorough testing and defensive validation!

[constructor-claude-bedrock]

Code Review Summary

This PR adds dynamic file type support for catalog uploads, expanding from CSV-only to support CSV, JSON, and JSONL formats. The implementation is well-structured with a new validation method, comprehensive test coverage, and clear documentation. The changes maintain backward compatibility while introducing the new functionality cleanly.

Overall Assessment: The code quality is high, but there are a few important issues that should be addressed before merging.

---

Critical Issues

[File: ConstructorIO.java Line: L38] - LinkedHashSet Import Usage

• Issue: The code imports LinkedHashSet but uses it unnecessarily. A LinkedHashSet is used to preserve insertion order, but for a static constant set of file extensions, ordering is not functionally important.
• Why it matters: This adds unnecessary complexity. A regular HashSet would be more efficient for the use case (set membership testing).
• Recommendation: Consider changing to HashSet or using Set.of() if Java 9+ is available.

[File: ConstructorIO.java Line: L2070] - Lowercase Extension Not Preserved in Multipart Upload

• Issue: The getValidatedFileExtension method returns a lowercase extension (line 2071), but this lowercase version is then used as the filename in the multipart upload. For a file named items.CSV, the upload would be sent as items.csv.
• Why it matters: While many systems are case-insensitive, this could potentially cause issues if the server expects the original case to be preserved, or make logging/debugging confusing when filenames don't match.
• Recommendation: Either document this behavior in the method JavaDoc that extensions are normalized to lowercase, or preserve the original case and only use lowercase for validation comparison.

[File: Utils.java Line: L122] - Gson Instance as Static Field Without Thread-Safety Consideration

• Issue: A static Gson instance is created without documentation of thread-safety assumptions.
• Why it matters: While Gson instances are generally thread-safe, it is good practice to document thread-safety assumptions for static objects in test utilities.
• Recommendation: Add a comment documenting that Gson instances are thread-safe.

---

Minor Issues and Suggestions

[File: ConstructorIO.java Line: L2050] - Redundant Null Check

• Observation: The method checks if actualFileName is null, but File.getName() will never return null for a valid File object.
• Recommendation: The null check is defensive programming which is fine, but consider if isEmpty() alone would suffice.

[File: Utils.java Lines: L123-L167] - Code Duplication in File Creation Methods

• Observation: The methods createItemsJsonFile, createItemsJsonlFile, createVariationsJsonFile, and createVariationsJsonlFile have very similar structure.
• Recommendation: Consider extracting common logic into a generic helper method to reduce duplication. This is not critical but would improve maintainability.

[File: ConstructorIOCatalogTest.java Line: L172] - Mixed Format Test Uses Hardcoded Path

• Observation: The test ReplaceCatalogWithMixedFileTypesShouldSucceed uses a hardcoded path instead of the itemsFile field.
• Why it matters: This breaks consistency with other tests and could fail if the path structure changes.
• Recommendation: Use the itemsFile field that is already set up in the Before method.

---

Positive Observations

1. Excellent Test Coverage: The PR includes comprehensive tests covering JSON format uploads, JSONL format uploads, invalid extensions, null file handling, mixed format uploads, and all three catalog operations (replace, update, patch).

2. Good Error Messages: The validation method provides clear, actionable error messages that include the expected extensions and what was found.

3. Backward Compatibility: The changes are fully backward compatible - existing CSV uploads will continue to work without any code changes.

4. Documentation: JavaDoc comments have been updated appropriately to reflect the new functionality.

5. Clean Validation Logic: The getValidatedFileExtension method is well-structured with clear validation steps and appropriate error handling.

---

Testing Recommendations

1. Edge Cases: Consider adding tests for files with multiple dots in the name, files with uppercase extensions, and very long filenames.

2. Integration Testing: Verify that the actual API endpoints accept these new formats (if not already done).

---

Conclusion

This is a solid implementation that adds valuable functionality to the library. The core logic is sound, test coverage is comprehensive, and the code is generally well-written. Please address the critical issues around LinkedHashSet usage and the lowercase extension behavior, and consider the minor suggestions for code quality improvements.

Once these issues are addressed, this PR will be ready to merge. Great work on maintaining backward compatibility and providing thorough test coverage!

[Alexey-Pavlov]

LGTM!

[constructor-claude-bedrock]

Code Review Summary

This PR adds dynamic file type support for catalog uploads, extending the existing CSV-only implementation to support JSON and JSONL formats. The implementation introduces validation logic for file extensions and adds comprehensive test coverage. Overall, the code is well-structured and thoroughly tested.

Detailed Feedback

[File: constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java]

Positive:

• The use of LinkedHashSet for VALID_CATALOG_EXTENSIONS ensures insertion order is maintained (which gives predictable error messages) and guarantees no duplicates.
• The getValidatedFileExtension method is well-documented with clear Javadoc explaining parameters, return values, and exceptions.
• Error messages are informative and include the actual file name and valid extensions.
• The validation logic correctly handles edge cases like null files, missing extensions, and files ending with a dot.

Suggestions:

1. [Line: 2036] Consider extracting validation logic to reduce duplication:
   The validation logic checks for null, empty filename, and missing extensions. Consider using a more defensive approach by checking if the extension is at the start of the filename:

   if (lastDotIndex == -1 || lastDotIndex == 0) {
       // Extension at position 0 would mean filename starts with a dot (hidden files)
   }

   Currently, files like .csv (hidden files on Unix systems) would pass validation, which might not be intended.

2. [Line: 2036] The error message construction uses string concatenation with VALID_CATALOG_EXTENSIONS. Consider using a more readable format:

   "Invalid file type for '" + fileName + "': file must have one of " 
       + String.join(", ", VALID_CATALOG_EXTENSIONS) + " extension."

   This would show ".csv, .json, .jsonl" instead of "[.csv, .json, .jsonl]".

3. [Line: 2071] The extension is converted to lowercase which is good for case-insensitive matching. However, consider documenting this behavior in the method Javadoc to clarify that extensions are case-insensitive.

4. [General] The MediaType for all file uploads is set to application/octet-stream. Consider using more specific MIME types:

   • .csv → text/csv
   • .json → application/json
   • .jsonl → application/x-ndjson or application/jsonl

   This would improve HTTP semantics, though it may not affect functionality if the server doesn't rely on Content-Type.

[File: constructorio-client/src/test/java/io/constructor/client/ConstructorIOCatalogTest.java]

Positive:

• Excellent test coverage including edge cases (null files, missing extensions, invalid extensions).
• Tests for all three operations (replace, update, patch) with all three formats (CSV, JSON, JSONL).
• Mixed format tests ensure flexibility in real-world scenarios.
• Proper cleanup in @After method prevents resource leaks.
• Good use of ExpectedException rule for validating error messages.

Suggestions:

1. [Line: 48-58] The test setup downloads CSV files from a remote URL in @Before. Consider adding error handling or using local test resources:

   • Remote dependencies can cause tests to fail if the network is unavailable or the URL changes.
   • This adds latency to test execution.
   • Consider committing test CSV files to the repository or mocking the HTTP calls.

2. [General] The constant PRODUCTS_SECTION is a good refactoring, but several tests still pass the section parameter even when it's the default. Consider creating helper methods to reduce duplication:

   private CatalogRequest createRequest(Map<String, File> files) {
       return new CatalogRequest(files, PRODUCTS_SECTION);
   }

3. [Lines: 436-482] The JSONL tests are well-structured, but they're essentially duplicates of the CSV tests. Consider using parameterized tests (JUnit's @Parameterized or @ParameterizedTest) to reduce code duplication:

   @ParameterizedTest
   @MethodSource("fileProvider")
   public void replaceCatalogWithItemsFileShouldReturnTaskInfo(File itemsFile) throws Exception {
       // Test logic
   }

4. [Line: 474] Test name convention: Consider using the naming pattern operationType_FileType_ShouldReturnSuccess for better organization:

   • replaceCatalog_JsonlItems_ShouldReturnTaskInfo
   • updateCatalog_InvalidExtension_ShouldThrowException

[File: constructorio-client/src/test/java/io/constructor/client/Utils.java]

Positive:

• Excellent helper methods for generating test data.
• Proper use of File.createTempFile and file.deleteOnExit() for temporary files.
• Good Javadoc documentation for all new methods.
• Realistic test data using existing createProductItem() and createProductVariation() methods.

Issues:

1. [Line: 77] Bug - Duplicate method call:

   variation.setUrl(url);
   variation.setUrl(url);  // Duplicate call

   This appears to be a pre-existing bug (not introduced by this PR), but worth fixing.

Suggestions:

2. [Line: 134, 158, etc.] Using deleteOnExit() is good for cleanup, but it only runs on JVM exit. Since the test class manually deletes files in @After, consider whether deleteOnExit() is necessary:

   • If tests fail before reaching @After, files will accumulate until JVM exit.
   • Consider documenting that both mechanisms are intentional for redundancy.

3. [Lines: 134-146] The JSON file creation wraps items in an array. Ensure this matches the expected API format. If the API expects a specific JSON structure (e.g., {"items": [...]}), document this requirement.

4. [Line: 307] In createNoExtensionFile(), consider using a more deterministic naming scheme for test debugging:

   File file = new File(tmpDir, "items_no_extension_" + System.currentTimeMillis());

General Observations

1. Backward Compatibility: The changes are backward compatible since CSV remains supported. Existing clients will continue to work without modification. ✅

2. Thread Safety: The static VALID_CATALOG_EXTENSIONS set is immutable after initialization, which is good for thread safety.

3. Documentation: The updated Javadoc in the catalog methods clearly documents the new format support. Consider also updating the README.md to show examples with JSON/JSONL files.

4. Error Handling: Validation happens early and provides clear error messages. This will help developers quickly identify issues.

Conclusion

This is a solid implementation with excellent test coverage. The code follows good practices and maintains backward compatibility. The main suggestions are minor improvements around code duplication in tests, MIME type specificity, and the duplicate setUrl() bug fix in Utils.java.

Recommendation: Approve with minor suggestions for improvement.

Great work on the comprehensive testing! 🎉

[Alexey-Pavlov]

Thank you!