Skip to content

[NIFI-2810] Allow Content Type to be set in PutS3Object processor#1034

Closed
vegaed wants to merge 3 commits intoapache:masterfrom
vegaed:master
Closed

[NIFI-2810] Allow Content Type to be set in PutS3Object processor#1034
vegaed wants to merge 3 commits intoapache:masterfrom
vegaed:master

Conversation

@vegaed
Copy link

@vegaed vegaed commented Sep 20, 2016

@trixpan
Copy link
Contributor

trixpan commented Sep 22, 2016

@vegaed

Thanks for looking at appveyor. Do you mind cherry picking 7a2fc08 into a separate PR?

@vegaed
Copy link
Author

vegaed commented Sep 22, 2016

@trixpan moved it out of this PR

@pvillard31
Copy link
Contributor

@vegaed LGTM. Do you have an associated JIRA for this? If not, could you create one and link it to this PR?

@vegaed vegaed changed the title Allow Content Type to be set in PutS3Object processor [NIFI-2810] Allow Content Type to be set in PutS3Object processor Sep 22, 2016
@vegaed
Copy link
Author

vegaed commented Sep 22, 2016

@pvillard31 I think I did it right should be good to go now.

Copy link
Contributor

@pvillard31 pvillard31 left a comment

Choose a reason for hiding this comment

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

@vegaed I made a couple of comments, let me know your thoughts.

.build();

public static final PropertyDescriptor CONTENT_TYPE = new PropertyDescriptor.Builder()
.name("Content Type")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the combination of .name() and .displayName()? name ensures backward compatibility when displayName allows to be more flexible on what is displayed to users. We are trying to ensure every new processor properties is using both.

Copy link
Author

Choose a reason for hiding this comment

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

Done

"AWS S3 Java client will attempt to determine the correct content type if one hasn't been set" +
" yet. Users are responsible for ensuring a suitable content type is set when uploading streams. If " +
"no content type is provided and cannot be determined by the filename, the default content type " +
"\"application/octet-stream\" will be used.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you set "application/octet-stream" as default value of the property? It's more user-friendly because user will know what is the default value which is used without looking at the description.

Copy link
Author

Choose a reason for hiding this comment

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

Well if I do that then the default behavior will not kick in. This is an optional property. In our case the client couldn't figure out the type so we needed to tell it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK yeah, understood!


runner.assertAllFlowFilesTransferred(PutS3Object.REL_SUCCESS, 1);
List<MockFlowFile> flowFiles = runner.getFlowFilesForRelationship(PutS3Object.REL_SUCCESS);
MockFlowFile ff1 = flowFiles.get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand the unit test correctly, there will be a new attribute written into the flow file (s3.contenttype). Could you add the corresponding @WritesAttribute annotation at the top of the PutS3Object class?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@asfgit asfgit closed this in b34de74 Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants