You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by rpmiskin <gi...@git.apache.org> on 2016/01/24 10:39:25 UTC
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
GitHub user rpmiskin opened a pull request:
https://github.com/apache/nifi/pull/187
NIFI-1434 Prevent array index exception in PutEmail
I've updated PutEmail to avoid an ArrayIndexOutOfBoundsException in the case where the FROM property evaluates to an empty string.
Reading through code in MimeMessage, message.addFrom(InternetAddress[]) is equivalent to message.setFrom(String) in the case where the array has length 1 and will clear the sender when the array has length 0 (rather than throwing an exception).
I have not made any changes to TestPutEmail. To provide more complete testing I think it would be necessary to do a little refactoring to allow the static call to Transport.send() to be stubbed. Possibly introducing a PutEmail.send() method that would be overridden when testing?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rpmiskin/nifi NIFI-1434
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/187.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #187
----
commit 95c80b433056bd161c5ed64fa24186ac8b46485c
Author: Richard Miskin <r....@gmail.com>
Date: 2016-01-24T09:07:57Z
NIFI-1434 Prevent array index exception in PutEmail
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the pull request:
https://github.com/apache/nifi/pull/187#issuecomment-178328472
Hello @rpmiskin Do you mind closing this PR? Looks like @JPercivall just forgot to amend the comit entry to ensure it said 'This closes #187' but it does show as merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by rpmiskin <gi...@git.apache.org>.
Github user rpmiskin commented on the pull request:
https://github.com/apache/nifi/pull/187#issuecomment-177163297
@JPercivall I've pushed an updated version of this branch which ensures that required properties evaluate to valid InternetAddress objects.
I've also made it possible to stub out `Transport.send()` and added some additional unit tests that check what is actually being set on the Message.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by rpmiskin <gi...@git.apache.org>.
Github user rpmiskin commented on the pull request:
https://github.com/apache/nifi/pull/187#issuecomment-176588865
@JPercivall
I believe SMTPTransport will pick up the from address from a couple of other places in preference to the FROM header, so I don't think it should necessarily be required.
From com.sun.mail.smtp.SMTPTransport.mailFrom():
Issue the MAIL FROM: command to start sending a message.
Gets the sender's address in the following order:
SMTPMessage.getEnvelopeFrom()
mail.smtp.from property
From: header in the message
System username using the InternetAddress.getLocalAddress() method
How about I update it so that the FROM property descriptor is optional, but if it is set it must evaluate to a valid InternetAddress? Should the same logic be applied to the other InternetAddress values?
Thanks for creating the other jira issue, I'll add some tests as part of my change but probably won't look to get 100% coverage of existing functionality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the pull request:
https://github.com/apache/nifi/pull/187#issuecomment-176807151
@rpmiskin
Following the RFC 822 guideline (that the MimeMessage class is based on) it says this about the "From" header:
4.4.1. FROM / RESENT-FROM
This field contains the identity of the person(s) who wished
this message to be sent. The message-creation process should
default this field to be a single, authenticated machine
address, indicating the AGENT (person, system or process)
entering the message. If this is not done, the "Sender" field
MUST be present. If the "From" field IS defaulted this way,
the "Sender" field is optional and is redundant with the
"From" field. In all cases, addresses in the "From" field
must be machine-usable (addr-specs) and may not contain named
lists (groups).
MimeMessage doesn't recognize the "Sender" field as being different from "From" (in comments uses the terms interchangeably) so I'd say leave the "From" header as required.
I am on board with creating a custom validator for the internet addresses though, that would be great.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the pull request:
https://github.com/apache/nifi/pull/187#issuecomment-176558911
There 100% needs to be better unit testing for the PutEmail processor and I agree that this ticket is outside the scope of that. I've created a new one here[1]. Stubbing things out is a valid approach since the unit test can't embed an smtp server. Feel free to comment there with more ideas!
[1] https://issues.apache.org/jira/browse/NIFI-1449
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1434 Prevent array index exception in PutE...
Posted by rpmiskin <gi...@git.apache.org>.
Github user rpmiskin closed the pull request at:
https://github.com/apache/nifi/pull/187
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---