allow changes to file metadata via API #6962 #6968

pdurbin · 2020-06-05T20:20:22Z

What this PR does / why we need it:

A regression was introduced in pull request #6893 whereby modifying file metadata API failed with "Filename already exists". This bug is in develop, not production.

Which issue(s) this PR closes:

Closes #6962

Special notes for your reviewer:

None.

Suggestions on how to test this:

Try updating file metadata via API in develop vs this pull request. Docs: http://guides.dataverse.org/en/4.20/api/native-api.html#updating-file-metadata
Ensure the FilesIT is passing

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

No.

Additional documentation:

None.

coveralls · 2020-06-05T20:27:20Z

Coverage increased (+0.008%) to 19.593% when pulling 2debed2 on 6962-bugfix into 97e7a60 on develop.

pdurbin · 2020-06-08T16:31:37Z

I'm putting this back in code review after fixing a bug @scolapasta found. Now an API user can pass in the existing label or directoryLabel and we still allow edits to go through.

scolapasta

A couple of small comments, general idea is good.

scolapasta · 2020-06-08T19:53:03Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

+                String oldLabel = df.getFileMetadata().getLabel();
+                String oldDirectoryLabel = df.getFileMetadata().getDirectoryLabel();
+                String oldPathPlusName = oldDirectoryLabel + oldLabel;
+                String incomingPathPlusName = directoryLabel + label;


does this do weird things if either one is null? i.e I would expect if you only passed on in, and it was the same, then there should be no labelChange, but I think the code for the equals would be false.

I figured what matters in terms of if there was a change or not is if the "path + filename" is different, so that's what I'm comparing. No, weird things do not happen if one is null, according to my testing.

Sorry, maybe what I wrote wasn't clear, so let me use an example:
oldDirectory = "directory" and old label = "label"

If I only path in a label as "label" (i.e. no directory), I would expect it to allow it, as I am not changing anything. But the label check would compare "directorylabel" with "nulllabel" and so would run the check when it shouldn't.

Also if (for some reason) for new values you passed directory = "director" and label = "ylabel", the check would find them equal and so not check names, even though you are changing the names.

So the check also needs to have a "/" in there.

Good catch. I fixed both of these.

scolapasta · 2020-06-08T19:54:20Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

                String path = "";
                if (directoryLabel != null) {
                    path = directoryLabel + "/";
                }
                if (label == null) {
-                    label = df.getFileMetadata().getLabel();
+                    label = oldLabel;
                }
