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

Treat Kotlin nullable as non-required #1060

Closed
wants to merge 2 commits into from

Conversation

rocketraman
Copy link
Contributor

Modify the core MethodParameter class to return the Kotlin nullability
of a parameteter (cached for performance).

Where isOptional is used, also check for isKotlinNullable i.e.
values are not considered required if they are Kotlin nullables:

  • spring-messaging: named value method arguments
  • spring-web: named value method arguments
  • spring-webmvc: request parts

This means that Kotlin client code no longer has to explicity specify
"required=false" for Kotlin nullables -- this information is inferred
automatically by the framework.

Issue: SPR-14165

@rocketraman
Copy link
Contributor Author

Notes on submission / pending work:

  • I put the kotlin-specific code into a separate module spring-core-kotlin to avoid adding the kotlin compiler to the spring-core module (inspired from spring-beans-groovy).
  • I wasn't sure where to add tests for this. My first though was MethodParameterTests, but currently it contains very little in the way of tests. Guidance is appreciated.

Otherwise the implementation turned out to be quite simple and concise.

@sdeleuze sdeleuze self-assigned this May 16, 2016
@sdeleuze
Copy link
Contributor

Thanks for this PR, I only had a quick look but I would be in favor of not creating a new spring-core-kotlin, instead you could add kotlin dependency as an optional one. After that I will make a detailed review of your PR.

@rocketraman rocketraman force-pushed the SPR-14165 branch 2 times, most recently from 1ad77c6 to 8403735 Compare May 16, 2016 14:36
@rocketraman
Copy link
Contributor Author

Thanks for this PR, I only had a quick look but I would be in favor of not creating a new spring-core-kotlin, instead you could add kotlin dependency as an optional one. After that I will make a detailed review of your PR.

Done!

@rocketraman
Copy link
Contributor Author

NOTE: Another place we could use this functionality is when @Inject or @Autowired is used, the Kotlin nullable information could be used to determine if the value is required or not.

@rocketraman
Copy link
Contributor Author

@sdeleuze Ping. Looks like it needs another rebase, but would appreciate a review.

/**
* Return whether this method parameter is declared as a Kotlin
* nullable.
* @since 4.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's target 5.0 for this feature

@sdeleuze
Copy link
Contributor

sdeleuze commented Aug 8, 2016

Sorry for the delay and thanks for your updated PR. I have added comments on the relevant lines of your commit.

Could you also rebase that on top of the new master (Spring Framework 5.0) and add some Kotlin unit tests to check that works?

Good idea to support that also for @Inject, feel free to add that to your PR and update the title of this JIRA issue to make is less specific.

@rocketraman
Copy link
Contributor Author

Good idea to support that also for @Inject, feel free to add that to your PR and update the title of this JIRA issue to make is less specific.

It looks more complicated to do this [1], so I'd prefer to have this pull request reviewed as-is for now, just to keep things as simple as possible. We can add the @Inject functionality in a subsequent pull.

Sorry for the delay and thanks for your updated PR. I have added comments on the relevant lines of your commit.

Could you also rebase that on top of the new master (Spring Framework 5.0) and add some Kotlin unit tests to check that works?

I have updated the pull with all of your comments taken into account. Thanks!

[1] It looks like a variation of RequiredAnnotationBeanPostProcessor is needed.

@rocketraman
Copy link
Contributor Author

Ping @sdeleuze for review.

Modify the core MethodParameter class to return the Kotlin nullability
of a parameteter (cached for performance). The Kotlin-specific
reflection code is within a new KotlinUtils object. Calls
to this code are gated by the presence of Kotlin in the classpath,
which is checked once at initialization time.

Issue: SPR-14165
Where `isOptional` is used, also check for `isKotlinNullable` i.e.
values are not considered required if they are Kotlin nullables:

- spring-messaging: named value method arguments
- spring-web: named value method arguments
- spring-webmvc: request parts

This means that Kotlin client code no longer has to explicity specify
"required=false" for Kotlin nullables -- this information is inferred
automatically by the framework.

At this time, this introspection does not apply to bean injection --
only to web and messaging method arguments.

Issue: SPR-14165
@sdeleuze
Copy link
Contributor

Superseeded by #1242

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

Successfully merging this pull request may close these issues.

None yet

2 participants