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

Jump to bottom

GDCC/7527-fix-redetect #8835

Merged
merged 7 commits into from
Sep 19, 2022
Merged

GDCC/7527-fix-redetect #8835

merged 7 commits into from
Sep 19, 2022

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jul 8, 2022 •
edited by pdurbin
Loading

What this PR does / why we need it: For non File Stores, redetect was using the name of the temp file and it's extension (always .tmp) to determine the mimetype, and with .tmp not mapping to a known mimetype could return null as the answer (if other detect steps didn't work). This fix provides the name from the datafile in the db and sends 'application/octet-stream' if no other determination was made.

Which issue(s) this PR closes:

Closes #7527
Closes #8821

Special notes for your reviewer: Ran across this when doing other refactoring - would be good if someone who was following the issue could verify that this fixes both aspects.

Suggestions on how to test this: per the issue.

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

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

Additional documentation:

@coveralls
Copy link

coveralls commented Jul 28, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.004%) to 20.052% when pulling 77eea56 on GlobalDataverseCommunityConsortium:GDCC/7527-fix-redetect into 6ca1c9d on IQSS:develop.

@landreev landreev self-requested a review September 6, 2022 15:49
@landreev landreev self-assigned this Sep 6, 2022
landreev
landreev approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@landreev landreev left a comment

Choose a reason for hiding this comment

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

I really thought we had fixed this a while ago; but it appears we only talked about fixing it, but never got around to it. Great to see it fixed it finally.
Looks good. (To be clear, this was broken for all stores, not just the remote kind; it wasn't working with the filesystem storage either - because it was running the check on the internal, storageidentifier-based filename).

@landreev landreev removed their assignment Sep 7, 2022
@pdurbin
Copy link
Member

pdurbin commented Sep 9, 2022

A couple questions:

@landreev landreev mentioned this pull request Sep 12, 2022
Closed
@landreev
Copy link
Contributor

No, this PR does not close #8821.
#7631 should be closed regardless, because we decided not to merge it.
Note that #7527 was 2 issues in one: a trivial issue with the file name; and a strange, not easily-reproducible EJB rollback that happened only on some files in our prod., and on the installation that first reported it.
This PR fixes the trivial part of #7527 and we should merge it.
My #7631 fixed that too, but also contained an attempt to add the @Stateless annotation to the API bean in question, which for some unknown reason made the strange EJB issue go away. But it was determined that we didn't want to do that.

I'll check if that weird part of #7527 was still happening in our prod. If it is, it would be great to actually figure out what's going on there (should not block or delay merging this PR though).

@landreev
Copy link
Contributor

Whether #8821 is in fact the same thing is as the mysterious EJB sub-issue of #7527, I'm not 100% sure, will check.

landreev
landreev reviewed Sep 12, 2022
View reviewed changes
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RedetectFileTypeCommand.java
@@ -62,7 +63,7 @@ public DataFile execute(CommandContext ctxt) throws CommandException {
}

logger.fine("target file: " + localFile);
String newlyDetectedContentType = FileTypeDetection.determineFileType(localFile);
String newlyDetectedContentType = FileUtil.determineFileType(localFile, fileToRedetect.getDisplayName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a null check in the next line here? - From looking at #8821, it appears that determineFileType() may return null here, for especially unrecognizable files; and that results in a constraint violation.
Sorry for asking for this while the PR is already in QA; but would be a trivial change. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change in FileUtil.java in this PR prevents a null return, so I think we're good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, of course, doh! Thanks!

@landreev
Copy link
Contributor

Yes, this PR closes #8821 as well. Sorry for the extra noise.

@landreev landreev mentioned this pull request Sep 12, 2022
Closed
@sekmiller sekmiller merged commit 454f3f1 into IQSS:develop Sep 19, 2022
@sekmiller sekmiller removed their assignment Sep 19, 2022
@pdurbin pdurbin added this to the 5.12 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@landreev landreev landreev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.12
Development

Successfully merging this pull request may close these issues.

Bean Validation errors calling file redetect api endpoint Error with Redetect File Type API
5 participants
@qqmyers @coveralls @pdurbin @landreev @sekmiller