Modified DropOffUtil to put all file extensions in FILEXT #160

dlmarion · 2021-06-22T14:50:38Z

Closes #159

Closes NationalSecurityAgency#159

…er in the file name

fbruton · 2021-06-23T19:49:09Z

In the case of multiple file extensions, should the last file extension be preserved as a separate value?

dlmarion · 2021-06-23T19:52:52Z

That was not mentioned in the internal ticket.

drivenflywheel · 2021-06-23T20:17:42Z

src/main/java/emissary/output/DropOffUtil.java

@@ -1019,12 +1019,12 @@ public void processMetadata(final List<IBaseDataObject> payloadList) {
            parentTypes.put("" + level, p.getFileType());

            final String fn = p.getStringParameter("Original-Filename");


I might be misinterpreting the ticket intent, but I understand it as handling multiple values in the Original-Filename parameter, more like:

Suggested change

final String fn = p.getStringParameter("Original-Filename");

if (p.hasParameter("Original-Filename")) {

List<Object> fileNames = p.getParameter("Original-Filename");

for (Object filename : fileNames) {

final String fn = (String) filename;

I re-read the issue and modified the code in dedf4c9

drivenflywheel · 2021-06-23T20:20:01Z

src/main/java/emissary/output/DropOffUtil.java

                    if (fext.length() > 0 && fext.length() <= this.maxFilextLen) {
-                        p.setParameter("FILEXT", fext.toLowerCase());
+                        p.setParameter("FILEXT", fext);


If the above comment re: multiple Original-Filename values is correct, this will need to change so the extensions from all Original-Filename values are preserved.

This has been addressed in dedf4c9

drivenflywheel · 2021-06-24T14:35:49Z

In the case of multiple file extensions, should the last file extension be preserved as a separate value?

That's a good question, and probably needs clarification. My first instinct is that this (last extension) would be more aligned with how the data might be searched for.

dlmarion · 2021-06-28T15:28:08Z

Re-reading issue it seems that maybe I misunderstood the original requirements. It seems "Original-Filename" may contain multiple file names. I was thinking the issue was with file extensions. I'll rework.

…al-Filename

dlmarion · 2021-06-28T15:53:24Z

Note that there is a change in the preserved file extension in this change. Prior to this change the code used the last occurrence of . and this change uses the first. For a file foo.tar.gz the file extension prior to this change would have been .gz and this change makes it tar.gz.

drivenflywheel

Note that there is a change in the preserved file extension in this change. Prior to this change the code used the last occurrence of . and this change uses the first. For a file foo.tar.gz the file extension prior to this change would have been .gz and this change makes it tar.gz.

Verbal feedback I've gotten from the original requestor is that this should continue to extract only after the last occurrence of .. Same extraction as pre-PR, but now extracting the "last file extension" from each of the values stored under the "Original-filename" key

…nsions on a file

dlmarion · 2021-07-26T14:24:11Z

Verbal feedback I've gotten from the original requestor is that this should continue to extract only after the last occurrence of .. Same extraction as pre-PR, but now extracting the "last file extension" from each of the values stored under the "Original-filename" key

Addressed in d80fff0

fbruton · 2021-07-29T23:29:29Z

src/main/java/emissary/output/DropOffUtil.java

-                    if (fext.length() > 0 && fext.length() <= this.maxFilextLen) {
-                        p.setParameter("FILEXT", fext.toLowerCase());
+            if (p.hasParameter("Original-Filename")) {
+                final List<String> extensions = new ArrayList<>();


Recommend extracting lines 1022:1034 to a new method.

What's the rationale for the recommendation? Is this duplicated elsewhere? Where you looking for an associated test for this block of code?

Agreed that this method extraction would make the processMetadata method more coherent, but I don't consider that a blocker for the PR. We see virtually the same pattern immediately after this FILEXT processing (https://github.com/NationalSecurityAgency/emissary/pull/160/files#diff-5e7393b5cf28de1fa844df12051fecba96c6962dcbf4aaa3be6b7c233677e410R1040-R1060 would also benefit from a similar method extraction).

Just concerned about the overall length and complexity of processMetadata. I believe extracting to method will make it more readable.

Addressed in 78aa260

drivenflywheel · 2021-10-01T15:59:08Z

@dlmarion - can you rebase this onto the lastest master at your convenience? (rebasing will remove most of the CodeQL security alerts)
@fbruton - can you gave this a final look after it's been rebased?

No reason this little PR is still languishing.

…curityAgency#160) * Modified DropOffUtil to put all file extensions in FILEXT Closes NationalSecurityAgency#159 * Fixed assert message * Guard against the position of the first period being the last character in the file name * Modified DropOffUtil to add a file extension for each entry in Original-Filename * Modified to only use last file extension when there are multiple extensions on a file * Move file extension extraction code to it's own method

dlmarion added 3 commits June 22, 2021 14:49

Modified DropOffUtil to put all file extensions in FILEXT

5796852

Closes NationalSecurityAgency#159

Fixed assert message

ad00b99

Guard against the position of the first period being the last charact…

e1b9bc6

…er in the file name

drivenflywheel reviewed Jun 24, 2021

View reviewed changes

jpdahlke added this to the v6.7.0 milestone Jun 25, 2021

Modified DropOffUtil to add a file extension for each entry in Origin…

dedf4c9

…al-Filename

dlmarion requested a review from drivenflywheel July 1, 2021 11:55

drivenflywheel requested changes Jul 9, 2021

View reviewed changes

dlmarion added 2 commits July 26, 2021 14:16

Merge branch 'master' into 159-filext-all-extensions

ab61326

Modified to only use last file extension when there are multiple exte…

d80fff0

…nsions on a file

drivenflywheel approved these changes Jul 26, 2021

View reviewed changes

fbruton reviewed Jul 29, 2021

View reviewed changes

Move file extension extraction code to it's own method

78aa260

drivenflywheel approved these changes Aug 2, 2021

View reviewed changes

jpdahlke removed this from the v7.1.0 milestone Sep 29, 2021

Merge branch 'master' into 159-filext-all-extensions

c73d0dd

jpdahlke added this to the 7.2.0 milestone Oct 13, 2021

dev-mlb approved these changes Nov 6, 2021

View reviewed changes

dev-mlb merged commit 2a59ae5 into NationalSecurityAgency:master Nov 6, 2021

drivenflywheel mentioned this pull request Nov 12, 2021

extractFileExtensions should only extract extensions after a '.' #179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified DropOffUtil to put all file extensions in FILEXT #160

Modified DropOffUtil to put all file extensions in FILEXT #160

dlmarion commented Jun 22, 2021

fbruton commented Jun 23, 2021

dlmarion commented Jun 23, 2021

drivenflywheel Jun 23, 2021

dlmarion Jun 28, 2021

drivenflywheel Jun 23, 2021

dlmarion Jun 28, 2021

drivenflywheel commented Jun 24, 2021

dlmarion commented Jun 28, 2021

dlmarion commented Jun 28, 2021

drivenflywheel left a comment

dlmarion commented Jul 26, 2021

fbruton Jul 29, 2021

dlmarion Jul 30, 2021

drivenflywheel Jul 30, 2021

fbruton Jul 31, 2021

dlmarion Aug 2, 2021

drivenflywheel commented Oct 1, 2021 •

edited

Loading

		@@ -1019,12 +1019,12 @@ public void processMetadata(final List<IBaseDataObject> payloadList) {
		parentTypes.put("" + level, p.getFileType());

		final String fn = p.getStringParameter("Original-Filename");

-            final String fn = p.getStringParameter("Original-Filename");
+           if (p.hasParameter("Original-Filename")) {
+                List<Object> fileNames = p.getParameter("Original-Filename");
+                for (Object filename : fileNames) {
+                    final String fn = (String) filename;

Modified DropOffUtil to put all file extensions in FILEXT #160

Modified DropOffUtil to put all file extensions in FILEXT #160

Conversation

dlmarion commented Jun 22, 2021

fbruton commented Jun 23, 2021

dlmarion commented Jun 23, 2021

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

drivenflywheel commented Jun 24, 2021

dlmarion commented Jun 28, 2021

dlmarion commented Jun 28, 2021

drivenflywheel left a comment

Choose a reason for hiding this comment

dlmarion commented Jul 26, 2021

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

drivenflywheel commented Oct 1, 2021 • edited Loading

drivenflywheel commented Oct 1, 2021 •

edited

Loading