fix: encoded paths are not read back#353
Conversation
📝 WalkthroughWalkthrough
ChangesEncoded path test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java`:
- Around line 79-140: Update the folder writer to create decoded filenames, and
update the reader’s checkFolderHasFile flow to probe the decoded name first,
then the raw encoded name while preserving the directory-containment guard.
Extend testEncodedIdsFindTheirPaths to include a legacy encoded filename, then
re-save and re-read it to verify migration compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04156f2f-d11c-411c-b9f6-69c95acef06f
📒 Files selected for processing (1)
src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java
| @Test | ||
| void testEncodedIdsFindTheirPaths(@TempDir Path tempDir) throws IOException { | ||
| RoCrate.RoCrateBuilder builder = new RoCrate.RoCrateBuilder(); | ||
| { | ||
| FileEntity.FileEntityBuilder dataEntityBuilder = | ||
| new FileEntity.FileEntityBuilder(); | ||
| dataEntityBuilder.setId("id 1"); | ||
| dataEntityBuilder.addTypes(List.of("File")); | ||
| UUID uuid = UUID.randomUUID(); | ||
| Path path = tempDir.resolve(uuid.toString()); | ||
| Files.writeString(path, "File"); | ||
| dataEntityBuilder.setLocation(path); | ||
|
|
||
| builder.addDataEntity(dataEntityBuilder.build()); | ||
| } | ||
| { | ||
| FileEntity.FileEntityBuilder dataEntityBuilder = | ||
| new FileEntity.FileEntityBuilder(); | ||
| dataEntityBuilder.setId("id\uD83E\uDD791"); | ||
| dataEntityBuilder.addTypes(List.of("File")); | ||
| UUID uuid = UUID.randomUUID(); | ||
| Path path = tempDir.resolve(uuid.toString()); | ||
| Files.writeString(path, "File"); | ||
| dataEntityBuilder.setLocation(path); | ||
|
|
||
| builder.addDataEntity(dataEntityBuilder.build()); | ||
| } | ||
| { | ||
| FileEntity.FileEntityBuilder dataEntityBuilder = | ||
| new FileEntity.FileEntityBuilder(); | ||
| dataEntityBuilder.setId("id|1"); | ||
| dataEntityBuilder.addTypes(List.of("File")); | ||
| UUID uuid = UUID.randomUUID(); | ||
| Path path = tempDir.resolve(uuid.toString()); | ||
| Files.writeString(path, "File"); | ||
| dataEntityBuilder.setLocation(path); | ||
|
|
||
| builder.addDataEntity(dataEntityBuilder.build()); | ||
| } | ||
| Path location = tempDir.resolve("out-crate-simple-file"); | ||
| { | ||
| RoCrate crate = builder.build(); | ||
| CrateWriter<String> writer = Writers.newFolderWriter(); | ||
| writer.save(crate, location.toAbsolutePath().toString()); | ||
| } | ||
| { | ||
| CrateReader<String> roCrateReader = Readers.newFolderReader(); | ||
|
|
||
| RoCrate roCrate = roCrateReader.readCrate( | ||
| location.toAbsolutePath().toString() | ||
| ); | ||
| for (DataEntity dataEntity : roCrate.getAllDataEntities()) { | ||
| System.out.println(dataEntity.getId() + ": " + dataEntity.getPath()); | ||
| } | ||
| for (DataEntity dataEntity : roCrate.getAllDataEntities()) { | ||
| assertNotNull( | ||
| dataEntity.getPath(), | ||
| "Path of ID: " + dataEntity.getId() | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the reader/writer compatibility behavior before merging these tests.
Line 206 fails with the current reader: checkFolderHasFile("id%2042", ...) decodes and probes only id 42, so the encoded legacy file is never found. The special-ID round trip also depends on the writer changing from encoded to decoded filenames. Update the writer to emit decoded names and have the reader try decoded first, then the raw encoded name with the same containment guard. Also re-save and re-read the legacy case to cover the stated migration objective.
Also applies to: 195-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java`
around lines 79 - 140, Update the folder writer to create decoded filenames, and
update the reader’s checkFolderHasFile flow to probe the decoded name first,
then the raw encoded name while preserving the directory-containment guard.
Extend testEncodedIdsFindTheirPaths to include a legacy encoded filename, then
re-save and re-read it to verify migration compatibility.
There was a problem hiding this comment.
Do you just complain I write my tests before I fix the issue? I am working test-driven. This is a test, the CI detects it. No need to run an expensive review about this. Instead, help me to figure out why the tests fail on Windows.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Relevant source files =='
fd -t f . src | rg '(Folder.*(Reader|Writer)|.*(Reader|Writer).*Folder|ReadAndWriteTest)\.java$' || true
echo
echo '== Reader/writer outlines =='
for f in $(fd -t f . src | rg '(Folder.*(Reader|Writer)|.*(Reader|Writer).*Folder)\.java$'); do
echo "--- $f"
ast-grep outline "$f" --items all --type method --match 'checkFolderHasFile|save|write|copy' || true
done
echo
echo '== Path-resolution and filename handling =='
rg -n -C 5 'checkFolderHasFile|IdentifierUtils\.(encode|decode)|getFileName\(|resolve\(' src/main src/test 2>/dev/null || true
echo
echo '== Windows CI definitions =='
fd -t f -HI . .github 2>/dev/null | while read -r f; do
echo "--- $f"
rg -n -C 3 'windows|Windows|runner\.os|os:' "$f" || true
done
echo
echo '== Test IDs and filesystem operations =='
rg -n -C 3 'id\|1|id\\uD83E\\uDD791|Files\.(writeString|move)|resolve\(id' src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.javaLength of output: 192723
@Pfeil, confirmed: the Windows failure is caused by id|1 in testEncodedIdsFindTheirPaths.
WriteFolderStrategy now decodes IDs before writing filenames:
String filename = IdentifierUtils.decode(id).orElse(id);So the encoded entity ID for id|1 is decoded and written as id|1. | is forbidden in Windows file names, so the copy fails there. The space (id 1) and emoji cases are valid Windows filenames.
The reader fallback is already in place: CrateReader.checkFolderHasFile tries the decoded filename first, then the original encoded filename, with the containment check preserved. This is not a reader issue.
For a platform-independent round-trip test, remove or replace the pipe case with a character that is URI-encoded but valid in Windows filenames (the space case already covers that). If supporting IDs such as id|1 is a product requirement, the writer needs an explicit portability rule: retain percent-encoding whenever the decoded filename is illegal on the target filesystem. In that case, the test should expect %7C, not |, on Windows.
Sorry again for treating the TDD test as the problem rather than helping identify this platform constraint.
🐇💭
🧬 Code Graph Analysis Results
src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java
(No additional snippets available beyond the full file content you provided.)
src/main/java/edu/kit/datamanager/ro_crate/RoCrate.java
Lines 248-253
`@Override`
public void addDataEntity(DataEntity entity) {
this.metadataContext.checkEntity(entity);
this.roCratePayload.addDataEntity(entity);
this.rootDataEntity.addToHasPart(entity.getId());
}Lines 484-489
public RoCrateBuilder addDataEntity(DataEntity dataEntity) {
this.metadataContext.checkEntity(dataEntity);
this.payload.addDataEntity(dataEntity);
this.rootDataEntity.addToHasPart(dataEntity.getId());
return this;
}Lines 491-527
public void addDataEntity(DataEntity entity, String parentId)
throws IllegalArgumentException {
if (parentId == null) {
throw new IllegalArgumentException("Parent ID is null.");
}
DataEntity parentEntity = parentId.equals("./")
? this.rootDataEntity
: this.payload.getDataEntityById(parentId);
if (parentEntity == null) {
throw new IllegalArgumentException(
"Parent ID not found in the crate."
);
}
if (parentEntity.getTypes().contains("File")) {
throw new IllegalArgumentException(
"Parent entity cannot be a File."
);
}
if (!parentEntity.getTypes().contains("Dataset")) {
throw new IllegalArgumentException(
"Parent entity must be a Dataset in order to contain another DataEntity as a part."
);
}
this.metadataContext.checkEntity(entity);
if (parentEntity instanceof DataSetEntity) {
((DataSetEntity) parentEntity).addToHasPart(entity.getId());
} else {
parentEntity.addProperty("hasPart", entity.getId());
}
this.payload.addDataEntity(entity);
}Lines 206-209
`@Override`
public DataEntity getDataEntityById(String id) {
return this.roCratePayload.getDataEntityById(id);
}Lines 220-223
`@Override`
public Set<DataEntity> getAllDataEntities() {
return new HashSet<>(this.roCratePayload.getAllDataEntities());
}src/main/java/edu/kit/datamanager/ro_crate/entities/data/DataEntity.java
Lines 61-64
`@JsonIgnore`
public Path getPath() {
return path;
}Lines 88-96
public T setLocation(Path path) {
if (path != null) {
if (this.getId() == null) {
this.setId(path.getFileName().toString());
}
this.location = path;
}
return self();
}Lines 123-128
public T setLocation(URI uri) {
if (uri != null && this.getId() == null) {
this.setId(uri.toString());
}
return self();
}src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java
Lines 91-103
public RoCrate readCrate(T location) throws IOException {
// get the ro-crate-metadata.json
ObjectNode metadataJson = strategy.readMetadataJson(location);
// get the content of the crate
File files = strategy.readContent(location);
// this set will contain the files that are associated with entities
HashSet<String> usedFiles = new HashSet<>();
usedFiles.add(files.toPath().resolve(FILE_METADATA_JSON).toFile().getPath());
usedFiles.add(files.toPath().resolve(FILE_PREVIEW_HTML).toFile().getPath());
usedFiles.add(files.toPath().resolve(FILE_PREVIEW_FILES).toFile().getPath());
return rebuildCrate(metadataJson, files, usedFiles).markAsImported();
}src/main/java/edu/kit/datamanager/ro_crate/writer/CrateWriter.java
Lines 47-54
public void save(Crate crate, DESTINATION_TYPE destination) throws IOException {
Validator defaultValidation = new Validator(new JsonSchemaValidation());
defaultValidation.validate(crate);
if (this.provenanceManager != null) {
this.provenanceManager.addProvenanceInformation(crate);
}
this.strategy.save(crate, destination);
}src/main/java/edu/kit/datamanager/ro_crate/reader/Readers.java
Lines 49-51
public static CrateReader<String> newFolderReader() {
return new CrateReader<>(new ReadFolderStrategy());
}src/main/java/edu/kit/datamanager/ro_crate/writer/Writers.java
Lines 21-23
public static CrateWriter<String> newFolderWriter() {
return new CrateWriter<>(new WriteFolderStrategy());
}
1 participant
The issue is actually that IDs get encoded, and their files get the encoded filenames. When reading the IDs back and decoding them, the path is not correct any more.
The fix will be:
In order to not break existing crates:
Other:
Summary by CodeRabbit
Bug Fixes
Tests