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 6 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
28 changes: 21 additions & 7 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 @@ -392,16 +392,30 @@ public Response updateFileMetadata(@FormDataParam("jsonData") String 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 the user is trying to change the label/directoryLabel or not.
boolean labelChange = true;
String oldLabel = df.getFileMetadata().getLabel();
String oldDirectoryLabel = df.getFileMetadata().getDirectoryLabel();
String oldPathPlusName = oldDirectoryLabel + "/" + oldLabel;
if (directoryLabel == null) {
directoryLabel = oldDirectoryLabel;
}
if (label == null) {
label = df.getFileMetadata().getLabel();
label = oldLabel;
}
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)));
String incomingPathPlusName = directoryLabel + "/" + label;
if (oldPathPlusName.equals(incomingPathPlusName)) {
labelChange = false;
}
logger.fine("For file id " + df.getId() + " user is trying to change the label: " + labelChange);
if (labelChange && IngestUtil.conflictsWithExistingFilenames(label, directoryLabel, fmdList, df)) {
String pathPlusFilename = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just for the error message, but why not just use "incomingPathPlusName" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if we do (I just tried), we get "null/" as part of the output. Tests fail with messages like this:

Expected: Filename already exists at README.md
  Actual: Filename already exists at null/README.md

That is to say, that code (below) is there to make sure there's no "null/" in the output, like above.

String pathPlusFilename = "";
if (directoryLabel != null) {
    pathPlusFilename = directoryLabel + "/" + label;
} else {
    pathPlusFilename = label;
}

if (directoryLabel != null) {
pathPlusFilename = directoryLabel + "/" + label;
} else {
pathPlusFilename = label;
}
return error(BAD_REQUEST, BundleUtil.getStringFromBundle("files.api.metadata.update.duplicateFile", Arrays.asList(pathPlusFilename)));
}

optionalFileParams.addOptionalParams(upFmd);
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());
}

}