Open chrisrueger opened 3 years ago
I think there is a more generalized solution that should be explored when dealing with system properties vs session properties. One of the reasons system properties are using is that it can be tricky to gain access to the session.
However, for this case JakartaMail has an interface MessageAware that is designed to gain access to the session. For instance, MimeMultipart use this to set the parent.
I think a similar approach should be applied to the MimeBodyPart by adding a single new constructor public MimeBodyPart(DataSource ds) throws MessagingException
that reads the stream from the datasource and inpects if it is a MessageAware datasource. Ideally, it would be great if we could set the parent of the BodyPart and this point as it allows getting the session from the parent. However, it may not be possible to determine the correct parent at this point and it may not matter either as all we need is a path to the root to gain access to the session and addPart will reset the parent anyway. Alternatively, then all we need is a refence to the MessageAware object.
Next step would be to then add a private void initializeProperties()
to init the properties from the session assigned to the parent by new MessageContext(this).getSession()
and or the session from the datasource ds.getMessageContext().getSession()
. If neither is present fallback to system properties. Another alternative would be to lookup the properties
For testing we would then have to check that in the JakartaMail itself if we need to move from MimeBodyPart(InputStream) MimeBodyPart(DataSource) to ensure the right settings are applied for other uses.
There are more details to workout but this outlines the general approach.
Thanks for the detailed suggestion.
to gain access to the session
Let me try to repeat in my words to see if I got that correctly:
So is the idea to put the configuration like mail.mime.encodefilename
and mail.mime.setcontenttypefilename
into Properties of the Session instead of system properties?
And then let MimeBodyPart
somehow find out if the property is set inside the Session
's properties (or the other thing you mentioned) .... and if not fallback to system properties.
So the missing piece at the moment is that MimeBodyPart currently has no way to get access to the session. But passing a MessageAware DataSource (like FileDataSource in my case) would allow this.
Question:
Why do you prefer putting MimeBodyPart-specific options in session properties - as opposed to set the directly on MimeBodyPart? Is this for consistency, because the system properties already exist? I mean the scope is smaller than system properties, but still larger. I am not doubting your approach, just trying to understand.
You have captured the idea of my intent. I'm sure once it is coded up, the flaws of my hand waving will come to light :).
The topic of session vs. system properties as come up before in the past. Unfortunately, I didn't draft up this approach and explain to @bshannon but were are his thoughts on a related topic:
In a mutli-threaded application, querying the property dynamically doesn't really help.
In most cases these static properties were not expected to change over the lifetime of an application.
In some cases, the use of static was an unfortunate compromise because the code that needed it didn't have access to the Session. Changing all the APIs to pass the Session through explicitly would've made the code more complex.
Thread local storage was considered, but that depends too heavily on the threading model used by the application.
Wherever possible, Session properties were used, but in some cases there was no good way to do that so System properties were used.
When I read that I see two trade-offs at play when the original code was developed:
My thought is that going with "MessageAware DataSource" is actually inline with what has already been done in the past. E.G. MimeMultipart(DataSource). If needed my approach could work without adding new methods as getParent and setDataHandler already exist on the MimeBodyPart. My initial thoughts are that my solution would satisfy both trade offs above and make the behavior more like what has been done before already in this codebase.
Adding the setters has merit but, not sure we need to be able to change the properties at that scope. The setter approach also means that we have to split methods in to classes in the Jakarta namespace and the com.sun namespace based on if they are spec or reference implementation.
I have done another commit to get the properties from session.
I found that we cannot get the InputStream from the DataSource because, although originally it comes from the DataSource, it changes during the MimeMultipart#parse. For this reason I created a new constructor having MimeBodyPart(Session session, InputStream is)
similarly as the constructor that MimeMessage has.
@jbescos Reviewing this is still on my todo list. I'll get you a proper review once I can give this a test drive.
Is your feature request related to a problem? Please describe. Some background: The reason behind this issue is our attempt to make it possible to send an Attachment to "Outlook client which has problems displaying filenames with german umlauts (ä, ü, ö etc.) correctly. Search the internet for "outlook rfc2231 rfc2047".
In the past we were using JavaMail 1.4.3 which worked out of the box with outlook. But now we are on 1.6.2 (yes, JavaMail, but we consider switching to JakartaMail. But JakartaMail still has the same problem as JavaMail 1.6.2)
Currently MimeBodyPart uses static variables for certain customisations like this:
The problem:
SystemProperties are global for the whole application. We would like to use this only in some very specific cases where a customer can e.g. pass a flag to decide wether or not to encode the filename etc.
The magic currently happens in #setFileName(part, name) which unfortunately is package private so it hard / impossible to customize it. You can see it in the comments like:
Describe the solution you'd like
Is it possible to make the private static fields such as
encodeFileName
,setContentTypeFileName
actual class members add a new constructor where you can pass the fields?This would allow us to use this constructor if we want to have full control over all parameters. Otherwise we just use the default constructors which work as it is.
e.g. example pseudo code:
I assume this would also be backwards compatible when this is a new constructor.
Describe alternatives you've considered
An alternative was to create a customized version in our own
package jakarta.mail.internet;
and overridejavax.mail.internet.MimeBodyPartSynesty.setFileName(String)
Unfortunatelly then we have to copy lots of code from #setFileName(part, name)We have not used this alternative yet, it was just a POC.
e.g.
Additional context Add any other context or screenshots about the feature request here.