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:
[email protected]
