FileExchangeStore YAML Example #2526
Conversation
- Add file-based exchange store example in YAML for Membrane. - Update test utilities with better readability, runtime optimizations, and exception formatting using `.formatted()`. - Enhance tests for file exchange store functionality and directory cleanup. - Refine test setup with `@BeforeEach`, improve logging, and make helper methods private for encapsulation. - General cleanup of unused imports and redundant code.
📝 WalkthroughWalkthroughAdded a new file-exchangestore example config, modified the custom-interceptor YAML, and refactored test utilities and a file-exchange-store test (visibility reductions, helper methods, minor timing/formatting tweaks). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java (2)
90-100: Consider usingFiles.createTempDirectoryfor safer temp directory creation.The current approach (create temp file → delete → create directory) has a potential race condition and doesn't check the results of
delete()andmkdirs(). Consider usingFiles.createTempDirectory()instead, which atomically creates a unique temporary directory.🔎 Proposed improvement
-private static @NotNull File createTempFolder() throws IOException { - File t = createTempFile("fes", null); - - //noinspection ResultOfMethodCallIgnored - t.delete(); - - File base = new File(t, "FileExchangeStoreTest"); - //noinspection ResultOfMethodCallIgnored - base.mkdirs(); - return base; -} +private static @NotNull File createTempFolder() throws IOException { + Path tempDir = Files.createTempDirectory("fes"); + File base = new File(tempDir.toFile(), "FileExchangeStoreTest"); + if (!base.mkdirs()) { + throw new IOException("Failed to create directory: " + base.getAbsolutePath()); + } + return base; +}
102-116: Good refactoring with helpful documentation.Extracting this helper method improves test readability. The JavaDoc clearly explains its purpose. Using
formatted()for path construction is a good modernization.Optionally, consider checking the
mkdirs()result to catch failures early:-//noinspection ResultOfMethodCallIgnored -new File(base, "2015/%d/%d".formatted(m, d)).mkdirs(); +File dir = new File(base, "2015/%d/%d".formatted(m, d)); +if (!dir.mkdirs() && !dir.exists()) { + throw new RuntimeException("Failed to create directory: " + dir); +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
distribution/examples/extending-membrane/custom-interceptor/apis.yamldistribution/examples/extending-membrane/file-exchangestore/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.javadistribution/src/test/java/com/predic8/membrane/examples/util/Process2.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (14)
distribution/examples/extending-membrane/custom-interceptor/apis.yaml (2)
1-2: LGTM!Adding the YAML language server schema directive improves IDE support and enables automatic validation of the configuration file.
4-6: Inconsistency between summary and code.The AI summary claims the class specification was removed from the myInterceptor bean, but line 6 still shows
class: com.predic8.MyInterceptor. The code and summary are inconsistent.distribution/src/test/java/com/predic8/membrane/examples/util/Process2.java (3)
80-80: LGTM!Using
String.formatted()improves code readability and maintainability for the platform-specific command construction.
159-159: LGTM!Including the absolute path in the error message improves debuggability when the example directory is missing.
182-182: Verify the reduced sleep duration is sufficient.The post-startup sleep was reduced from 100ms to 50ms. While this improves test performance, ensure this change doesn't introduce flakiness on slower CI environments, especially since this sleep occurs after the explicit startup wait mechanism.
distribution/examples/extending-membrane/file-exchangestore/apis.yaml (2)
1-8: LGTM!The file exchange store component configuration is clear and well-documented. The relative path
./exchangesis appropriate for an example, and the explanatory comments are helpful.
11-14: LGTM!The API configuration is straightforward and correct. The proxy setup forwards requests from localhost:2000 to the external API endpoint.
distribution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.java (3)
141-141: LGTM!Reducing the visibility of the
unziphelper method toprivateimproves encapsulation since it's only used internally within this class.
165-165: LGTM!Reducing the visibility of the
copyInputStreamhelper method toprivateimproves encapsulation since it's only used internally.
180-180: LGTM!Using
String.formatted()and including the absolute path improves error message clarity and debuggability.distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java (4)
21-21: LGTM!Adding the
@NotNullannotation import supports better null-safety documentation for the new helper method.
44-44: LGTM!JUnit 5 test methods don't require
publicvisibility, so removing it is appropriate and aligns with modern JUnit conventions.
60-60: LGTM!Consistent with modern JUnit 5 conventions where test methods don't require
publicvisibility.
127-127: LGTM!Using
varfor local type inference is appropriate here since the type is immediately clear from the constructor call.
...ibution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.java
Show resolved
Hide resolved
…StoreExampleTest` - Streamline imports by using wildcards. - Improve code readability with consistent formatting and indentation in test methods. - Replace legacy directory creation logic with modern `Files.createTempDirectory`. - Optimize assertions for deleting old entries within tests. - Minor logging and helper method adjustments for better maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java (4)
34-34: Simplify logger initialization.The
.getName()call is redundant;LoggerFactory.getLogger(Class)is the preferred overload and preserves compile-time type safety.Proposed fix
- private static final Logger log = LoggerFactory.getLogger(FileExchangeStoreExampleTest.class.getName()); + private static final Logger log = LoggerFactory.getLogger(FileExchangeStoreExampleTest.class);
46-49: Consider using JUnit'sfail()for consistency.The
sleep(300)appears to wait for file writes; a brief comment explaining the delay would improve clarity. Also,Assertions.fail()is more idiomatic than throwingAssertionErrordirectly.Proposed fix
- sleep(300); + // Allow time for exchange file to be written to disk + sleep(300); if (!containsRecursively(new File(baseDir, "exchanges"), this::filterDotMsg)) - throw new AssertionError("Did not find *.msg in exchanges dir."); + fail("Did not find *.msg in exchanges dir.");
57-86: Ensure cleanup runs even when assertions fail.If any assertion fails before
recursiveDelete(base), the temp directory is left behind. Use try-finally or JUnit's@TempDirannotation for automatic cleanup.Option 1: Use try-finally
@Test void testDeleteOldFolders() throws Exception { - File base = createTempFolder(); - - // Creates old entries from 2015 in the filestore to test if they are deleted - createOldEntriesForDeletion(base); - - // Assert that the old entries are there - ... - - // cleanup - recursiveDelete(base); + try { + // Creates old entries from 2015 in the filestore to test if they are deleted + createOldEntriesForDeletion(base); + + // Assert that the old entries are there + for (int m = 1; m <= 3; m++) + for (int d = 1; d <= 31; d++) + if (!(m == 2 && d > 28)) + assertTrue(new File(base, "2015/" + m + "/" + d).exists()); + + // Let the FileExchangeStore delete the old entries + getFileExchangeStore(base).deleteOldFolders(getCalendar()); + + // Assert that the old entries are deleted + for (int d = 1; d <= 31; d++) + assertFalse(new File(base, "2015/1/" + d).exists()); + for (int d = 1; d <= 13; d++) + assertFalse(new File(base, "2015/2/" + d).exists()); + for (int d = 17; d <= 28; d++) + assertTrue(new File(base, "2015/2/" + d).exists()); + for (int d = 1; d <= 31; d++) + assertTrue(new File(base, "2015/3/" + d).exists()); + } finally { + recursiveDelete(base); + } }Option 2: Use JUnit 5 @tempdir (preferred)
@Test void testDeleteOldFolders(@TempDir Path tempDir) throws Exception { File base = new File(tempDir.toFile(), "FileExchangeStoreTest"); if (!base.mkdirs()) { throw new IOException("Failed to create directory: " + base.getAbsolutePath()); } // ... rest of test without manual cleanup }
105-111: Consider validating directory creation.While the subsequent
assertTrue(...exists())will catch failures, explicit error handling here would produce clearer failure messages.Proposed fix
private void createOldEntriesForDeletion(File base) { for (int m = 1; m <= 3; m++) for (int d = 1; d <= 31; d++) - if (!(m == 2 && d > 28)) - //noinspection ResultOfMethodCallIgnored - new File(base, "2015/%d/%d".formatted(m, d)).mkdirs(); + if (!(m == 2 && d > 28)) { + File dir = new File(base, "2015/%d/%d".formatted(m, d)); + if (!dir.mkdirs() && !dir.exists()) { + throw new RuntimeException("Failed to create test directory: " + dir); + } + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
distribution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java (2)
88-95: LGTM!The temp folder creation with proper error handling is well implemented.
36-39: LGTM!The helper methods are well-structured and improve test readability. Good use of extraction to keep test methods focused.
Also applies to: 53-55, 113-126
.../test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java
Show resolved
Hide resolved
.../test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.java
Show resolved
Hide resolved
.formatted().@BeforeEach, improve logging, and make helper methods private for encapsulation.Summary by CodeRabbit
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.