Version 2.1.0 #233
Version 2.1.0 #233
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ev-json-compare-7.x chore(deps): update dependency com.github.fslev:json-compare to v7
…ven-publish-java-8.x chore(deps): update plugin io.freefair.maven-publish-java to v8.13.1
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ests not working, yet)
…va into development
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ng, started working on proper template for FreeMarker library
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…d into crate builder process properly
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…Preview, integrated stream writing to all previews, some tests still failing
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…re fault tolerance
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…writing-crates Generalize reading and writing crates
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java (1)
25-25: 💡 Verification agent🧩 Analysis chain
Update RoCrateWriter to use factory pattern too.
Line 25 uses deprecated classes that are marked for removal.
Replace this line with the new factory-based approach for consistency:
- RoCrateWriter writer = new RoCrateWriter(new FolderWriter()); + writer.save(crate, temp.toString()); + HelpFunctions.compareTwoDir(temp.toFile(), new File(WorkflowHubTest.class.getResource("/crates/workflowhub/workflow1/").getPath()));Given the factory pattern used for readers, consider using equivalent Writers factory methods:
- RoCrateWriter writer = new RoCrateWriter(new FolderWriter()); + CrateWriter<String> writer = Writers.newFolderWriter(); writer.save(crate, temp.toString()); HelpFunctions.compareTwoDir(temp.toFile(), new File(WorkflowHubTest.class.getResource("/crates/workflowhub/workflow1/").getPath()));
🏁 Script executed:
#!/bin/bash # Check for Writers factory class and methods rg -A 3 "class Writers" src/main/javaLength of output: 376
To verify the available factory methods and confirm the deprecation status of RoCrateWriter, let’s inspect the Writers class and the RoCrateWriter declaration:
🏁 Script executed:
#!/bin/bash # Show the top of the Writers factory class sed -n '1,200p' src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java # Locate any newFolderWriter factory method rg -n "newFolderWriter" -A3 src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java # Confirm whether RoCrateWriter is marked @Deprecated rg -n "@Deprecated" -C2 "class RoCrateWriter" src/main/javaLength of output: 5488
Replace deprecated RoCrateWriter instantiation with the Writers factory
Both RoCrateWriter and FolderWriter are marked @deprecated(since="2.1.0", forRemoval=true). Use the new factory methods in Writers to stay current.• File: src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java
Line 25- RoCrateWriter writer = new RoCrateWriter(new FolderWriter()); + CrateWriter<String> writer = Writers.newFolderWriter(); writer.save(crate, temp.toString()); HelpFunctions.compareTwoDir( temp.toFile(), new File(WorkflowHubTest.class .getResource("/crates/workflowhub/workflow1/").getPath()) );🧰 Tools
🪛 GitHub Actions: Build & Tests
[warning] 25-25: Deprecated API usage: RoCrateWriter and FolderWriter are deprecated and marked for removal.
🧹 Nitpick comments (18)
src/main/java/edu/kit/datamanager/ro_crate/writer/FolderWriter.java (1)
7-11: Add an explicit@deprecatedJavadoc tagAlthough the class is annotated with
@Deprecated, the Javadoc itself is missing the corresponding@deprecatedtag. Adding it keeps the documentation consistent and prevents Javadoc-lint warnings.* @author Nikola Tzotchev on 9.2.2022 г. + * @deprecated Replaced by {@link FolderStrategy}. * - * @deprecated Use {@link FolderStrategy} instead. */src/test/java/edu/kit/datamanager/ro_crate/writer/ZipWriterTest.java (1)
10-12: PassPathinstead ofStringto avoid unnecessary conversion
savealready receives aPathin the test boiler-plate. Converting it toStringloses type-safety, and the underlying strategy can work directly withPath(the factory returns aCrateWriter<Path>).- Writers.newZipPathWriter() - .save(crate, target.toAbsolutePath().toString()); + Writers.newZipPathWriter() + .save(crate, target.toAbsolutePath());src/main/java/edu/kit/datamanager/ro_crate/writer/CrateWriter.java (3)
3-6:Objects.requireNonNullimport missing after null-check suggestionIf you adopt the null-check proposed below, remember to add:
import java.util.Objects;
15-17: Null-check constructor argumentThe writer is unusable with a
nullstrategy, so fail fast.- public CrateWriter(GenericWriterStrategy<DESTINATION> strategy) { - this.strategy = strategy; + public CrateWriter(GenericWriterStrategy<DESTINATION> strategy) { + this.strategy = Objects.requireNonNull(strategy, "strategy must not be null"); }
7-12: Document generic type to silence Javadoc warningsThe build pipeline reports “no @param for
<DESTINATION>”. Add the generic parameter description.* pattern for writing crates as different formats. (zip, folders, etc.) + * + * @param <DESTINATION> type of the output target (e.g., Path, OutputStream, String) */🧰 Tools
🪛 GitHub Actions: Build & Tests
[warning] 11-11: Javadoc warning: no @param for .
src/test/java/edu/kit/datamanager/ro_crate/writer/FolderWriterTest.java (1)
17-20: PreferPathoverStringfor writer destinationSame reasoning as for
ZipWriterTest: keep type-safety and let the strategy receive aPath.- Writers.newFolderWriter() - .save(crate, target.toAbsolutePath().toString()); + Writers.newFolderWriter() + .save(crate, target.toAbsolutePath());src/main/java/edu/kit/datamanager/ro_crate/reader/FolderStrategy.java (1)
22-23: Reuse the ObjectMapper instead of instantiating it on every callCreating a new
ObjectMapperis relatively expensive and also breaks the configuration singleton pattern already provided byMyObjectMapper. Consider caching the mapper in astatic finalfield:-private ObjectMapper objectMapper = MyObjectMapper.getMapper(); +private static final ObjectMapper MAPPER = MyObjectMapper.getMapper();and then use
MAPPER.readTree(...).
This reduces allocations and keeps configuration consistent.src/test/java/edu/kit/datamanager/ro_crate/reader/ZipReaderTest.java (1)
38-43: Optional: assert crate integrity after extractionAfter calling
readCrate, you only assert that the strategy flagisExtracted()turnedtrue.
Adding at least one domain-specific assertion (e.g., presence ofro-crate-metadata.jsonor a known entity) would strengthen the test and guard against regressions in the new strategy layer.No code change required in this PR, just a suggestion for future coverage.
src/main/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategy.java (1)
60-68: Simplify metadata write – avoid manual buffering
ZipOutputStream.write(byte[])can handle the whole JSON payload; the explicit buffer/read loop adds unnecessary complexity and CPU cycles.- byte[] buff = new byte[4096]; - int readLen; - zipStream.putNextEntry(zipParameters); - try (InputStream inputStream = new ByteArrayInputStream(str.getBytes(StandardCharsets.UTF_8))) { - while ((readLen = inputStream.read(buff)) != -1) { - zipStream.write(buff, 0, readLen); - } - } + zipStream.putNextEntry(zipParameters); + zipStream.write(str.getBytes(StandardCharsets.UTF_8)); zipStream.closeEntry();This also eliminates one nested
InputStream.src/test/java/edu/kit/datamanager/ro_crate/reader/FolderReaderTest.java (2)
24-25: Strengthen assertion: verify that content is written, not just directory exists
@TempDiralready guarantees the path is a directory, so the current check doesn’t validate the writer. Assert that it contains at least one file after saving:- assertTrue(target.toFile().isDirectory()); + assertTrue(target.toFile().isDirectory(), "Target should be a directory"); + assertTrue(target.toFile().list().length > 0, + "Crate directory is empty – save operation may have failed");
72-75: Use try-with-resources forCrateReaderif it implementsCloseableIf
CrateReaderholds any closable resources (temp dirs, streams), encapsulate it in a try-with-resources block to avoid leaks:- CrateReader<String> reader = Readers.newFolderReader(); - RoCrate c1_read = reader.readCrate(temp1.toFile().toString()); - RoCrate c2_read = reader.readCrate(temp2.toFile().toString()); + try (CrateReader<String> reader = Readers.newFolderReader()) { + RoCrate c1_read = reader.readCrate(temp1.toFile().toString()); + RoCrate c2_read = reader.readCrate(temp2.toFile().toString()); + // existing assertions … + }If the class is not
Closeable, consider adding that for safer resource management.src/main/java/edu/kit/datamanager/ro_crate/reader/ZipStrategy.java (1)
37-39: Typo in Javadoc – “Creates”, not “Crates”Minor spelling issue that may trip up auto-generation tools.
- * Crates a ZipReader with the default configuration as described in the class documentation. + * Creates a ZipReader with the default configuration as described in the class documentation.src/main/java/edu/kit/datamanager/ro_crate/reader/ZipStreamStrategy.java (1)
34-37: Typo in constructor Javadoc- * Crates a ZipStreamReader with the default configuration as described in + * Creates a ZipStreamReader with the default configuration as described insrc/test/java/edu/kit/datamanager/ro_crate/reader/CrateReaderTest.java (1)
170-206: Minor style issues in test helper code
- Test method names starting with an upper-case
T(TestWithFile...) deviate from the lower-camel convention used elsewhere and from common JUnit naming idioms (testXyz).- Mixing
toString()andtoFile().toString()(line 197 vs 198) is inconsistent and may confuse future readers, though it works the same.Not blocking, but aligning these will improve readability.
src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java (2)
31-33: Typo in Javadoc lowers perceived quality
consideres→considers.
Minor, but worth fixing because this class is prominently referenced in the public API.- * The reader consideres "hasPart" and "isPartOf" properties and considers all + * The reader considers "hasPart" and "isPartOf" properties and treats all
275-293: Lambda formatting hampers readabilityThe line break between
or(()and the->as a comment separator is confusing.
A one-liner (or adding braces) improves readability and avoids future mis-merges.- return maybeDescriptor.or(() - -> // from https://www.researchobject.org/ro-crate/1.2-DRAFT/root-data-entity.html#finding-the-root-data-entity - StreamSupport.stream(graph.spliterator(), isParallel) - .filter(node -> node.path(PROP_ID).asText().equals(FILE_METADATA_JSON)) - .findFirst() - ); + // Fallback: RO-Crate 1.2-DRAFT algorithm + return maybeDescriptor.or(() -> + StreamSupport.stream(graph.spliterator(), isParallel) + .filter(node -> node.path(PROP_ID).asText().equals(FILE_METADATA_JSON)) + .findFirst());src/test/java/edu/kit/datamanager/ro_crate/writer/CrateWriterTest.java (2)
74-77: Potential OS-dependent false positive
Files.isDirectory(extractionPath.resolve(dirName))assumes case-sensitive and normalised names.
On Windows this may unexpectedly pass/fail. Consider normalising both paths with.toAbsolutePath().normalize()or, even simpler, compareFiles.exists&Files.isDirectoryafter ensuringdirNameis lower-case invariant.🧰 Tools
🪛 GitHub Actions: Build & Tests
[error] 74-74: Test failure: org.opentest4j.AssertionFailedError at CrateWriterTest.java:74 in testFilesBeingAdjusted(Path).
[error] 74-74: Test failure: org.opentest4j.AssertionFailedError at CrateWriterTest.java:74 in testFilesBeingAdjusted(Path).
197-201:ensureCrateIsExtractedInfails when the writer outputs a folder
ZipFilethrows if given a directory (see FolderWriterTest).
Consider gracefully handling that case:- try (ZipFile zf = new ZipFile(pathToCrate.toFile())) { - zf.extractAll(expectedPath.toFile().getAbsolutePath()); - } + if (Files.isDirectory(pathToCrate)) { + FileUtils.copyDirectory(pathToCrate.toFile(), expectedPath.toFile()); + } else { + try (ZipFile zf = new ZipFile(pathToCrate.toFile())) { + zf.extractAll(expectedPath.toFile().getAbsolutePath()); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/FolderReader.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/FolderStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/GenericReaderStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/ReaderStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/RoCrateReader.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/ZipReader.java(2 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/ZipStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/reader/ZipStreamStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/CrateWriter.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/FolderStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/FolderWriter.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/GenericWriterStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriter.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/WriterStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/ZipStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategy.java(1 hunks)src/main/java/edu/kit/datamanager/ro_crate/writer/ZipWriter.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/HelpFunctions.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/BuilderSpec12Test.java(3 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java(3 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/TestRemoveAddContext.java(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/RealTest.java(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/reader/CrateReaderTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/reader/FolderReaderTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/reader/RoCrateReaderSpec12Test.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/reader/ZipReaderTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/reader/ZipStreamReaderTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/writer/CrateWriterTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/writer/FolderWriterTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriterSpec12Test.java(3 hunks)src/test/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategyTest.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/writer/ZipWriterTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/main/java/edu/kit/datamanager/ro_crate/reader/ZipReader.java
- src/test/java/edu/kit/datamanager/ro_crate/HelpFunctions.java
- src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java
- src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java
- src/main/java/edu/kit/datamanager/ro_crate/writer/GenericWriterStrategy.java
- src/main/java/edu/kit/datamanager/ro_crate/writer/WriterStrategy.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/edu/kit/datamanager/ro_crate/reader/ZipStreamReaderTest.java
- src/test/java/edu/kit/datamanager/ro_crate/crate/TestRemoveAddContext.java
- src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/test/java/edu/kit/datamanager/ro_crate/reader/RoCrateReaderSpec12Test.java (1)
src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)
src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/RealTest.java (2)
src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java (1)
CrateReader(36-340)src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)
src/main/java/edu/kit/datamanager/ro_crate/reader/RoCrateReader.java (5)
src/main/java/edu/kit/datamanager/ro_crate/writer/ZipWriter.java (1)
Deprecated(9-10)src/main/java/edu/kit/datamanager/ro_crate/writer/FolderWriter.java (1)
Deprecated(10-11)src/main/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriter.java (1)
Deprecated(9-15)src/main/java/edu/kit/datamanager/ro_crate/reader/FolderReader.java (1)
Deprecated(11-12)src/main/java/edu/kit/datamanager/ro_crate/reader/ZipReader.java (1)
Deprecated(23-47)
src/main/java/edu/kit/datamanager/ro_crate/writer/CrateWriter.java (1)
src/main/java/edu/kit/datamanager/ro_crate/validation/Validator.java (1)
Validator(9-20)
src/main/java/edu/kit/datamanager/ro_crate/writer/ZipWriter.java (5)
src/main/java/edu/kit/datamanager/ro_crate/writer/FolderWriter.java (1)
Deprecated(10-11)src/main/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriter.java (1)
Deprecated(9-15)src/main/java/edu/kit/datamanager/ro_crate/reader/FolderReader.java (1)
Deprecated(11-12)src/main/java/edu/kit/datamanager/ro_crate/reader/RoCrateReader.java (1)
Deprecated(15-20)src/main/java/edu/kit/datamanager/ro_crate/reader/ZipReader.java (1)
Deprecated(23-47)
src/test/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategyTest.java (1)
src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java (1)
Writers(9-42)
src/test/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriterSpec12Test.java (2)
src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java (1)
Writers(9-42)
src/test/java/edu/kit/datamanager/ro_crate/crate/BuilderSpec12Test.java (1)
src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)
src/test/java/edu/kit/datamanager/ro_crate/reader/ZipReaderTest.java (2)
src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java (1)
Writers(9-42)src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)
src/test/java/edu/kit/datamanager/ro_crate/reader/FolderReaderTest.java (2)
src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java (1)
Writers(9-42)src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java (1)
Readers(10-77)
🪛 GitHub Actions: Build & Tests
src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java
[warning] 25-25: Deprecated API usage: RoCrateWriter and FolderWriter are deprecated and marked for removal.
src/main/java/edu/kit/datamanager/ro_crate/writer/CrateWriter.java
[warning] 11-11: Javadoc warning: no @param for .
src/test/java/edu/kit/datamanager/ro_crate/writer/CrateWriterTest.java
[error] 74-74: Test failure: org.opentest4j.AssertionFailedError at CrateWriterTest.java:74 in testFilesBeingAdjusted(Path).
[error] 74-74: Test failure: org.opentest4j.AssertionFailedError at CrateWriterTest.java:74 in testFilesBeingAdjusted(Path).
🔇 Additional comments (26)
src/test/java/edu/kit/datamanager/ro_crate/reader/RoCrateReaderSpec12Test.java (1)
32-32: Excellent refactoring to use the factory method pattern.The change from direct instantiation of
RoCrateReaderto usingReaders.newFolderReader()aligns with the new strategy-based architecture for crate readers. This factory method approach provides better flexibility and maintainability.src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java (2)
5-6: Good update of imports to reflect new architecture.The import changes correctly reference the new generic reader interface and factory class.
21-21: Great use of factory method pattern.The change to use
Readers.newZipPathReader()is consistent with the new strategy-based architecture.src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/RealTest.java (2)
15-16: Good update of imports to align with new architecture.The import changes correctly reference the new generic reader interface and factory class.
33-33: Great use of factory method pattern.The change to use
Readers.newFolderReader()is consistent with the new strategy-based architecture. Using the genericCrateReader<String>and factory method provides better flexibility and readability.src/test/java/edu/kit/datamanager/ro_crate/crate/BuilderSpec12Test.java (2)
11-11: Good addition of Readers import.The import of the Readers utility class aligns with the new factory-based reader creation approach.
42-42: Great use of factory method pattern.The change to use
Readers.newFolderReader()is consistent with the new strategy-based architecture and improves code flexibility.src/test/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriterSpec12Test.java (4)
13-13: Import updated to use new factory utility class.The updated import reflects the architectural shift to use factory methods from the new
Readersutility class instead of directly importing reader implementation classes.
29-29: Factory method used instead of direct class instantiation.Good refactoring to use the new factory method pattern (
Readers.newFolderReader()) instead of directly instantiatingRoCrateReader/FolderReader. This centralizes reader creation logic and improves maintainability.
32-33: Writer creation refactored to use factory method.The code now uses
Writers.newFolderWriter()instead of directly instantiating a writer implementation class. This change aligns with the strategy-based architecture refactoring and improves consistency.
50-50: Second reader instance also refactored to use factory method.Consistent use of
Readers.newFolderReader()throughout the test ensures uniformity and aligns with the new architecture. This makes the test more maintainable as implementation details change.src/test/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategyTest.java (1)
1-11: Good implementation of the ZipStreamStrategy test.This test class properly extends the abstract
CrateWriterTestbase class and implements thesaveCratemethod to test the zip stream writing functionality. The class structure aligns well with the new testing architecture.src/main/java/edu/kit/datamanager/ro_crate/reader/RoCrateReader.java (3)
13-15: Clear deprecation documentation with migration path.The deprecation notice clearly indicates that users should migrate to the new
CrateReaderclass, which is essential for guiding users through the API transition.
15-19: Well-structured deprecated subclass implementation.The class is properly marked as deprecated with the
@Deprecatedannotation that includessinceandforRemovalattributes. The simple extension ofCrateReader<String>with a single constructor that delegates to the superclass maintains backward compatibility while encouraging migration to the new API.
1-20: Successfully addressed prior review comments.The refactoring completely addresses the previous review comments about avoiding
instanceofchecks and unsupported method errors by moving to a generic strategy-based implementation pattern.src/main/java/edu/kit/datamanager/ro_crate/writer/RoCrateWriter.java (3)
7-8: Clear deprecation documentation with migration path.The deprecation notice clearly indicates that users should migrate to the new
CrateWriterclass, which is essential for guiding users through the API transition.
9-14: Well-structured deprecated subclass implementation.The class is properly marked as deprecated with the
@Deprecatedannotation that includessinceandforRemovalattributes. The implementation extendsCrateWriter<String>with a single constructor that delegates to the superclass, maintaining backward compatibility while encouraging migration to the new API.
1-15: Successfully addressed prior review comments.The refactoring completely addresses the previous review comments about error handling for incompatible writer strategies by moving to a generic strategy-based implementation pattern. The new architecture eliminates the need for runtime type checking and provides clear compile-time type safety.
src/main/java/edu/kit/datamanager/ro_crate/reader/GenericReaderStrategy.java (1)
6-28: Well-designed generic interface for reader strategies.The
GenericReaderStrategy<T>interface introduces a clean abstraction for reading RO-Crate data from different source types. This design provides excellent flexibility through the type parameterT, allowing for specialized implementations depending on the input location type (e.g., String paths, InputStreams, etc.).The interface is well-documented and follows the strategy pattern, which aligns with the PR objective of improving the reader/writer implementation using generics. This approach will make it easier to implement and maintain different reading strategies without code duplication.
src/main/java/edu/kit/datamanager/ro_crate/reader/ReaderStrategy.java (1)
3-14: Clean deprecation of legacy interface.The
ReaderStrategyinterface has been properly deprecated and updated to extendGenericReaderStrategy<String>. This is a good approach for maintaining backward compatibility while encouraging migration to the new generic interface. The deprecation annotation includes appropriate metadata (since = "2.1.0", forRemoval = true) and the Javadoc clearly directs users to the replacement interface.This change aligns with the goal of transitioning to a more generic and flexible architecture while providing a migration path for existing code.
src/main/java/edu/kit/datamanager/ro_crate/writer/ZipWriter.java (1)
3-10: Clean deprecation of ZipWriter in favor of ZipStrategy.The
ZipWriterclass has been properly deprecated and updated to extendZipStrategy. This is a good approach for maintaining backward compatibility while encouraging migration to the new strategy-based implementation. The deprecation annotation includes appropriate metadata and the Javadoc clearly directs users to the replacement class.This change aligns with the overall refactoring approach seen in other components (like
FolderWriter→FolderStrategyandRoCrateWriter→CrateWriter<String>), creating a consistent pattern throughout the codebase.src/main/java/edu/kit/datamanager/ro_crate/reader/FolderStrategy.java (1)
33-35: Validate the folder path before returning it
readContentblindly returnsnew File(location)even if the path does not exist or is not a directory. A quick guard improves robustness:-public File readContent(String location) { - return new File(location); +public File readContent(String location) { + File folder = new File(location); + if (!folder.isDirectory()) { + throw new IllegalArgumentException( + String.format("Path %s is not an existing directory", location)); + } + return folder; }src/main/java/edu/kit/datamanager/ro_crate/reader/FolderReader.java (1)
11-12: Deprecation shim looks goodTiny adapter class properly forwards to
FolderStrategy, is marked@Deprecated, and setsforRemoval = true.
No further action required.src/test/java/edu/kit/datamanager/ro_crate/reader/ZipReaderTest.java (1)
14-17: Ensure the target path used in tests is actually a ZIP file
saveCratereceives aPath targetwhose filename is not guaranteed to have a “.zip” suffix.
ZipStrategyrelies on the extension to decide whether to treat the path as a ZIP archive or a directory, so the test may produce a false positive/negative depending on the temporary path.- Writers.newZipPathWriter().save(crate, target.toAbsolutePath().toString()); + Path zipFile = target.resolveSibling(target.getFileName().toString() + ".zip"); + Writers.newZipPathWriter().save(crate, zipFile.toString()); + assertTrue(zipFile.toFile().isFile());Please verify the base test class does not expect the original
targetto be mutated; otherwise adapt accordingly.src/main/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategy.java (1)
70-72: Verify preview API usage
crate.getPreview().saveAllToStream(str, zipStream);passes the pretty-printed metadata string to the preview, whereas theZipStrategysibling passes only theZipFile.
Please double-check that this overload is intended and consistent with thePreviewcontract—otherwise previews might embed stale or duplicate metadata.src/test/java/edu/kit/datamanager/ro_crate/writer/CrateWriterTest.java (1)
61-71: Assertion failure in CI indicates writer does not rename the fileCI shows a failure at this assertion. Before tweaking the test, confirm whether the writer or the test is wrong:
- Manually inspect the written crate – is
cp7glop.aipresent with the old or new name?- If it is present with the old name the writer still needs to perform the rename step.
Would you like a small script that opens the produced ZIP and lists the entry names so we can locate the regression quickly?
src/test/java/edu/kit/datamanager/ro_crate/writer/ZipStreamStrategyTest.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/kit/datamanager/ro_crate/reader/ZipStreamStrategy.java
Show resolved
Hide resolved
src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java
Outdated
Show resolved
Hide resolved
…n and safety checks
…t-entities Fix renaming of data set entities
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/gradle.yml (2)
50-51: Add descriptive job name and ensure ordering
Consider adding aname:for thejavadocjob (e.g., "Generate Javadoc") and aneeds: buildso it runs after the main build, improving clarity and dependency management.javadoc: + name: Generate Javadoc + needs: build runs-on: ubuntu-latest
64-64: Add trailing newline at end of file
Including a final newline avoids linter warnings and ensures the file adheres to POSIX text standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/gradle.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/gradle.yml
[warning] 53-53: wrong indentation: expected 4 but found 6
(indentation)
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/edu/kit/datamanager/ro_crate/crate/OtherFilesTest.java (1)
27-44: Consider adding additional writer strategy tests.While this test covers the folder writing scenario, consider adding similar tests for zip and zipstream strategies to ensure complete coverage of all factory methods in the
Writersutility class.For example, you might want to add test methods that use:
Writers.newZipPathWriter().save(roCrate, zipPath.toFile().getAbsolutePath());And:
try (FileOutputStream fos = new FileOutputStream(zipPath.toFile())) { Writers.newZipStreamWriter().save(roCrate, fos); }This would align with the PR objectives of improving test coverage and thoroughly validating all the new factory methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/edu/kit/datamanager/ro_crate/crate/OtherFilesTest.java(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java(3 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/preview/PreviewCrateTest.java(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/edu/kit/datamanager/ro_crate/crate/realexamples/WorkflowHubTest.java
- src/test/java/edu/kit/datamanager/ro_crate/crate/preview/PreviewCrateTest.java
- src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/edu/kit/datamanager/ro_crate/crate/OtherFilesTest.java (1)
src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java (1)
Writers(9-42)
🔇 Additional comments (2)
src/test/java/edu/kit/datamanager/ro_crate/crate/OtherFilesTest.java (2)
6-6: Correct import update for new writer pattern.The addition of
Writersutility class import aligns with the library's architectural shift from specific writer implementations to a strategy-based approach with factory methods.
43-44: Good simplification using the factory pattern.The change from direct writer instantiation to the factory method
Writers.newFolderWriter()simplifies the test code while maintaining the same functionality. This follows good software design principles by hiding implementation details and making the code more maintainable.Looking at the PR objectives, this change directly addresses the goal of introducing factory methods to support common cases, specifically the folder-based writing scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/test/java/edu/kit/datamanager/ro_crate/preview/PreviewTest.java (9)
29-29: Misleading variable name for StaticPreview instance.The variable is named
customPreviewbut it's actually aStaticPreviewinstance, which could be confusing. This appears to be a result of refactoring from previous tests.-StaticPreview customPreview = new StaticPreview(file1.toFile(), file2.toFile()); +StaticPreview staticPreview = new StaticPreview(file1.toFile(), file2.toFile());
32-32: Update variable reference to match suggested name change.This variable reference should be updated to match the variable name change suggested above.
-customPreview.saveAllToFolder(dir.resolve("result").toFile()); +staticPreview.saveAllToFolder(dir.resolve("result").toFile());And similarly in the ZIP test:
-StaticPreview customPreview = new StaticPreview(file1.toFile(), file2.toFile()); +StaticPreview staticPreview = new StaticPreview(file1.toFile(), file2.toFile()); -customPreview.saveAllToZip(new ZipFile(dir.resolve("destination.zip").toFile())); +staticPreview.saveAllToZip(new ZipFile(dir.resolve("destination.zip").toFile()));Also applies to: 57-59
34-34: Use more descriptive variable names.The variable
eis not descriptive. Consider using a more meaningful name likeresultDirorextractedDir.-var e = dir.resolve("result"); +var resultDir = dir.resolve("result");And in the ZIP test:
-var e = dir.resolve("extracted"); +var extractedDir = dir.resolve("extracted");Also applies to: 64-64
36-38: Update references to renamed variables.Update these references to match the variable name changes suggested above.
-var roPreview = e.resolve("ro-crate-preview.html"); -var roDir = e.resolve("ro-crate-preview_files"); -var roDirFile = roDir.resolve("fileInDir.html"); +var roPreview = resultDir.resolve("ro-crate-preview.html"); +var roDir = resultDir.resolve("ro-crate-preview_files"); +var roDirFile = roDir.resolve("fileInDir.html");And in the ZIP test:
-var roPreview = e.resolve("ro-crate-preview.html"); -var roDir = e.resolve("ro-crate-preview_files"); -var roDirFile = roDir.resolve("fileInDir.html"); +var roPreview = extractedDir.resolve("ro-crate-preview.html"); +var roDir = extractedDir.resolve("ro-crate-preview_files"); +var roDirFile = roDir.resolve("fileInDir.html");Also applies to: 65-67
84-85: Inconsistent comment with implementation.The comment "this crate will not have a json file" is inconsistent with the implementation, as a JSON file is being copied to the crate in the following line.
-// this crate will not have a json file +// Create a temporary crate directory with the necessary metadata
111-111: Incorrect error message in exception assertion.In
testAutomaticPreviewZip, the error message correctly indicates an invalid ZIP file. However, intestCustomPreviewAddToFolder, the error message also refers to an invalid ZIP file when it should be about an invalid folder.-Assertions.fail("Expected IOException when providing invalid ZIP file for preview."); +Assertions.fail("Expected IOException when providing non-existent directory for preview.");Also applies to: 135-135
78-168: Consider adding content validation for AutomaticPreview and CustomPreview tests.The tests for
AutomaticPreviewandCustomPreviewonly verify file existence but not the content of the generated preview files. Consider enhancing these tests to validate that the content is correctly generated.For example, you could add content validation similar to the StaticPreview tests:
// In testAutomaticPreviewAddToFolder String previewContent = Files.readString(crate.resolve("ro-crate-preview.html")); assertTrue(previewContent.contains("<!-- RO-Crate Preview -->"), "Preview should contain RO-Crate marker");Apply similar validation in the ZIP test methods as well.
93-157: Consider refactoring common ZIP file creation logic.There's duplicate code for creating a ZIP file with a JSON file in both
testAutomaticPreviewZipandtestCustomPreviewZip. Consider extracting this common logic into a helper method to improve maintainability.private ZipFile createZipWithMetadata(Path dir, String filename) throws IOException { InputStream crateJson = PreviewTest.class.getResourceAsStream("/crates/other/idrc_project/ro-crate-metadata.json"); ZipParameters zipParameters = new ZipParameters(); zipParameters.setFileNameInZip("ro-crate-metadata.json"); ZipFile zipFile = new ZipFile(dir.resolve(filename).toFile()); zipFile.addStream(crateJson, zipParameters); crateJson.close(); return zipFile; }Then use it in both test methods:
ZipFile zipFile = createZipWithMetadata(dir, "test.zip");
107-114: Use JUnit's assertThrows for cleaner exception testing.The try-catch blocks for testing exceptions can be replaced with JUnit's
assertThrowsmethod for cleaner and more idiomatic tests.-try { - // this should trow an exception but not stop the execution - ZipFile randomZipFile = new ZipFile(dir.resolve("dddd.zip").toFile()); - automaticPreview.saveAllToZip(randomZipFile); - Assertions.fail("Expected IOException when providing invalid ZIP file for preview."); -} catch (IOException ex) { - //ok -} +Assertions.assertThrows(IOException.class, () -> { + ZipFile randomZipFile = new ZipFile(dir.resolve("dddd.zip").toFile()); + automaticPreview.saveAllToZip(randomZipFile); +}, "Expected IOException when providing invalid ZIP file for preview.");Apply similar changes to the other exception test blocks.
Also applies to: 158-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/edu/kit/datamanager/ro_crate/preview/CustomPreview.java(1 hunks)src/test/java/edu/kit/datamanager/ro_crate/preview/PreviewTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/edu/kit/datamanager/ro_crate/preview/CustomPreview.java
🔇 Additional comments (1)
src/test/java/edu/kit/datamanager/ro_crate/preview/PreviewTest.java (1)
1-170: Good test coverage for all preview implementations.Overall, this test class provides excellent coverage for the three preview implementations. The tests are well-structured with clear setup, action, and verification phases, and they properly handle exceptions.
|
As I reused the branch (which seems to confuse coveralls) we do not get precise test coverage. According to my local tests it should be again at ~90% and most uncovered lines are exceptions. Therefore, as long as all tests pass, I am fine. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/gradle.yml (1)
64-64: Add newline at end of file.
YAMLlint flags a missing newline character at EOF. Please append a blank line after the finalrun: ./gradlew javadocto satisfy POSIX conventions and avoid lint errors:@@ - run: ./gradlew javadoc +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/gradle.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/gradle.yml
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/gradle.yml (1)
59-61: Verify necessity ofro-crate-html-jsin the Javadoc job.
Thejavadocjob installs thero-crate-html-jsnpm package, but the<code>./gradlew javadoc</code>task typically only generates JavaDoc. If HTML previews or template processing aren’t part of this step, consider removing the npm install to speed up your workflow and reduce external dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/gradle.yml (1)
53-65:⚠️ Potential issueFix indentation under
steps:to avoid YAML parse errorsEach
-step entry must be indented two spaces deeper than thesteps:key. Without this, the workflow may fail to parse. Please apply the following diff:@@ lines 53-65 - steps: - - uses: actions/checkout@v4 - - name: Set up openJDK version - uses: actions/setup-java@v4 - with: - java-version: 21 - distribution: 'zulu' - - name: Install Dependencies - run: npm install -g ro-crate-html-js - - name: Setup Gradle - uses: gradle/actions/setup-gradle@v4 - - name: Compile Javadoc - run: ./gradlew javadoc + steps: + - uses: actions/checkout@v4 + - name: Set up openJDK version + uses: actions/setup-java@v4 + with: + java-version: 21 + distribution: 'zulu' + - name: Install Dependencies + run: npm install -g ro-crate-html-js + - name: Setup Gradle + uses: gradle/actions/setup-gradle@v4 + - name: Compile Javadoc + run: ./gradlew javadoc🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
🧹 Nitpick comments (1)
.github/workflows/gradle.yml (1)
64-65: Consider uploading Javadoc as an artifactTo make the generated Javadoc available for review or downstream jobs, you can upload it after generation:
@@ lines 64-65 - run: ./gradlew javadoc + run: ./gradlew javadoc + - name: Upload Javadoc artifact + if: always() + uses: actions/upload-artifact@v4 + with: + name: javadoc + path: build/docs/javadoc🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/gradle.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/gradle.yml
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/gradle.yml (1)
50-52: Approve: Javadoc job correctly gated on build completionThe new
javadocjob now declaresneeds: [build], ensuring that Javadoc generation only runs after all build matrix jobs succeed. This guards against wasted CI runs on failing builds.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores
.gitignoreto exclude new build artifacts and system files.