vlsi commented on code in PR #6757: URL: https://github.com/apache/jmeter/pull/6757#discussion_r4098267404
########## src/protocol/mail/src/test/java/org/apache/jmeter/protocol/smtp/sampler/protocol/SendMailCommandTest.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jmeter.protocol.smtp.sampler.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collections; + +import javax.mail.Message; +import javax.mail.internet.InternetAddress; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class SendMailCommandTest { + + private static final String NON_ASCII_FILE_NAME = "\u0442\u0435\u043a\u0441\u0442.txt"; // Russian for "text" + + @TempDir + File tempDir; + + @Test + void testNonAsciiAttachmentFileNameIsEncodedPerRfc2231() throws Exception { + SendMailCommand sendMailCommand = createSendMailCommandWithAttachment(NON_ASCII_FILE_NAME); + String rawMessage = writeMessageToString(sendMailCommand.prepareMessage()); + // The file name must be encoded according to RFC 2231, so that the + // recipients see the original file name (see issue #6652) + assertTrue( + rawMessage.contains("filename*=UTF-8''%D1%82%D0%B5%D0%BA%D1%81%D1%82.txt"), Review Comment: This expectation holds only when the JVM default charset is UTF-8. On Java 17 with a non-UTF-8 default (`windows-1252` on Windows), the header is `filename*=Cp1252''%3F%3F%3F%3F%3F.txt` and the test fails, on a CI job with that setup and on a contributor's machine. Once `SendMailCommand` encodes the name as UTF-8 explicitly (see the review summary), the assertion no longer depends on the platform. Please run the test on Java 17 with `-Dfile.encoding=windows-1252` to confirm. ########## src/protocol/mail/src/test/java/org/apache/jmeter/protocol/smtp/sampler/protocol/SendMailCommandTest.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jmeter.protocol.smtp.sampler.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collections; + +import javax.mail.Message; +import javax.mail.internet.InternetAddress; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class SendMailCommandTest { + + private static final String NON_ASCII_FILE_NAME = "\u0442\u0435\u043a\u0441\u0442.txt"; // Russian for "text" + + @TempDir + File tempDir; + + @Test + void testNonAsciiAttachmentFileNameIsEncodedPerRfc2231() throws Exception { Review Comment: The file name is carried in two headers, and the test checks only `Content-Disposition`. `setFileName` also sets the `name` parameter of `Content-Type`; please assert it as well. A name longer than about 60 characters takes a different path (RFC 2231 continuations, `filename*0*=`) and deserves its own case. The `test` prefix repeats what `@Test` already says; a name such as `nonAsciiAttachmentFileNameIsEncodedAsUtf8` reads better in the report. The same applies to `testAsciiAttachmentFileNameIsNotEncoded`. ########## src/protocol/mail/src/test/java/org/apache/jmeter/protocol/smtp/sampler/protocol/SendMailCommandTest.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jmeter.protocol.smtp.sampler.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collections; + +import javax.mail.Message; +import javax.mail.internet.InternetAddress; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class SendMailCommandTest { + + private static final String NON_ASCII_FILE_NAME = "\u0442\u0435\u043a\u0441\u0442.txt"; // Russian for "text" + + @TempDir + File tempDir; + + @Test + void testNonAsciiAttachmentFileNameIsEncodedPerRfc2231() throws Exception { + SendMailCommand sendMailCommand = createSendMailCommandWithAttachment(NON_ASCII_FILE_NAME); + String rawMessage = writeMessageToString(sendMailCommand.prepareMessage()); + // The file name must be encoded according to RFC 2231, so that the + // recipients see the original file name (see issue #6652) + assertTrue( + rawMessage.contains("filename*=UTF-8''%D1%82%D0%B5%D0%BA%D1%81%D1%82.txt"), + "filename* parameter with RFC 2231 encoded file name expected in:\n" + rawMessage); + // The mangled name must not appear anywhere + assertFalse(rawMessage.contains("B5:AB"), "mangled file name found in:\n" + rawMessage); Review Comment: This assertion pins one particular way the old version mangled the name and misses the others: the `?????.txt` output above passes it. Please parse the message back and compare the decoded name, which fails on any mangling and prints both values: ```java MimeMessage parsed = new MimeMessage(null, new ByteArrayInputStream(rawBytes)); BodyPart attachment = ((Multipart) parsed.getContent()).getBodyPart(1); assertEquals(NON_ASCII_FILE_NAME, attachment.getFileName(), "decoded attachment file name"); ``` Keep the `filename*=` check next to it if the test is meant to pin the RFC 2231 form. ########## src/bom-thirdparty/build.gradle.kts: ########## @@ -75,7 +75,7 @@ dependencies { api("io.burt:jmespath-jackson:0.6.0") api("jakarta.jms:jakarta.jms-api:3.1.0") api("javax.activation:javax.activation-api:1.2.0") - api("javax.mail:mail:1.5.0-b01") + api("com.sun.mail:javax.mail:1.6.2") Review Comment: `src/dist/src/dist/expected_release_jars.csv` still lists `mail-1.5.0-b01.jar`, so `:src:dist:verifyReleaseDependencies` fails in a release build. For a `-SNAPSHOT` version the task only logs the difference, so CI stays green: ```text + 659031 javax.mail-1.6.2.jar - 519087 mail-1.5.0-b01.jar ``` Please regenerate the file with `./gradlew :src:dist:verifyReleaseDependencies -PupdateExpectedJars`. `com.sun.mail:javax.mail:1.6.2` (2018) is also the last release under these coordinates, so Renovate will never propose an update. The same `javax.mail` package continues as `com.sun.mail:jakarta.mail` 1.6.x (latest 1.6.8). That artifact depends on `com.sun.activation:jakarta.activation`, which ships the `javax.activation` classes JMeter already gets from `com.sun.activation:javax.activation:1.2.0`, so it needs an exclude and a Renovate rule that keeps it on 1.6.x. Please consider it, or explain in the description why 1.6.2 is preferred. ########## xdocs/changes.xml: ########## @@ -98,6 +98,7 @@ Summary <li>Update json-path to 2.10.0 for JSON query expressions.</li> <li>Update Neo4j Java driver to 6.x for Bolt-based database tests.</li> <li>Update Rhino JavaScript engine to 1.8.0 for JSR-223 JavaScript execution.</li> + <li>Update <code>javax.mail</code> to 1.6.2 from 1.5.0-b01, so mail attachments with non-ASCII file names are encoded correctly.</li> Review Comment: This repeats the Bug fixes entry. Please reduce this line to the version bump (`Update javax.mail to 1.6.2 from 1.5.0-b01`) and use this section for the changes users can observe: the jar in `lib/` becomes `javax.mail-1.6.2.jar`, the Maven coordinates become `com.sun.mail:javax.mail`, and the domain in the generated `Message-ID` becomes the canonical host name. ########## src/protocol/mail/src/test/java/org/apache/jmeter/protocol/smtp/sampler/protocol/SendMailCommandTest.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jmeter.protocol.smtp.sampler.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collections; + +import javax.mail.Message; +import javax.mail.internet.InternetAddress; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class SendMailCommandTest { + + private static final String NON_ASCII_FILE_NAME = "\u0442\u0435\u043a\u0441\u0442.txt"; // Russian for "text" + + @TempDir + File tempDir; + + @Test + void testNonAsciiAttachmentFileNameIsEncodedPerRfc2231() throws Exception { + SendMailCommand sendMailCommand = createSendMailCommandWithAttachment(NON_ASCII_FILE_NAME); + String rawMessage = writeMessageToString(sendMailCommand.prepareMessage()); + // The file name must be encoded according to RFC 2231, so that the + // recipients see the original file name (see issue #6652) + assertTrue( + rawMessage.contains("filename*=UTF-8''%D1%82%D0%B5%D0%BA%D1%81%D1%82.txt"), + "filename* parameter with RFC 2231 encoded file name expected in:\n" + rawMessage); + // The mangled name must not appear anywhere + assertFalse(rawMessage.contains("B5:AB"), "mangled file name found in:\n" + rawMessage); + } + + @Test + void testAsciiAttachmentFileNameIsNotEncoded() throws Exception { + SendMailCommand sendMailCommand = createSendMailCommandWithAttachment("attachment.txt"); + String rawMessage = writeMessageToString(sendMailCommand.prepareMessage()); + assertTrue( + rawMessage.contains("filename=attachment.txt"), + "plain ASCII file name expected in:\n" + rawMessage); + assertFalse(rawMessage.contains("filename*="), "unexpected RFC 2231 encoding in:\n" + rawMessage); + } + + private SendMailCommand createSendMailCommandWithAttachment(String attachmentName) throws Exception { + File attachment = new File(tempDir, attachmentName); + Files.writeString(attachment.toPath(), "attachment content", StandardCharsets.UTF_8); + + SendMailCommand sendMailCommand = new SendMailCommand(); + sendMailCommand.setSmtpServer("localhost"); + sendMailCommand.setSmtpPort("25"); + sendMailCommand.setConnectionTimeOut("1000"); + sendMailCommand.setTimeOut("1000"); + sendMailCommand.setSender("[email protected]"); + sendMailCommand.setReceiverTo(Collections.singletonList(new InternetAddress("[email protected]"))); + sendMailCommand.setSubject("attachment file name test"); + sendMailCommand.setMailBody("body"); + sendMailCommand.addAttachment(attachment); + return sendMailCommand; + } + + private static String writeMessageToString(Message message) throws Exception { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + try (outputStream) { Review Comment: `ByteArrayOutputStream.close()` has no effect, so the `try` block can go; a plain `message.writeTo(outputStream);` is enough. Returning the bytes instead of a `String` would also let the round-trip check above parse them directly. ########## xdocs/changes.xml: ########## @@ -109,6 +110,7 @@ Summary <ch_section>Bug fixes</ch_section> <h3>General</h3> <ul> + <li><issue>6652</issue>SMTP Sampler encoded non-ASCII attachment file names incorrectly. Attachment file names are now encoded according to RFC 2231 by upgrading <code>javax.mail</code> to 1.6.2.</li> Review Comment: Please move this entry to an `Other samplers` section, the heading earlier releases use for non-HTTP samplers, and add `<pr>6757</pr>` and the contributor credit. The entry should state what users get rather than how it was done, for example: ```xml <li><issue>6652</issue><pr>6757</pr>Encode non-ASCII attachment file names in SMTP Sampler according to RFC 2231, so recipients see the original name. Contributed by Ashraf Ali (github.com/ashrafiucse)</li> ``` -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
