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

Improve Kotlin support #5537

Closed
philwebb opened this issue Mar 30, 2016 · 42 comments
Closed

Improve Kotlin support #5537

philwebb opened this issue Mar 30, 2016 · 42 comments

Comments

@philwebb
Copy link
Member

Placeholder issue for things we can do to improve Kotlin support

@value

Using @Value("${thing}") is a pain because you need to escape $ (e.g. @Value("\${thing}"). Perhaps a different key like @Value("#{thing}").

Open Classes

Open classes are a pain, can we use a classloader to remove final.
Also all @Bean methods

perhaps we can use a custom bean classloader

Default constructor arguments with @Autowired

Give a constructor with a default param:

open class Foo @Autowired constructor(val restTemplate: RestOperation = NoOpRestOperation());

Kotlin will create multiple constructors, this makes Spring fall over because there's multiple annotated constructors. A method to be smart about which to pick would be nice.

Main method

@JvmStatic is needed for main methods. Check if we can find them automatically. Also the name of the class is a bit mad ThePackageKt so perhaps we can replace it with something nice.

Add Kotlin Module

Look at Jackson for an example.

@philwebb philwebb added the type: enhancement A general enhancement label Mar 30, 2016
@wilkinsona
Copy link
Member

Gradle plugin

Our plugin applies the Java plugin automatically. That doesn't make sense when you're building a pure Kotlin project and leads to unnecessary noise as the Java-related tasks are still run as part of the build:

$ ./gradlew classes
:compileKotlin
:compileJava UP-TO-DATE
:processResources
:classes

@philwebb
Copy link
Member Author

/cc @jkschneider

@philwebb
Copy link
Member Author

If looks like @Value might already support #{...} expressions as an alternative to ${...}.

@jkschneider
Copy link
Contributor

Spring MVC

Reified extension functions for the RestOperations interface. There are probably similar better-typing gains to be made elsewhere.

@jkschneider
Copy link
Contributor

Spring HATEOAS

Extension functions to improve the transformation of objects and collections of objects into Resource and PagedResources, respectively. Also better link generation for Resource and PagedResources.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 2, 2016

Thanks for creating this issue @philwebb 👍, I am especially interested by removing the need for the open qualifier.

Maybe we could discuss Monday with the team if they would be ok to automatically register Kotlin Jackson module in Jackson2ObjectMapperBuilder if it is in the classpath like we do for various other modules (I could add this to Spring Framework 4.3 easily).

@jkschneider Good idea about Spring HATEOAS. Be aware that I also plan to experiment on allowing to specify relations thanks to annotations + regular data classes, see spring-projects/spring-hateoas#401 for more details. Nothing specific to Kotlin, but that will make it easier to deal with data classes in both Kotlin and Java.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 4, 2016

@philwebb SPR-14108 + related pull request for Jackson Kotlin module automatic registration submitted for inclusion in Spring Framework 4.3.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 4, 2016

I have just merged Jackson Kotlin module automatic registration, so it will be available in 4.3.0.RC1 upcoming release tomorrow and then in Spring Boot 1.4.

About @Value I did a quick test some times ago, and I dont remember # was supported out of the box as a dropin replacement for $, I think only @Value("#{config['foo']}) and @Value("\${foo}") were. That's why I described in this StackOverflow answer how to customize ${...} to something else. Is there a better solution available out of the box?

About extensions, Kotlin Primavera contains some examples of Kotlin extensions applied to Spring (I am sure there a lot of others to imagine even more useful).

I have tried to reproduce the "Default constructor arguments with @Autowired" described above in this test project but did not succeed. Any chance someone send a PR on this test project to show how to reproduce this issue?

You can find bellow some issues from Kotlin bugtracker related to Spring:

@jkschneider
Copy link
Contributor

@lkogler - Hadi Hariri mentioned you as a Spek contributor. Thought you might be interested in this effort as well.

@jkschneider
Copy link
Contributor

@sdeleuze Sent a PR for "Default constructor arguments with @Autowired". Adding required=false was a valid workaround as of some pre-1.0 release, but no longer works.

Thanks for putting together all of the links!

@philwebb or @sdeleuze if you could create a spring-kotlin project in spring-projects and give me commit access, I'll start pouring in the work I have here.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 4, 2016

@jkschneider As a first step I have created the spring-kotlin in my own sdeleuze account. Maybe we could both of us (and any interested people) contribute anything useful for building Spring + Kotlin applications, including documentation. When the project will be mature enough, we will evaluate moving it to https://github.com/spring-projects.

I already gave you the push rights, please sign the CLA as explained in the README. I will setup the build process using http://build.spring.io and publish the snapshots in https://repo.spring.io/snapshot/ as soon as we have the first bits committed.

Does it sound ok for you?

@spencergibb
Copy link
Member

@sdeleuze He's signed the CLA for spring cloud previously.

@jkschneider
Copy link
Contributor

@sdeleuze thanks, sounds like a plan!

@nfrankel
Copy link

nfrankel commented Apr 4, 2016

I'm interested as well, and have just signed (again?) the CLA - just to be sure.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 4, 2016

Nice :-)

