Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding setters for system properties #515

Closed
wants to merge 1 commit into from

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Feb 3, 2021

Relates to #513

Adding those parameters in constructor bring some problems:

  • These classes already have some constructors and we cannot modify them. So I would need to create duplicates to add these parameters.
  • Doesn't look very well when there are many parameters to be set.

So I thought about keeping constructors and creating a builder. Then setting parameters is easy, but it has the disadvantage that scales badly with heritage. These classes are somehow thought to be extended.

And finally, the thing that fits better in this scenario is adding some setters. It has the disadvantage that it is not thread safe, but anyway there are other fields that are not thread safe already. The user must know that same instances of this class are not thought to be modified by different threads.

@jbescos jbescos force-pushed the issue513 branch 3 times, most recently from 6342a0a to f197255 Compare February 4, 2021 06:52
@chrisrueger
Copy link

Looks like a reasonable fix for #513 👍

@jbescos jbescos force-pushed the issue513 branch 3 times, most recently from 0938c78 to 06b7e50 Compare February 11, 2021 10:07
@chrisrueger
Copy link

maybe you can join the discussion in #513 ? there is an alternativ proposal. @jbescos

@jbescos jbescos marked this pull request as draft February 15, 2021 08:00
@jbescos
Copy link
Member Author

jbescos commented Feb 15, 2021

@chrisrueger I read it and I will need to try it. I am busy with other tasks right now but I will come back to this. I am converting this PR to draft for the time being.

@jbescos
Copy link
Member Author

jbescos commented Mar 1, 2021

I have added changes related to session properties. There are no setters but properties are protected, so it can be modified in other ways (not only via session properties).

@jbescos
Copy link
Member Author

jbescos commented Mar 5, 2021

There are 2 tests failing:

FAILED........javasoft/sqe/tests/jakarta/mail/Message/setFileNameTest_encodeFalse_decodeTrue.java#setFileNameTest_encodeFalse_decodeTrue
FAILED........com/sun/tdk/signaturetest/SignatureTest.java

@lukasj
Copy link
Contributor

lukasj commented Mar 5, 2021

In the mail-tck repo

@jbescos
Copy link
Member Author

jbescos commented Mar 8, 2021

There is a signature test failing. It complains with the added fields and the constructor. Do we modify that test to not fail?.

There is other test failing that I need to investigate: api/javasoft/sqe/tests/jakarta/mail/Message/testlist.html#setFileNameTest_encodeFalse_decodeTrue: setFileName() API(s) with mail.mime.encodefilename=false and mail.mime.decodefilename=true

Added Fields
------------

jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.allowutf8
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.cacheMultipart
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.decodeFileName
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.encodeFileName
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.ignoreMultipartEncoding
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.setContentTypeFileName
jakarta.mail.internet.MimeBodyPart:     field protected boolean jakarta.mail.internet.MimeBodyPart.setDefaultTextCharset
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.allowutf8
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.cacheMultipart
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.decodeFileName
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.encodeFileName
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.ignoreMultipartEncoding
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.setContentTypeFileName
jakarta.mail.internet.MimeMessage:      field protected boolean jakarta.mail.internet.MimeMessage.setDefaultTextCharset
jakarta.mail.internet.MimePartDataSource:                   field protected boolean jakarta.mail.internet.MimePartDataSource.ignoreMultipartEncoding

Added Constructors
------------------

jakarta.mail.internet.MimeBodyPart:     constructor public jakarta.mail.internet.MimeBodyPart.<init>(jakarta.mail.Session,java.io.InputStream) throws jakarta.mail.MessagingException

duplicate messages suppressed: 7

STATUS:Failed.16 errors
----------out2:(0/0)----------
result: Failed. 16 errors


test result: Failed. 16 errors

@jbescos
Copy link
Member Author

jbescos commented Mar 8, 2021

I investigated this test:
api/javasoft/sqe/tests/jakarta/mail/Message/testlist.html#setFileNameTest_encodeFalse_decodeTrue

The issue is that the system properties are set after MimeMessage is instanced, so that properties are ignored because MimeMessage makes use of them before.

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>

Move methods from jakarta to com.sun.mail

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>

Use session properties instead of setters

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos
Copy link
Member Author

jbescos commented Mar 8, 2021

I have created a jakartaee/mail-tck#24 for the issue of: api/javasoft/sqe/tests/jakarta/mail/Message/testlist.html#setFileNameTest_encodeFalse_decodeTrue

Now it is failing only the SignatureTest, but I need advise about how to proceed.

@jbescos
Copy link
Member Author

jbescos commented Aug 27, 2021

I'm closing this PR and opening a new one: #578

@jbescos jbescos closed this Aug 27, 2021
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