From eb1a5d3dc866a8c031cc0aa4694d82032cb0a74d Mon Sep 17 00:00:00 2001 From: Steve Gerhardt Date: Mon, 12 Sep 2022 19:03:09 -0600 Subject: [PATCH] Fix SimpleMailMessage nullability annotations Fixes the nullability annotations in `SimpleMailMessage` so the `set*(...)` methods have their parameters marked as `@Nullable`. These method parameters had no annotations and when `SimpleMailMessage` is used from Kotlin with `-Xjsr305=strict` enabled, the `set*(...)` methods were treated as being marked `@NonNull`. This also meant Kotlin would not see matched pairs of get/set methods as `var` properties, so calling `message.text = "Text"` would not compile in Kotlin. Adds tests in `SimpleMailMessageJsr305ComplianceTests.kt` that verify the nullability annotations for `SimpleMailMessage` are correct. This file will actually fail to compile if the nullability annotations are missing, causing the tests to fail. Also modifies `spring-context-support.gradle` to force the Kotlin test code to compile with `-Xjsr305=strict`, enabling the test code to work properly. --- .../spring-context-support.gradle | 11 ++ .../mail/SimpleMailMessage.java | 24 ++-- .../SimpleMailMessageJsr305ComplianceTests.kt | 135 ++++++++++++++++++ 3 files changed, 160 insertions(+), 10 deletions(-) create mode 100644 spring-context-support/src/test/kotlin/org/springframework/mail/SimpleMailMessageJsr305ComplianceTests.kt diff --git a/spring-context-support/spring-context-support.gradle b/spring-context-support/spring-context-support.gradle index 160f8028985c..157b2da3662b 100644 --- a/spring-context-support/spring-context-support.gradle +++ b/spring-context-support/spring-context-support.gradle @@ -1,5 +1,7 @@ description = "Spring Context Support" +apply plugin: "kotlin" + dependencies { api(project(":spring-beans")) api(project(":spring-context")) @@ -12,6 +14,8 @@ dependencies { optional("com.github.ben-manes.caffeine:caffeine") optional("org.quartz-scheduler:quartz") optional("org.freemarker:freemarker") + optional("org.jetbrains.kotlin:kotlin-reflect") + optional("org.jetbrains.kotlin:kotlin-stdlib") testImplementation(project(":spring-context")) testImplementation(testFixtures(project(":spring-beans"))) testImplementation(testFixtures(project(":spring-context"))) @@ -27,3 +31,10 @@ dependencies { testFixturesImplementation("org.assertj:assertj-core") testFixturesImplementation("org.mockito:mockito-core") } + +// Compile the Kotlin test code differently from the rest - it needs +// -Xjsr305=strict to ensure that parameter and return types are +// correctly annotated from Kotlin's perspective. +compileTestKotlin { + kotlinOptions.freeCompilerArgs += "-Xjsr305=strict" +} diff --git a/spring-context-support/src/main/java/org/springframework/mail/SimpleMailMessage.java b/spring-context-support/src/main/java/org/springframework/mail/SimpleMailMessage.java index 4e8295f70d31..3642e322ac51 100644 --- a/spring-context-support/src/main/java/org/springframework/mail/SimpleMailMessage.java +++ b/spring-context-support/src/main/java/org/springframework/mail/SimpleMailMessage.java @@ -93,7 +93,7 @@ public SimpleMailMessage(SimpleMailMessage original) { @Override - public void setFrom(String from) { + public void setFrom(@Nullable String from) { this.from = from; } @@ -103,7 +103,7 @@ public String getFrom() { } @Override - public void setReplyTo(String replyTo) { + public void setReplyTo(@Nullable String replyTo) { this.replyTo = replyTo; } @@ -113,7 +113,7 @@ public String getReplyTo() { } @Override - public void setTo(String to) { + public void setTo(@Nullable String to) { this.to = new String[] {to}; } @@ -128,12 +128,12 @@ public String[] getTo() { } @Override - public void setCc(String cc) { + public void setCc(@Nullable String cc) { this.cc = new String[] {cc}; } @Override - public void setCc(String... cc) { + public void setCc(@Nullable String... cc) { this.cc = cc; } @@ -143,12 +143,12 @@ public String[] getCc() { } @Override - public void setBcc(String bcc) { + public void setBcc(@Nullable String bcc) { this.bcc = new String[] {bcc}; } @Override - public void setBcc(String... bcc) { + public void setBcc(@Nullable String... bcc) { this.bcc = bcc; } @@ -158,7 +158,7 @@ public String[] getBcc() { } @Override - public void setSentDate(Date sentDate) { + public void setSentDate(@Nullable Date sentDate) { this.sentDate = sentDate; } @@ -168,7 +168,7 @@ public Date getSentDate() { } @Override - public void setSubject(String subject) { + public void setSubject(@Nullable String subject) { this.subject = subject; } @@ -178,7 +178,7 @@ public String getSubject() { } @Override - public void setText(String text) { + public void setText(@Nullable String text) { this.text = text; } @@ -278,4 +278,8 @@ private static String[] copy(String[] state) { return state.clone(); } + public void testFunction(String parameter) { + System.out.println(parameter); + } + } diff --git a/spring-context-support/src/test/kotlin/org/springframework/mail/SimpleMailMessageJsr305ComplianceTests.kt b/spring-context-support/src/test/kotlin/org/springframework/mail/SimpleMailMessageJsr305ComplianceTests.kt new file mode 100644 index 000000000000..f438a360c566 --- /dev/null +++ b/spring-context-support/src/test/kotlin/org/springframework/mail/SimpleMailMessageJsr305ComplianceTests.kt @@ -0,0 +1,135 @@ +package org.springframework.mail/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed 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 + * + * https://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. + */ + +import org.junit.jupiter.api.Test + +import java.util.* +import kotlin.reflect.KParameter +import kotlin.reflect.full.declaredMemberFunctions + +/** + * These tests are intended to verify correct behavior of SimpleMailMessage when used from + * Kotlin with `-Xjsr305=strict` enabled to strictly enforce Java nullability annotations. + * + * Kotlin's JSR305 strict mode treats all non-annotated parameters and return types as + * non-nullable, while Java code does not perform any such checks and can still pass and + * capture null parameters and return values, respectively. + * + * Kotlin also treats null and definitely-non-null types as incompatible types. As such, + * The get/set methods in [SimpleMailMessage] must therefore have coherent nullability + * annotations for Kotlin to treat them as properties with getters and setters. Otherwise, + * Kotlin sees them as functions with incompatible get/set types and does not associate + * them. + * + * These errors often appear only at compile time and not in the IDE, making it somewhat + * confusing to troubleshoot. + * + * __If any of the annotations on [SimpleMailMessage] are missing or incoherent with their + * respective getter / setter method, this test will actually fail to compile, which is + * a form of test failure in and of itself.__ + * + * @author Steve Gerhardt + */ +class SimpleMailMessageJsr305ComplianceTests { + + private inline fun T.getUnsafeVarargSetterByReflection(name : String) : Function1?, Unit> { + return { arrayForVararg -> + this::class.declaredMemberFunctions + .first { + val isFirstValueParameterVararg = it.parameters + .firstOrNull { p -> p.kind == KParameter.Kind.VALUE } + ?.isVararg == true + + it.name == name && isFirstValueParameterVararg + } + .call(this, arrayForVararg) + } + } + + @Test + fun `Null message parameters via Java setters should be null via Kotlin getters`() { + val message = SimpleMailMessage() + message.setFrom(null) + message.getUnsafeVarargSetterByReflection("setTo")(null) + message.setReplyTo(null) + message.getUnsafeVarargSetterByReflection("setCc")(null) + message.getUnsafeVarargSetterByReflection("setBcc")(null) + message.setSentDate(null) + message.setSubject(null) + message.setText(null) + + assert(message.from == null) + assert(message.to == null) + assert(message.replyTo == null) + assert(message.cc == null) + assert(message.bcc == null) + assert(message.sentDate == null) + assert(message.subject == null) + assert(message.text == null) + } + + @Test + fun `To, CC, and BCC with lists of null recipients should appear as valid lists with null entries via Kotlin getters`() { + val message = SimpleMailMessage() + message.setTo(*arrayOf(null, null, null, null)) + message.setCc(*arrayOf(null, null, null)) + message.setBcc(*arrayOf(null, null)) + + assert(message.to.let { it != null && it.size == 4 && it.all { item -> item == null } }) + assert(message.cc.let { it != null && it.size == 3 && it.all { item -> item == null } }) + assert(message.bcc.let { it != null && it.size == 2 && it.all { item -> item == null } }) + } + + @Test + fun `Non-null message parameters via Java setters should be non-null via Kotlin getters`() { + val message = SimpleMailMessage() + message.setFrom("me@mail.org") + message.setTo("you@mail.org") + message.setReplyTo("reply@mail.org") + message.setCc(*arrayOf("he@mail.org", "she@mail.org")) + message.setBcc(*arrayOf("us@mail.org", "them@mail.org")) + val sentDate = Date() + message.setSentDate(sentDate) + message.setSubject("my subject") + message.setText("my text") + + assert(message.from == "me@mail.org") + assert(message.to.let { it != null && it.contains("you@mail.org") }) + assert(message.replyTo == "reply@mail.org") + assert(message.cc.let { it != null && it.toList() == listOf("he@mail.org", "she@mail.org") }) + assert(message.bcc.let { it != null && it.toList() == listOf("us@mail.org", "them@mail.org") }) + assert(message.sentDate == sentDate) + assert(message.subject == "my subject") + assert(message.text == "my text") + } + + @Test + fun `Message parameters can be set via Kotlin setters`() { + val message = SimpleMailMessage() + message.from = "me@mail.org" + message.replyTo = "reply@mail.org" + val sentDate = Date() + message.sentDate = sentDate + message.subject = "my subject" + message.text = "my text" + + assert(message.from == "me@mail.org") + assert(message.replyTo == "reply@mail.org") + assert(message.sentDate == sentDate) + assert(message.subject == "my subject") + assert(message.text == "my text") + } +}