@jkschneider
Copy link
Contributor

@sdeleuze Regarding the default finality problem, what do you think about a Lombok-style annotation-processor hack to strip final from the bytecode at annotation-processing time? It isn't ideal in the sense that it requires a plugin (Eclipse) or non-default setting (IntelliJ), but...

Otherwise, I'm not sure where we would put a custom ClassLoader such that all entry points would be covered (main, servlet initialization, test, etc.).

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 5, 2016

Honestly I really would like to avoid using Lombok-style annotation-processor hack, since IMO one of the Kotlin advantages is to avoid this kind of non standard code that requires special support in IDEs and build tools.

I would rather try to experiment with custom bean classloaders, reflection and eventually JDK dynamic proxies to see if we can do something here. Only Spring need to extend these classes for technical reasons, the end user code don't have to, so even if I am not a reflection guru, I think it is worth to try that way.

@cemo
Copy link
Contributor

cemo commented Apr 5, 2016

@sdeleuze
👍 for

avoid using Lombok-style annotation-processor hack

@wilkinsona
Copy link
Member

Thinking aloud: I wonder if JetBrains would consider an enhancement to Kotlin that allows an annotation to make a class open, perhaps via a meta-annotation?

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 5, 2016

@wilkinsona The question has been forwarded to the Kotlin team at JetBrains, I would be in favor of such solution.

@mikegehard
Copy link
Contributor

Thank you all in advance for pushing this stuff forward!

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 7, 2016

@jkschneider I have merged your PR in kotlin-autowired-constructor to reproduce the issue you described , and indeed I got an error, but an @Autowired annotation requires at least one argument one likely because Kotlin generates the default constructor annotated with @Autowired, and Spring has nothing to autowire on this one so it raises this error.

That said, I am not sure this should be seen as a bug. @Autowired purpose it to instantiate constructor parameters, and there is a conflict with Kotlin default constructor arguments that try to do the same. I tend to think that Kotlin default arguments should not be used on @Autowired annotated constructors, mixing both is not something I would recommend anyway.

For now, I think I would be in favor to just add some documentation about that behavior (in the additional tips section of my Kotlin + Spring Boot blog post for example).

Any thoughts?

@sdeleuze
Copy link
Contributor

See SPR-14165 about supporting Kotlin nullable information in Spring MVC.

@sdeleuze
Copy link
Contributor

I have created an issue on Kotlin bugtracker to find a workaround to the annoying mandatory open qualifier for @Service , @Repository , @Bean and @Configuration classes and methods. Feel free to vote for it and add your comments. https://youtrack.jetbrains.com/issue/KT-12149

@wendigo
Copy link

wendigo commented Apr 29, 2016

Is this really a problem? We've been developing Spring Boot applications on top of Kotlin for a couple of months and we've just adjusted :) Changing this behaviour right now, after 1.0.0 was released is not really a good idea.

@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 29, 2016

I think exposing such technical detail to the user is a real issue, at least for me. It gives a feeling that using Spring + Kotlin is not "natural" IMO. In any case, if we find a solution, it won't break your application, it will just make Spring + Kotlin working without open and allow you to have less verbose code if you want to.

@jkschneider
Copy link
Contributor

jkschneider commented May 2, 2016

@sdeleuze If you haven't already, you can read through the long discussion here on the Kotlin forums. Eventually, Andrey Breslav answered with a blog post on final-by-default that outlined why it is unlikely to change on their side. In spite of the great points he makes here, I think there is virtually no risk to opening classes and methods we know need to be open after an initial pass of the Kotlin compiler. In short, let the Kotlin compiler use final constraints to check smart casts, etc and then open whatever needs to be opened. Seems to be the best of both worlds.

