Suspend handler methods fail on nullable value class parameters

[efemoney]

Affects: Spring Framework 6.1.4, Spring Boot 3.2.3

---

This is related to support added in #27345 for value classes. This is because it tries to box an already boxed value class.

---

Take for example this handler method:

@JvmInline value class SomeId(val int: Int)

@GetMapping("/path")
suspend fun handle(
  @RequestParam nonNull: SomeId,
  @RequestParam nullable: SomeId?,
)

Calling /path?nonNull=1&nullable=2 will fail because the type of the nullable method parameter is SomeId? and Spring cannot find a Converter from String -> SomeId.

If we instead add a custom converter from String -> SomeId, the same endpoint will still fail because now the resolved argument has type SomeId but invokeSuspendingFunction wants to box it because its a value class.

My current workaround is to override invokeSuspendingFunction with my own variant that checks the arg[index] type first before boxing.

---

Adding excerpt from my own code comments:

Because of the way value classes work on the jvm, the runtime java reflection types of both parameters,
and hence the type that the spring handler method arguments have are Class<Int> & Class<SomeId> respectively.
These are the types used to [resolve arguments][HandlerMethodArgumentResolver.resolveArgument] and
more importantly, to convert from request param, request header etc.

(In our case, since value classes are not handled by default, we configure a special [GenericConverter]
that will convert into a value class type from request params, headers etc.)

However, run time kotlin reflection types for both parameters are KClass<SomeId> && KClass<SomeId?>
and these are the types used in [CoroutinesUtils.invokeSuspendingFunction] to determine whether
to box the converted argument or not.

This argument could be of wrapped type or value class type
depending on whether the declaration is nullable or not and boxing will fail on the latter!

This method is a kotlin port of [CoroutinesUtils.invokeSuspendingFunction] that checks
if the instance type is the value class just before deciding whether to box or not

[sdeleuze]

@efemoney Could you please test 6.1.5-SNAPSHOT when this build will be finished? I would be interested by your confirmation that this change and #32324 one are working as expected for your use cases before the release.

[efemoney]

Very clever to change the conversion type for (inline) value classes to the wrapped type, I didn't think of that!

Tested this and it works for the majority of cases but it fails for one case:

nullable value class without a default value supplied

@JvmInline value class SomeId(val id: Int)

suspend fun something(
  @RequestParam nullable: SomeId?,
)

This is because, according to kotlin, parameter is not optional (which triggers value constructor) but the query param can be left out because nullable type so its trying to call constructor with null value.

I believe we might need another check here of parameter.getType().isMarkedNullable() && arg[index] == null.

---

Here are my test cases & results

I use @WebMvcTest to test here the entire pipeline from params conversion to suspend function invocation.

@WebMvcTest(TestController::class)
class SuspendHandlerTests(@Autowired private val mockMvc: MockMvc) {

  @Test
  fun `test both params optional, with no values passed`() {
    mockMvc.get("/both-optional")
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("1 - null") }
      }
  }

  @Test
  fun `test both params optional, with only nullable value passed`() {
    mockMvc.get("/both-optional") { param("nullable", "2") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("1 - 2") }
      }
  }

  @Test
  fun `test both params optional, with both values passed`() {
    mockMvc.get("/both-optional") { param("nonNull", "3"); param("nullable", "2") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - 2") }
      }
  }

  @Test
  fun `test both params optional, nullable default, with no values passed`() {
    mockMvc.get("/both-optional-nullable-default")
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("1 - 2") }
      }
  }

  @Test
  fun `test both params optional, nullable default, with only nullable value passed`() {
    mockMvc.get("/both-optional-nullable-default") { param("nullable", "3") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("1 - 3") }
      }
  }

  @Test
  fun `test both params optional, nullable default, with both values passed`() {
    mockMvc.get("/both-optional-nullable-default") { param("nonNull", "3"); param("nullable", "4") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - 4") }
      }
  }

  @Test
  fun `test one param optional one required, with required value passed`() {
    mockMvc.get("/one-required-one-optional") { param("nonNull", "3") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - null") }
      }
  }

  @Test
  fun `test one param optional one required, with both values passed`() {
    mockMvc.get("/one-required-one-optional") { param("nonNull", "3"); param("nullable", "4") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - 4") }
      }
  }

  @Test
  fun `test one param optional one required, nullable default, with required value passed`() {
    mockMvc.get("/one-required-one-optional-nullable-default") { param("nonNull", "3") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - 2") }
      }
  }

  @Test
  fun `test one param optional one required, nullable default, with both values passed`() {
    mockMvc.get("/one-required-one-optional-nullable-default") { param("nonNull", "3"); param("nullable", "4") }
      .asyncDispatch()
      .andExpect {
        status { isOk() }
        content { string("3 - 4") }
      }
  }
}

@RestController
private class TestController {
  // "optional" in this context means that the url query parameter is optional
  //  not necessarily that the kotlin parameter is optional 

  @GetMapping("/both-optional")
  suspend fun bothOptional(
    @RequestParam nonNull: SomeId = SomeId(1),
    @RequestParam nullable: SomeId?,
  ): String {
    return nonNull.id.toString() + " - " + nullable?.id.toString()
  }

  @GetMapping("/both-optional-nullable-default")
  suspend fun bothOptionalDefault(
    @RequestParam nonNull: SomeId = SomeId(1),
    @RequestParam nullable: SomeId? = SomeId(2),
  ): String {
    return nonNull.id.toString() + " - " + nullable?.id.toString()
  }

  @GetMapping("/one-required-one-optional")
  suspend fun oneRequiredOneOptional(
    @RequestParam nonNull: SomeId,
    @RequestParam nullable: SomeId?,
  ): String {
    return nonNull.id.toString() + " - " + nullable?.id.toString()
  }

  @GetMapping("/one-required-one-optional-nullable-default")
  suspend fun oneRequiredOneOptionalDefault(
    @RequestParam nonNull: SomeId,
    @RequestParam nullable: SomeId? = SomeId(2),
  ): String {
    return nonNull.id.toString() + " - " + nullable?.id.toString()
  }
}

// Needs to be public or else will throw access restrictions exceptions
@JvmInline value class SomeId(val id: Int)

[sdeleuze]

@efemoney Please test again when this build will be finished. Thanks for your feedback, much appreciated.

[efemoney]

Tested and all my tests pass without any workarounds!