[GitHub] [nifi] turcsanyip commented on a change in pull request #4423: NIFI-7664 - Add Content Disposition property to PutS3Object processor

[GitBox <gi...@apache.org>]

turcsanyip commented on a change in pull request #4423:
URL: https://github.com/apache/nifi/pull/4423#discussion_r464122120



##########
File path: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestPutS3Object.java
##########
@@ -171,12 +172,11 @@ public void testFilenameWithNationalCharacters() throws UnsupportedEncodingExcep
 
         runner.run(1);
 
-        ArgumentCaptor<PutObjectRequest> captureRequest = ArgumentCaptor.forClass(PutObjectRequest.class);
-        Mockito.verify(mockS3Client, Mockito.times(1)).putObject(captureRequest.capture());
-        PutObjectRequest request = captureRequest.getValue();
+        runner.assertAllFlowFilesTransferred(PutS3Object.REL_SUCCESS, 1);
+        List<MockFlowFile> flowFiles = runner.getFlowFilesForRelationship(PutS3Object.REL_SUCCESS);
+        MockFlowFile ff = flowFiles.get(0);
 
-        ObjectMetadata objectMetadata = request.getMetadata();
-        assertEquals("Iñtërnâtiônàližætiøn.txt", URLDecoder.decode(objectMetadata.getContentDisposition(), "UTF-8"));
+        assertEquals("Iñtërnâtiônàližætiøn.txt", URLDecoder.decode(ff.getAttribute(CoreAttributes.FILENAME.key()), "UTF-8"));

Review comment:
       This test was written to test if the Content-Disposition header is being passed to AWS in URL encoded form.
   So to check if the Content-Disposition is `I%C3%B1t%C3%ABrn%C3%A2ti%C3%B4n%C3%A0li%C5%BE%C3%A6ti%C3%B8n.txt` for the test data.
   
   The `filename` FlowFile attribute is not URL encoded, it simply contains `Iñtërnâtiônàližætiøn.txt`. URLDecoder does not do anything with it and the assert will always be true.
   
   Please revert back to the original test if there is no other reason to modify that.
   
   One change in the assert seems to me reasonable though:
   `assertEquals(URLEncoder.encode("Iñtërnâtiônàližætiøn.txt", "UTF-8"), objectMetadata.getContentDisposition());`
   (the original assert would be true even if the Content-Disposition was not URL encoded)

##########
File path: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/FetchS3Object.java
##########
@@ -165,14 +165,19 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
 
             final ObjectMetadata metadata = s3Object.getObjectMetadata();
             if (metadata.getContentDisposition() != null) {
-                final String fullyQualified = metadata.getContentDisposition();
-                final int lastSlash = fullyQualified.lastIndexOf("/");
-                if (lastSlash > -1 && lastSlash < fullyQualified.length() - 1) {
-                    attributes.put(CoreAttributes.PATH.key(), fullyQualified.substring(0, lastSlash));
-                    attributes.put(CoreAttributes.ABSOLUTE_PATH.key(), fullyQualified);
-                    attributes.put(CoreAttributes.FILENAME.key(), fullyQualified.substring(lastSlash + 1));
+                final String contentDisposition = metadata.getContentDisposition();
+
+                if (contentDisposition.equals(PutS3Object.CONTENT_DISPOSITION_INLINE) || contentDisposition.startsWith("attachment; filename=")) {
+                    attributes.put(CoreAttributes.FILENAME.key(), key);

Review comment:
       Setting the`filename` to the full object key (eg. `dir1/dir2/file1`) is a bit inconsistent because `filename` typically does not contain the "path" (like in the else branch below and in case of other processor too).
   I would suggest to use the simple file name:
   - `dir1/dir2/file1` => `file1`
   - `file2` => `file2`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org