I think reflection and JDK dynamic proxies that you mentioned earlier are out, since reflection cannot affect finality (unlike visibility) and dynamic proxies only really apply to interfaces... but perhaps there is another solution here I am not imagining?

Code generation or bytecode manipulation is going to have to happen, it's just a question of whether we get it done at compile time or runtime.

Runtime: AFAIK, the Spring bean-creating mechanism does not use a custom classloader, so there is no opportunity there. Without looking deeper, it is unclear to me whether we have a hook after bean definition but before bean initialization (e.g. BeanDefinitionRegistryPostProcessor) that would allow us to do some magic.

Compile-time: Since we have no compile-time metaprogramming facility yet, kapt was the only option. Since it only runs in Gradle and not in IntelliJ. It used to be the case that you could choose to debug/run things either with the IntelliJ platform or Gradle, but as of Idea circa 15, this only applies to test runners. I did write a more or less complete version of the annotation processor solution late last week before realizing that IntelliJ wouldn't pick it up. Shame, because the Eclipse case would have been covered by attaching a java agent to the JDT and we would have been home free.

@sdeleuze
Copy link
Contributor

sdeleuze commented May 9, 2016

As discussed on Kotlin Slack, I have made some experiments about using spring-aspect and/or AspectJ (see this branch), but it seems that this approach can't be used everywhere (especially on Spring Boot configuration) and has big drawbacks regarding IDE and build tool integrations. That's not something I want to promote as the default solution.

As an alternative, I tried to update my geospatial-messenger sample app in order to use interface + JDK dynamic proxies (see this commit) and found the result quite convincing. It removes the need to use open on @Repository and @Service beans. Spring Boot configuration still requires open at class and function level, but that's less annoying to me because this is not the core of the app functionalities, and JetBrains seems open to discuss the addition of a allopen keyword that could be added on @Configuration classes in order to avoid specifying open for each function of the class.

Given the price to pay in term of IDE and build tool configuration (the kind of things I hate with lombok) for other solutions, that seems to be a good tradeoff IMO.

For people still wanting to remove open on CGLIB proxies, be aware that someone at JetBrains made an experiment with a Kotlin compiler plugin (no IDEA, Gradle or Maven plugin available) and it seems @jkschneider made some progress in using kapt for doing the similar operation.

@jkschneider
Copy link
Contributor

I think I should have been more explicit about what an annotation processor solution would NOT require. Lombok requires manual IDE config because the code it generates needs to be picked up by the IDE for code completion for other source code. The solution here only requires the bytecodes to be modified prior to execution, so would not require similar manual config. As far as the build tool goes, one or more of the Spring Gradle plugins could add the appropriate kapt dependency when necessary.

@snicoll
Copy link
Member

snicoll commented May 10, 2016

The annotation processor is pretty transparent indeed and supported properly by the IDE but I wonder to which annotation it would be supposed to react. I haven't used kotlin much myself but it feels to me it is still a lot of setup to workaround a language decision...

@sdeleuze
Copy link
Contributor

@snicoll I think the initial idea was to open classes annotated by @Component (directly or transitively).

@jkschneider Good point, indeed bytecode modification is only required for running the app, not compiling it. But I think we should be careful with such bytecode manipulation and limit the scope where it applies.

For example, while that looks like an handy solution for simple apps, what would be the result of such bytecode modification in a multi-module application where you have your @Repository and @Service classes in another module than your application? I guess the IDE will allow you to extend these classes (since it will use the bytecode not the sources) even if that was not necessarily your intent.

Based on my experiment and your last comment, maybe a good compromise would be to enable such bytecode modification only on @Configuration classes (directly or transitively) by default. That would means that @Configuration files are open by default (since we have not way to avoid that), but let the core of your application (@Repository and @Service) final by default in compliance with Kotlin mindset, and encourage people to use interfaces + JDK dynamic proxy, which I think is a good practice. The plugin could have an option to modify the scope of the modification to allow use specifying @Component instead of @Configuration if they prefer (I would not recommend that).

Any thoughts?

@sdeleuze
Copy link
Contributor

sdeleuze commented May 17, 2016

I have created a Spring + Kotlin FAQ : https://github.com/sdeleuze/spring-kotlin#spring--kotlin-faq

