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

Add support for Kotlin functions #1666

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sdeleuze
Copy link
Contributor

@sdeleuze sdeleuze commented Nov 4, 2024

This PR adds:

  • Support for Kotlin functions
  • A Kotlin extension to FunctionCallbackWrapper.Builder
  • Kotlin documentation for language specifics

It also removes the dependency on Spring Cloud Function and net.jodah:typetools which are replaced by leveraging
Spring Framework RootBeanDefinition and ResolvableType capabilities.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Nov 5, 2024

@markpollack Forced-pushed to remove a RootBeanDefinition cast which was not needed and rebased to fix a merge-conflict.

@tzolov tzolov self-assigned this Nov 5, 2024
@markpollack
Copy link
Member

merged in 9cf6633

@markpollack markpollack closed this Nov 5, 2024
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Nov 6, 2024

Reopening the PR after finding a regression and reverting related commit from main.

@sdeleuze sdeleuze reopened this Nov 6, 2024
This commit adds support for proper Kotlin functions handling
by adapting kotlin.jvm.functions.Function1 to
java.util.function.Function and
kotlin.jvm.functions.Function2 to
java.util.function.BiFunction.

It also removes the dependency on Spring Cloud Function and
net.jodah:typetools which are replaced by leveraging
Spring Framework ResolvableType capabilities.
This commit adds a Kotlin extension function for
FunctionCallbackWrapper.Builder.withInputType allowing
to specify withInputType<T>() instead of
withInputType(T::class.java).
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Nov 7, 2024

I spent some time debugging the initial version of this PR that we had to revert after a regression detailed in #1685 and reproducible with @ThomasVitale repro.

The reason why our tests did not catch this regression is not straightforward. In our tests, ConfigurationClassParser is creating AnnotationMetadata via reflection (StandardAnnotationMetadata) and with that, beanDefinition.getResolvableType() works as expected because the function type is resolved via RootBeanDefinition#factoryMethodToIntrospect but in the application that reproduces the issue, the AnnotationMetadata is a SimpleAnnotationMetadata created via ASM where beanDefinition.getResolvableType() is unable to resolve the type.

Generally speaking, BeanDefinition#getResolvableType() is a sort of best-effort representation of the currently known time information at runtime, so I have added a fallback resolution using the factory method, duplicating (for now) ConstructorResolver#resolveFactoryMethodIfPossible logic which is not exposed in the public API. We may expose that in a future version of Spring Framework.

I have added related tests using tests that involve scanning to trigger the ASM code path. Both tests and @ThomasVitale repro are green with that new version I forced pushed, and Open AI IT tests seems to be fine as well.

There still may be some small differences with original SCF algorithm, but I guess we could refine with proper test if something is not supported yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants