Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow changes to file metadata via API #6962 #6968

Merged
merged 7 commits into from
Jul 1, 2020
Merged

allow changes to file metadata via API #6962 #6968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
809643a
allow changes to file metadata via API #6962
pdurbin Jun 5, 2020
508e95c
allow file metadata changes if passing existing path/name #6962
pdurbin Jun 8, 2020
9954220
allow changes if dir exists and label passed, formatting #6962
pdurbin Jun 9, 2020
e5ec9bc
avoid dir1 + file1 == dir + 1file1 (add "/") #6962
pdurbin Jun 9, 2020
491caff
eliminate code we don't need, handled elsewhere #6962
pdurbin Jun 9, 2020
a34d7c1
ensure passing "" as directoryLabel doesn't cause problems #6962
pdurbin Jun 10, 2020
2debed2
simply conflict-checking code #6962
pdurbin Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/main/java/edu/harvard/iq/dataverse/api/Files.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,18 +390,21 @@ public Response updateFileMetadata(@FormDataParam("jsonData") String jsonData,

JsonReader jsonReader = Json.createReader(new StringReader(jsonData));
javax.json.JsonObject jsonObject = jsonReader.readObject();
String label = jsonObject.getString("label", null);
String directoryLabel = jsonObject.getString("directoryLabel", null);
String path = "";
if (directoryLabel != null) {
path = directoryLabel + "/";
}
if (label == null) {
label = df.getFileMetadata().getLabel();
String incomingLabel = jsonObject.getString("label", null);
String incomingDirectoryLabel = jsonObject.getString("directoryLabel", null);
String existingLabel = df.getFileMetadata().getLabel();
String existingDirectoryLabel = df.getFileMetadata().getDirectoryLabel();
String pathPlusFilename = IngestUtil.getPathAndFileNameToCheck(incomingLabel, incomingDirectoryLabel, existingLabel, existingDirectoryLabel);
// We remove the current file from the list we'll check for duplicates.
// Instead, the current file is passed in as pathPlusFilename.
List<FileMetadata> fmdListMinusCurrentFile = new ArrayList<>();
for (FileMetadata fileMetadata : fmdList) {
if (!fileMetadata.equals(df.getFileMetadata())) {
fmdListMinusCurrentFile.add(fileMetadata);
}
}
if (IngestUtil.conflictsWithExistingFilenames(label, directoryLabel, fmdList, df)) {
String incomingPathPlusFileName = path + label;
return error(BAD_REQUEST, BundleUtil.getStringFromBundle("files.api.metadata.update.duplicateFile", Arrays.asList(incomingPathPlusFileName)));
if (IngestUtil.conflictsWithExistingFilenames(pathPlusFilename, fmdListMinusCurrentFile)) {
return error(BAD_REQUEST, BundleUtil.getStringFromBundle("files.api.metadata.update.duplicateFile", Arrays.asList(pathPlusFilename)));
}

optionalFileParams.addOptionalParams(upFmd);
Expand Down
49 changes: 23 additions & 26 deletions src/main/java/edu/harvard/iq/dataverse/ingest/IngestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,38 +101,35 @@ public static String duplicateFilenameCheck(FileMetadata fileMetadata, Set<Strin
return fileName;
}

/**
* Given an existing file that may or may not have a directoryLabel, take
* the incoming label and/or directory label and combine it with what's in
* the existing file, overwriting and filling in as necessary.
*/
public static String getPathAndFileNameToCheck(String incomingLabel, String incomingDirectoryLabel, String existingLabel, String existingDirectoryLabel) {
String labelToReturn = existingLabel;
String directoryLabelToReturn = existingDirectoryLabel;
if (incomingLabel != null) {
labelToReturn = incomingLabel;
}
if (incomingDirectoryLabel != null) {
directoryLabelToReturn = incomingDirectoryLabel;
}
if (directoryLabelToReturn != null) {
return directoryLabelToReturn + "/" + labelToReturn;
} else {
return labelToReturn;
}
}

/**
* Given a new proposed label or directoryLabel for a file, check against
* existing files if a duplicate directoryLabel/label combination would be
* created.
*
* @param label The new label (filename) that is being proposed. Can be
* null.
* @param directoryLabel The new directoryLabel (file path) that is being
* proposed. Can be null.
* @param fileMetadatas The list fileMetadatas to be compared against,
* probably from a draft.
* @param dataFile The file that is being updated with a new name or path.
* @return true if there is a conflict, false otherwise.
*/
public static boolean conflictsWithExistingFilenames(String label, String directoryLabel, List<FileMetadata> fileMetadatas, DataFile dataFile) {
public static boolean conflictsWithExistingFilenames(String pathPlusFilename, List<FileMetadata> fileMetadatas) {
List<String> filePathsAndNames = getPathsAndFileNames(fileMetadatas);
if (label != null || directoryLabel != null) {
String path = "";
if (directoryLabel != null) {
path = directoryLabel + "/";
}
if (label == null) {
label = dataFile.getFileMetadata().getLabel();
}
String incomingPathPlusFileName = path + label;
logger.fine(filePathsAndNames.toString());
logger.fine("incomingPathName: " + incomingPathPlusFileName);
if (filePathsAndNames.contains(incomingPathPlusFileName)) {
return true;
}
}
return false;
return filePathsAndNames.contains(pathPlusFilename);
}

/**
Expand Down
230 changes: 230 additions & 0 deletions src/test/java/edu/harvard/iq/dataverse/api/DuplicateFilesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import static javax.ws.rs.core.Response.Status.CREATED;
import static javax.ws.rs.core.Response.Status.OK;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.nullValue;
import org.junit.BeforeClass;
import org.junit.Test;

Expand Down Expand Up @@ -263,4 +264,233 @@ public void moveFileToDirectoryContainingSameFileName() throws IOException {

}

/**
* In this test we make sure that other changes in the absence of label and
* directoryLabel go through, such as changing a file description.
*/
@Test
public void modifyFileDescription() throws IOException {

Response createUser = UtilIT.createRandomUser();
createUser.prettyPrint();
createUser.then().assertThat()
.statusCode(OK.getStatusCode());
String username = UtilIT.getUsernameFromResponse(createUser);
String apiToken = UtilIT.getApiTokenFromResponse(createUser);

Response createDataverseResponse = UtilIT.createRandomDataverse(apiToken);
createDataverseResponse.prettyPrint();
createDataverseResponse.then().assertThat()
.statusCode(CREATED.getStatusCode());

String dataverseAlias = UtilIT.getAliasFromResponse(createDataverseResponse);

Response createDataset = UtilIT.createRandomDatasetViaNativeApi(dataverseAlias, apiToken);
createDataset.prettyPrint();
createDataset.then().assertThat()
.statusCode(CREATED.getStatusCode());

Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);

Path pathtoReadme1 = Paths.get(Files.createTempDirectory(null) + File.separator + "README.md");
Files.write(pathtoReadme1, "File 1".getBytes());
System.out.println("README: " + pathtoReadme1);

Response uploadReadme1 = UtilIT.uploadFileViaNative(datasetId.toString(), pathtoReadme1.toString(), apiToken);
uploadReadme1.prettyPrint();
uploadReadme1.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.files[0].label", equalTo("README.md"));

Integer idOfReadme1 = JsonPath.from(uploadReadme1.getBody().asString()).getInt("data.files[0].dataFile.id");
System.out.println("id: " + idOfReadme1);

JsonObjectBuilder updateFileMetadata = Json.createObjectBuilder()
.add("description", "This file is awesome.");
Response updateFileMetadataResponse = UtilIT.updateFileMetadata(String.valueOf(idOfReadme1), updateFileMetadata.build().toString(), apiToken);
updateFileMetadataResponse.prettyPrint();
updateFileMetadataResponse.then().statusCode(OK.getStatusCode());

}

/**
* In this test we make sure that that if you keep the label the same, you
* are still able to change other metadata such as file description.
*/
@Test
public void modifyFileDescriptionSameLabel() throws IOException {

Response createUser = UtilIT.createRandomUser();
createUser.prettyPrint();
createUser.then().assertThat()
.statusCode(OK.getStatusCode());
String username = UtilIT.getUsernameFromResponse(createUser);
String apiToken = UtilIT.getApiTokenFromResponse(createUser);

Response createDataverseResponse = UtilIT.createRandomDataverse(apiToken);
createDataverseResponse.prettyPrint();
createDataverseResponse.then().assertThat()
.statusCode(CREATED.getStatusCode());

String dataverseAlias = UtilIT.getAliasFromResponse(createDataverseResponse);

Response createDataset = UtilIT.createRandomDatasetViaNativeApi(dataverseAlias, apiToken);
createDataset.prettyPrint();
createDataset.then().assertThat()
.statusCode(CREATED.getStatusCode());

Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);

Path pathtoReadme1 = Paths.get(Files.createTempDirectory(null) + File.separator + "README.md");
Files.write(pathtoReadme1, "File 1".getBytes());
System.out.println("README: " + pathtoReadme1);

JsonObjectBuilder json1 = Json.createObjectBuilder()
.add("directoryLabel", "code");

Response uploadReadme1 = UtilIT.uploadFileViaNative(datasetId.toString(), pathtoReadme1.toString(), json1.build(), apiToken);
uploadReadme1.prettyPrint();
uploadReadme1.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.files[0].label", equalTo("README.md"));

Integer idOfReadme1 = JsonPath.from(uploadReadme1.getBody().asString()).getInt("data.files[0].dataFile.id");
System.out.println("id: " + idOfReadme1);

JsonObjectBuilder updateFileMetadata = Json.createObjectBuilder()
.add("label", "README.md")
.add("directoryLabel", "code")
.add("description", "This file is awesome.");
Response updateFileMetadataResponse = UtilIT.updateFileMetadata(String.valueOf(idOfReadme1), updateFileMetadata.build().toString(), apiToken);
updateFileMetadataResponse.prettyPrint();
updateFileMetadataResponse.then().statusCode(OK.getStatusCode());

}

/**
* What if the directory for the file exists and you pass the filename
* (label) while changing the description? This should be allowed.
*/
@Test
public void existingDirectoryPassLabelChangeDescription() throws IOException {

Response createUser = UtilIT.createRandomUser();
createUser.prettyPrint();
createUser.then().assertThat()
.statusCode(OK.getStatusCode());
String username = UtilIT.getUsernameFromResponse(createUser);
String apiToken = UtilIT.getApiTokenFromResponse(createUser);

Response createDataverseResponse = UtilIT.createRandomDataverse(apiToken);
createDataverseResponse.prettyPrint();
createDataverseResponse.then().assertThat()
.statusCode(CREATED.getStatusCode());

String dataverseAlias = UtilIT.getAliasFromResponse(createDataverseResponse);

Response createDataset = UtilIT.createRandomDatasetViaNativeApi(dataverseAlias, apiToken);
createDataset.prettyPrint();
createDataset.then().assertThat()
.statusCode(CREATED.getStatusCode());

Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);

Path pathToFile = Paths.get(Files.createTempDirectory(null) + File.separator + "label");
Files.write(pathToFile, "File 1".getBytes());
System.out.println("file: " + pathToFile);

JsonObjectBuilder json1 = Json.createObjectBuilder()
.add("directory", "code");

Response uploadFile = UtilIT.uploadFileViaNative(datasetId.toString(), pathToFile.toString(), json1.build(), apiToken);
uploadFile.prettyPrint();
uploadFile.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.files[0].label", equalTo("label"));

Integer idOfFile = JsonPath.from(uploadFile.getBody().asString()).getInt("data.files[0].dataFile.id");
System.out.println("id: " + idOfFile);

JsonObjectBuilder updateFileMetadata = Json.createObjectBuilder()
.add("label", "label")
.add("description", "This file is awesome.");
Response updateFileMetadataResponse = UtilIT.updateFileMetadata(String.valueOf(idOfFile), updateFileMetadata.build().toString(), apiToken);
updateFileMetadataResponse.prettyPrint();
updateFileMetadataResponse.then().statusCode(OK.getStatusCode());

}

/**
* This test is for the following scenario.
*
* What if the database has null for the directoryLabel? What if you pass in
* directory as “” (because you don’t realize you can just not pass it).
* when it check and compares, the old directory is null. so will that mean
* labelChange = true and it will fail even though you didn’t really change
* the directory?
*
* While "labelChange" does end up being true,
* IngestUtil.conflictsWithExistingFilenames returns false, so the change is
* allowed to go through. The description is allowed to be changed and the
* directoryLabel remains null even though the user passed in an empty
* string, which is what we want.
Comment on lines +432 to +436
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that if it gets to the conflictswithexisting stage and that returns false, I think we have a problem:

if conflictswithexisting filenames returns false, it’s because it’s treating ‘’ and null differently, since the name is the same). So if you did the same thing, but actually changed the name of the file to a different file name that does exist? Wouldn’t conflictswithexisting still return false then and allow you to rename to an already existing file.

The use case is you have two files:
• the existing values are null for directory for both, names are file1 and file2
• call the api to edit file1 with “” as directoryLabel and label as “file2"
• I’m concerned that labelChange would be true and conflictswithexisting would be false and that would allow you to have the same null/file2 for both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add, one solution could be to not store null for directoryLabel ever. We could either store "" or even "/". This would also help in adding a unique constraint, since unique constraints are problematic with nulls.

*/
@Test
public void existingDirectoryNullPassEmptyStringChangeDescription() throws IOException {

Response createUser = UtilIT.createRandomUser();
createUser.prettyPrint();
createUser.then().assertThat()
.statusCode(OK.getStatusCode());
String username = UtilIT.getUsernameFromResponse(createUser);
String apiToken = UtilIT.getApiTokenFromResponse(createUser);

Response createDataverseResponse = UtilIT.createRandomDataverse(apiToken);
createDataverseResponse.prettyPrint();
createDataverseResponse.then().assertThat()
.statusCode(CREATED.getStatusCode());

String dataverseAlias = UtilIT.getAliasFromResponse(createDataverseResponse);

Response createDataset = UtilIT.createRandomDatasetViaNativeApi(dataverseAlias, apiToken);
createDataset.prettyPrint();
createDataset.then().assertThat()
.statusCode(CREATED.getStatusCode());

Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);

Path pathToFile = Paths.get(Files.createTempDirectory(null) + File.separator + "file1.txt");
Files.write(pathToFile, "File 1".getBytes());
System.out.println("file: " + pathToFile);

JsonObjectBuilder json1 = Json.createObjectBuilder()
.add("description", "This is my file.");

Response uploadFile = UtilIT.uploadFileViaNative(datasetId.toString(), pathToFile.toString(), json1.build(), apiToken);
uploadFile.prettyPrint();
uploadFile.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.files[0].label", equalTo("file1.txt"));

Integer idOfFile = JsonPath.from(uploadFile.getBody().asString()).getInt("data.files[0].dataFile.id");
System.out.println("id: " + idOfFile);

JsonObjectBuilder updateFileMetadata = Json.createObjectBuilder()
// It doesn't make sense to pass "" as a directoryLabel.
.add("directoryLabel", "")
.add("description", "This file is awesome.");
Response updateFileMetadataResponse = UtilIT.updateFileMetadata(String.valueOf(idOfFile), updateFileMetadata.build().toString(), apiToken);
updateFileMetadataResponse.prettyPrint();
updateFileMetadataResponse.then().statusCode(OK.getStatusCode());

Response datasetJson = UtilIT.nativeGet(datasetId, apiToken);
datasetJson.prettyPrint();
datasetJson.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.latestVersion.files[0].label", equalTo("file1.txt"))
.body("data.latestVersion.files[0].description", equalTo("This file is awesome."))
// Even though we tried to set directoryValue to "" above, it's correctly null in the database.
.body("data.latestVersion.files[0].directoryLabel", nullValue());
}

}
22 changes: 22 additions & 0 deletions src/test/java/edu/harvard/iq/dataverse/ingest/IngestUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.sql.Timestamp;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -611,4 +612,25 @@ public void testUnfUtil() {
assertEquals("UNF:6:FWBO/a1GcxDnM3fNLdzrHw==", datasetUnfValue);
}

@Test
public void testPathPlusFilename() {
String incomingLabel = "incomingLabel";
String incomingDirectoryLabel = "incomingDirectoryLabel";
String existingLabel = "existingLabel";
String existingDirectoryLabel = "existingDirectoryLabel";
String pathPlusFilename = IngestUtil.getPathAndFileNameToCheck(incomingLabel, incomingDirectoryLabel, existingLabel, existingDirectoryLabel);
assertEquals("incomingDirectoryLabel/incomingLabel", pathPlusFilename);
}

@Test
public void renameFileToSameName() {
String pathPlusFilename = "README.md";
FileMetadata file1 = new FileMetadata();
file1.setLabel("README.md");
FileMetadata file2 = new FileMetadata();
file2.setLabel("README2.md");
List<FileMetadata> fileMetadatas = Arrays.asList(file1, file2);
assertTrue(IngestUtil.conflictsWithExistingFilenames(pathPlusFilename, fileMetadatas));
}

}