@wilkinsona @philwebb Do you think we could do something to avoid the current CGLIB proxies by default behavior as soon as JDBC is used (see https://github.com/sdeleuze/spring-kotlin/blob/master/README.md#where-this-configuration-problem-configuration-class-foo-may-not-be-final-error-message-come-from)?

@jnizet
Copy link
Contributor

jnizet commented May 28, 2016

I started working with Kotlin and Spring-boot for just one week, and the biggest pain point for me, not directly related to Spring-boot, but still, is Mockito.
Many Mockito matchers (like any()) return null, and that is a problem with null-safe Kotlin arguments.
I used mockito-kotlin to try circumventing these problems, but it's based on Mockito 2.0.52-beta, which is incompatible with the version of Mockito used by Spring-boot.
As a quick workaround, I just used my own adapted version of mockito-kotlin, but that's something I would really want to avoid.

@sdeleuze
Copy link
Contributor

Good news, it seems Kotlin team is going to provide a solution to allow CGLIB proxies without having to open the class and all its functions.

@sdeleuze
Copy link
Contributor

sdeleuze commented Dec 27, 2016

The long awaited solution to the open keyword on class and functions issue is now available thanks to the new kotlin-spring plugin, see this blog post for more details. I have updated https://github.com/sdeleuze/spring-boot-kotlin-demo to use it, and it works well (be sure to have IDEA Kotlin 1.0.6+ plugin to get it working).

I will update http://start.spring.io/ shortly.

I am also in the process of integrating various Kotlin extensions directly in Spring Framework 5:

I have also a prototype of Kotlin based template rendering using Kotlin Script and Spring MVC ScriptTemplateView (JSR 223): https://github.com/sdeleuze/kotlin-script-templating.

@sdeleuze
Copy link
Contributor

You can use this link to track Kotlin related issues in the upcoming Spring Framework 5.0.

@sdeleuze
Copy link
Contributor

sdeleuze commented Jan 2, 2017

@wilkinsona @bclozel It seems Spring Boot TestRestTemplate can't leverage the Kotlin extensions I added in Spring Framework 5 RestOperationsExtension because it does not implement RestOperations. Do you think it could implement it in Spring Boot 2.0 or should I contribute a TestRestTemplateExtension?

@snicoll
Copy link
Member

snicoll commented Jan 2, 2017

We won't go back implementing RestOperations if that's what you're asking. If we want to follow along, we'll probably have a kotlin theme in 2.0 as we'll do whatever is the best option. What is your recommendation?

@sdeleuze
Copy link
Contributor

sdeleuze commented Jan 2, 2017

I think just providing a TestRestTemplateExtensions similar to what I did on RestOperationsExtensions would be fine.

@sdeleuze
Copy link
Contributor

sdeleuze commented Jan 23, 2017

Following Spring Framework 5 Kotlin support announcement, and after a few weeks developing a real Spring Boot + Kotlin reactive web application, I have tried to summarise what features Spring Boot 2.0 could provide to improve Kotlin support:

@sdeleuze
Copy link
Contributor

sdeleuze commented Feb 9, 2018

Spring Boot 2.0 now provides following Kotlin features:

The only outstanding issue missing is IMO opinion #8762 the @ConfigurationProperties binding for immutable Kotlin classes, it is likely too late now for inclusion in 2.0 but it would be a very nice feature to support in Spring Boot 2.1, I still plan to provide a PR for that. #10712 Null-safety annotation on Spring Boot APIs would also be a good fit for 2.1.

On start.spring.io side a lot of work has been done with @snicoll for Kotlin support:

  • Support for Kotlin 1.2 for Gradle and Maven projects
  • Link with Kotlin pre-selected via https://start.spring.io/#!language=kotlin
  • -Xjsr305=strict compiler flag
  • Java 8 bytecode generation
  • kotlin-reflect and kotlin-stdlib-jdk8 dependencies
  • jackson-module-kotlin dependency is added when relevant
  • kotlin-spring plugin enabled
  • Add version range support for Kotlin version

The remaining outstanding issue needed before Spring Boot 2.0 release from my POV is the lack of jackson-module-kotlin dependency for web projects (see #11133 for history of this tricky topic) which makes JSON REST webservices broken out of the box. We print messages in logs at startup and this will be mentioned in the documentation, but not including it in generated project is something that really puzzles me. After discussing with @snicoll, I think we have found a way to fix that and will try to provide an Initializr PR.

After all the work done for Spring Boot 2.0, and given the fact that missing features have their own issues, maybe we should close this general purpose issue?

@philwebb
Copy link
Member Author

philwebb commented Feb 9, 2018

Thanks for the summary @sdeleuze, I'll close this one.

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

No branches or pull requests