`HttpServiceProxyFactory` should omit optional `@RequestParam` if converted from `null` to empty string

[ftreede]

Spring version: 6.1.14

When using HttpServiceProxyFactory, we can define request parameters as optional, so that a value of null leads to the parameter being omitted completely:

@GetExchange("/test")
void testString(@RequestParam(value = "param", required = false) String param);

Here, calling testString(null) will result in a request with url /test. The query parameter is omitted completely.

However, If the value is not a string and run through conversion, this does not work.
In this example:

@GetExchange("/test")
void testDate(@RequestParam(value = "param", required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) OffsetDateTime param);

Calling testDate(null) makes a request with /test?param=, setting the query parameter to an empty string.

This behaviour is inconsistent and counterintuitive. Also, a server expecting a date value in the parameter is likely to not accept an empty string.

You can work around this by customizing the conversion service or the argument resolvers in the HttpServiceProxyFactory, but there really should be a consistent solution in spring itself.

A full test case for reference:

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.format.annotation.DateTimeFormat;
import org.springframework.http.HttpMethod;
import org.springframework.test.web.client.MockRestServiceServer;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.client.RestClient;
import org.springframework.web.client.support.RestClientAdapter;
import org.springframework.web.service.annotation.GetExchange;
import org.springframework.web.service.invoker.HttpServiceProxyFactory;

import java.time.OffsetDateTime;

import static org.springframework.test.web.client.match.MockRestRequestMatchers.*;
import static org.springframework.test.web.client.response.MockRestResponseCreators.*;

class HttpInterfaceConvertedNullTest {

    private MockRestServiceServer mockServer;
    private HttpInterface httpInterface;

    @BeforeEach
    void setUp() {
        // setup client and mock server
        RestClient.Builder rcb = RestClient.builder();
        mockServer = MockRestServiceServer.bindTo(rcb).build();
        rcb.baseUrl("http://example.com");

        // make http interface
        httpInterface = HttpServiceProxyFactory.builderFor(RestClientAdapter.create(rcb.build()))
                .build()
                .createClient(HttpInterface.class);
    }

    @Test
    void testDateNull() {
        // setup expectations - the requestTo expectation also requires there to be no query parameters
        mockServer.expect(requestTo("http://example.com/test"))
                .andExpect(method(HttpMethod.GET))
                .andRespond(withSuccess());

        // make call
        httpInterface.testDate(null);

        // verify
        mockServer.verify();
    }

    @Test
    void testStringNull() {
        // setup expectations - the requestTo expectation also requires there to be no query parameters
        mockServer.expect(requestTo("http://example.com/test"))
                .andExpect(method(HttpMethod.GET))
                .andRespond(withSuccess());

        // make call
        httpInterface.testString(null);

        // verify
        mockServer.verify();
    }


    interface HttpInterface {
        @GetExchange("/test")
        void testDate(@RequestParam(value = "param", required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) OffsetDateTime param);

        @GetExchange("/test")
        void testString(@RequestParam(value = "param", required = false) String param);
    }

}

[br1ghtt]

From what I see, the AbstractNamedValueArgumentResolver.addSingleValue uses a different converter based on the parameter type (String.class -> GenericConversionService.NoOpConverter, OffsetDateTime.class -> FormattingConversionService.PrinterConverter). Their convert method behave slightly different, when it comes to null values. NoOpsConverter returns null in case of a null source object, and on the other hand PrinterConverter returns empty string. AbstractNamedValueArgumentResolver.addSingleValue does not add the parameter in case of a not required null value. In case of empty string it adds a addRequestValue. So we could change printer converter or change addSingleValue to also not add on empty string. Does someone understand the impact when doing this change?

[ftreede]

I can definetely think of usecases where I'd want to explicitly pass an empty string as parameter.

A better approach would be to just not call the conversion service for null values. It is already getting skipped if the value is a string.

[br1ghtt]

I fully agree with you about passing empty strings as query parameter values.

With the following test I was able to reproduce the case when passing null to a not required converted request param in NamedValueArgumentResolverTests

@GetExchange
void executeDateNotRequired(@Nullable @TestValue(required = false) @DateTimeFormat(pattern = "yyyy-MM-dd") LocalDate value);

@Test
void dateTestNotRequiredNull() {
	this.service.executeDateNotRequired(null);
	assertTestValue("value");
}

And then skip conversion when value null in AbstractNamedValueArgumentResolver#addSingleValue

if (value != null && this.conversionService != null && !(value instanceof String)) {
	//
}

if (value == null) {
    Assert.isTrue(!required, () -> "Missing " + valueLabel + " value '" + name + "'");
    return;
}

If you agree with this I can submit a PR.

[rstoyanchev]

I don't think we need to prevent null values from conversion, but we can check after conversion if the value is empty and was null before. If it is not required, reset it back to null.

We have similar post conversion checks on the server side, but in the opposite direction where request values are converted to objects.