-
Notifications
You must be signed in to change notification settings - Fork 175
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
In 2.15, fixed to not be able to use Kotlin 1.4 or lower. #646
Conversation
Support for Kotlin 1.4 and below has been removed.
@@ -54,6 +54,13 @@ class KotlinModule @Deprecated( | |||
val singletonSupport: SingletonSupport = DISABLED, | |||
val strictNullChecks: Boolean = false | |||
) : SimpleModule(KotlinModule::class.java.name, PackageVersion.VERSION) { | |||
init { | |||
if (!KotlinVersion.CURRENT.isAtLeast(1, 5)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been confirmed to work with the following line
https://github.com/FasterXML/jackson-module-kotlin/pull/646/files#diff-938d5c566367fbca40b3ef96c99967dba4cd563f3854370272e0ae6b5d0f7f0dL68
init { | ||
if (!KotlinVersion.CURRENT.isAtLeast(1, 5)) { | ||
// Kotlin 1.4 was deprecated when this process was introduced(jackson-module-kotlin 2.15). | ||
throw IllegalStateException("jackson-module-kotlin requires Kotlin 1.5 or higher.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cowtowncoder
If there is better error message and exception
, I would like to hear them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@k163377 I think IllegalStateException
sounds like a reasonable exception to use.
@pjfanning What exception does Scala module use for its checks (wrt compatible jackson-databind
)? Might be useful to compare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scala module uses JsonMappingException for this with a message like:
Scala module 2.14.1 requires Jackson Databind version >= 2.14.0 and < 2.15.0 - Found jackson-databind version 2.13.2-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would certainly prefer that a checked exception is used as opposed to a RuntimeException like IllegalStateException. Security 'researchers' will eventually be on the case complaining about the use of RuntimeExceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I have bit mixed feelings here. Sec research will not find this unless their systems are misconfigured -- this doesn't depend on generated input but dependencies of projects -- but yeah app developers might prefer checked exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjfanning @cowtowncoder
Thanks for the review, I pushed for the change.
2a09127
@@ -65,40 +65,34 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon | |||
// Find a serializer to handle the case where the getter returns an unboxed value from the value class. | |||
override fun findSerializer(am: Annotated): StdSerializer<*>? = when (am) { | |||
is AnnotatedMethod -> { | |||
when (KotlinVersion.CURRENT.isAtLeast(1, 5)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed as it is no longer needed once support for Kotlin 1.4
and below is removed.
SSIA
see #643 (comment)