-                if (IngestUtil.conflictsWithExistingFilenames(label, directoryLabel, fmdList, df)) {


if directoryLabel is not passed in, do we want path to be ""? or keep the old value (as you do with label)?

path is only used in the error message...

"Filename already exists at README.md"

"Filename already exists at code/README.md"

... and I'm happy with how it works. So yes, I think leaving it as "" (empty string) is fine.

Sorry path confused me here, what I'm getting it is:

Why don't we have have a
if (directoryLabel == null) { directoryLabel = oldDrectorylLabel; }

like with label, otherwise null gets passed into the directory+name check, no? (and if we did this, we could really get rid of path altogether and just use directoryLabel + /. And this way if directorLabel were null, we would get a more accurate message anyway:

"Filename already exists at code/README.md", when "Filename already exists at README.md" isn't accurate?

I got rid of the extra path variable and made sure the messages are accurate.

pdurbin · 2020-06-08T20:17:19Z

@scolapasta I responded to your comments.

scolapasta · 2020-06-08T20:31:43Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

+                String oldLabel = df.getFileMetadata().getLabel();
+                String oldDirectoryLabel = df.getFileMetadata().getDirectoryLabel();
+                String oldPathPlusName = oldDirectoryLabel + oldLabel;
+                String incomingPathPlusName = directoryLabel + label;


Sorry, maybe what I wrote wasn't clear, so let me use an example:
oldDirectory = "directory" and old label = "label"

If I only path in a label as "label" (i.e. no directory), I would expect it to allow it, as I am not changing anything. But the label check would compare "directorylabel" with "nulllabel" and so would run the check when it shouldn't.

Also if (for some reason) for new values you passed directory = "director" and label = "ylabel", the check would find them equal and so not check names, even though you are changing the names.

So the check also needs to have a "/" in there.

scolapasta · 2020-06-08T20:35:13Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

                String path = "";
                if (directoryLabel != null) {
                    path = directoryLabel + "/";
                }
                if (label == null) {
-                    label = df.getFileMetadata().getLabel();
+                    label = oldLabel;
                }
-                if (IngestUtil.conflictsWithExistingFilenames(label, directoryLabel, fmdList, df)) {


Sorry path confused me here, what I'm getting it is:

Why don't we have have a
if (directoryLabel == null) { directoryLabel = oldDrectorylLabel; }

like with label, otherwise null gets passed into the directory+name check, no? (and if we did this, we could really get rid of path altogether and just use directoryLabel + /. And this way if directorLabel were null, we would get a more accurate message anyway:

"Filename already exists at code/README.md", when "Filename already exists at README.md" isn't accurate?

pdurbin · 2020-06-09T16:10:46Z

I responded to code review with a couple commits and comments.

scolapasta

Ver minor cleanup, but I think simplifies the code for maintainability.

scolapasta · 2020-06-09T18:32:12Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

-                    path = directoryLabel + "/";
+                // If the user is trying to change the label/directoryLabel or not.
+                boolean labelChange = true;
+                if (label == null && directoryLabel == null) {


Last suggested change - we can remove this check since it's handled by the code below (if both are null, they get populated with their old values). Simplifies the code for minimal "cost".

Good idea. Removed in 491caff.

scolapasta · 2020-06-09T18:39:10Z

src/main/java/edu/harvard/iq/dataverse/api/Files.java

-                    String incomingPathPlusFileName = path + label;
-                    return error(BAD_REQUEST, BundleUtil.getStringFromBundle("files.api.metadata.update.duplicateFile", Arrays.asList(incomingPathPlusFileName)));
+                if (labelChange && IngestUtil.conflictsWithExistingFilenames(label, directoryLabel, fmdList, df)) {
+                    String pathPlusFilename = "";


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

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 null, populated by old values anyway.

pdurbin · 2020-06-09T20:44:19Z

I removed the code we don't need (good catch) and left a comment about the output.

pdurbin · 2020-06-09T21:04:28Z

Not to self, while old directory is null, pass in "" as the directory.

pdurbin · 2020-06-10T15:17:55Z

Not to self, while old directory is null, pass in "" as the directory.

@scolapasta I addressed this in a34d7c1

scolapasta · 2020-06-11T15:06:50Z

src/test/java/edu/harvard/iq/dataverse/api/DuplicateFilesIT.java

+     * 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.


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.

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.

pdurbin · 2020-06-15T16:16:48Z

I attempted to simplify the code in 2debed2. DuplicatesIT and FilesIT both pass.

scolapasta

OK, this way looks cleaner and should cover issues with null.

allow changes to file metadata via API #6962

809643a

pdurbin mentioned this pull request Jun 5, 2020

quick fix to get test passing in FilesIT #6962 #6964

Closed

pdurbin self-assigned this Jun 8, 2020

allow file metadata changes if passing existing path/name #6962

508e95c

pdurbin removed their assignment Jun 8, 2020

scolapasta requested changes Jun 8, 2020

View reviewed changes

pdurbin self-assigned this Jun 8, 2020

allow changes if dir exists and label passed, formatting #6962

9954220

pdurbin assigned pdurbin and unassigned pdurbin Jun 9, 2020

avoid dir1 + file1 == dir + 1file1 (add "/") #6962

e5ec9bc

pdurbin removed their assignment Jun 9, 2020

scolapasta requested changes Jun 9, 2020

View reviewed changes

pdurbin self-assigned this Jun 9, 2020

eliminate code we don't need, handled elsewhere #6962

491caff

If null, populated by old values anyway.

pdurbin removed their assignment Jun 9, 2020

pdurbin self-assigned this Jun 9, 2020

ensure passing "" as directoryLabel doesn't cause problems #6962

a34d7c1

pdurbin removed their assignment Jun 10, 2020

djbrooke assigned scolapasta Jun 11, 2020

scolapasta requested changes Jun 11, 2020

View reviewed changes

djbrooke added this to the Dataverse 5 milestone Jun 11, 2020

scolapasta assigned 4tikhonov Jun 11, 2020

djbrooke unassigned 4tikhonov and scolapasta Jun 11, 2020

simply conflict-checking code #6962

2debed2

scolapasta approved these changes Jun 16, 2020

View reviewed changes

kcondon self-assigned this Jun 22, 2020

This was referenced Jun 25, 2020

Get API test suite passing 😴 😴 😴 #7020

Closed

API tests are failing, add sleep #7020 #7022

Closed

kcondon merged commit bc06fe6 into develop Jul 1, 2020

kcondon deleted the 6962-bugfix branch July 1, 2020 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow changes to file metadata via API #6962 #6968

allow changes to file metadata via API #6962 #6968

pdurbin commented Jun 5, 2020

coveralls commented Jun 5, 2020 •

edited

Loading

pdurbin commented Jun 8, 2020

scolapasta left a comment

scolapasta Jun 8, 2020

pdurbin Jun 8, 2020

scolapasta Jun 8, 2020

pdurbin Jun 9, 2020

scolapasta Jun 8, 2020

pdurbin Jun 8, 2020

scolapasta Jun 8, 2020

pdurbin Jun 9, 2020

pdurbin commented Jun 8, 2020

scolapasta Jun 8, 2020

scolapasta Jun 8, 2020

pdurbin commented Jun 9, 2020

scolapasta left a comment

scolapasta Jun 9, 2020

pdurbin Jun 9, 2020

scolapasta Jun 9, 2020

pdurbin Jun 9, 2020

pdurbin commented Jun 9, 2020

pdurbin commented Jun 9, 2020

pdurbin commented Jun 10, 2020

scolapasta Jun 11, 2020

scolapasta Jun 11, 2020

pdurbin commented Jun 15, 2020

scolapasta left a comment

allow changes to file metadata via API #6962 #6968

allow changes to file metadata via API #6962 #6968

Conversation

pdurbin commented Jun 5, 2020

coveralls commented Jun 5, 2020 • edited Loading

pdurbin commented Jun 8, 2020

scolapasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented Jun 9, 2020

scolapasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented Jun 9, 2020

pdurbin commented Jun 9, 2020

pdurbin commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented Jun 15, 2020

scolapasta left a comment

Choose a reason for hiding this comment

coveralls commented Jun 5, 2020 •

edited

Loading