Spring-framework: Locale inconsistently resolves to null for invalid input value

[phillipuniverse]

Affects: 5.1.5.RELEASE

---

I am trying to receive a Locale into a required @RequestParam. The happy path parsing works great, but I am receiving null into my @RequestMapping when Locale parsing fails. Here's my currently-failing test (run against Spring Boot 2.1.3.RELEASE):

@WebMvcTest
public class LocaleDeserializationTest {

    @Configuration
    static class Config {

        @RestController
        static class LocaleController {

            @GetMapping("/request-param")
            public String requestParam(@RequestParam Locale locale) {
                return locale.toLanguageTag();
            }
        }
    }

    @Autowired
    MockMvc mockMvc;

    @Test
    public void badLocaleRequestParam() throws Exception {
        mockMvc.perform(get("/request-param").param("locale", "thiswillbebadrequest"))
                .andExpect(status().isBadRequest());
    }
}

This in fact throws a NullPointerException because locale is null within the execution of the requestParam() method. I also tried marking the argument as @NonNull (the Spring variant) but that also still allowed null to come in for Locale.

A bit of investigation led me to the StringToLocaleConverter, and StringUtils.parseLocale() correctly returns null. I would expect that since my @RequestParam is a _required_ parameter, this would end up returning a 400 client response.

The same thing happens with @PathVariable, but I would expect that one would actually be a 404 not found.

[sbrannen]

Using the following test case against master for the Spring Framework...

import java.util.Locale;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Configuration;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.context.WebApplicationContext;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.*;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*;

@RunWith(SpringRunner.class)
@WebAppConfiguration
public class LocaleDeserializationTest {

    @Autowired
    WebApplicationContext wac;

    MockMvc mockMvc;

    @Before
    public void setUp() {
        this.mockMvc = webAppContextSetup(wac).build();
    }

    @Test
    public void badLocaleRequestParam() throws Exception {
        mockMvc.perform(get("/request-param").param("locale", "InvalidLocale"))//
                .andDo(print())//
                .andExpect(status().isBadRequest());
    }

    @Configuration
    static class Config {

        @RestController
        static class LocaleController {

            @GetMapping("/request-param")
            String requestParam(@RequestParam Locale locale) {
                System.err.println(locale);
                System.err.println(locale.toLanguageTag());
                return locale.toLanguageTag();
            }
        }
    }

}

... I see the following output:

invalidlocale
und

Thus, the Locale gets created with "InvalidLocale" stored as "invalidlocale", and the "und" aligns with the expected output as documented in the Javadoc for Locale.toLanguageTag():

Language: If language is empty, or not well-formed (for example "a" or "e2"), it will be emitted as "und" (Undetermined).

So, I would say the feature works as expected against master.

@phillipuniverse, I realize you stated it in this issue's description, but can you please confirm that you see a NullPointerException thrown from your handler method?

[phillipuniverse]

Ah yes, confirmed that using the strict Spring Framework variant @WebAppConfiguration then I get what you did, no NPE and parsing as you described.

If I take your same example and instead apply @WebMvcTest to it, I get the NPE. Here's the copy/paste version with your same JUnit 4:

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest;
import org.springframework.context.annotation.Configuration;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.context.WebApplicationContext;

import java.util.Locale;

@WebMvcTest
@RunWith(SpringRunner.class)
public class IssueCommentLocaleDeserializerTest {

    @Autowired
    WebApplicationContext wac;

    MockMvc mockMvc;

    @Before
    public void setUp() {
        this.mockMvc = webAppContextSetup(wac).build();
    }

    @Test
    public void badLocaleRequestParam() throws Exception {
        mockMvc.perform(get("/request-param").param("locale", "InvalidLocale"))//
                .andDo(print())//
                .andExpect(status().isBadRequest());
    }

    @Configuration
    static class Config {

        @RestController
        static class LocaleController {

            @GetMapping("/request-param")
            String requestParam(@RequestParam Locale locale) {
                System.err.println(locale);
                System.err.println(locale.toLanguageTag());
                return locale.toLanguageTag();
            }
        }
    }
}

Tracing through, the difference is that with a raw @WebAppConfiguration there is no conversionService within the TypeConverterDelegate#propertyEditorRegistry#getConversionService().

More succinctly:

• @WebAppConfiguration - https://github.com/spring-projects/spring-framework/blob/495ba2f58cdd195c4c0d16be5b43c22a8a193d81/spring-beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java#L124 returns null, thus uses the property editor support and undergoes conversion via the LocaleEditor
• @WebMvcTest - https://github.com/spring-projects/spring-framework/blob/495ba2f58cdd195c4c0d16be5b43c22a8a193d81/spring-beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java#L124 returns non-null, and undergoes conversion via StringToLocaleConverter

Looks like therein lies the subtle difference:

• LocaleEditor - uses StringUtils.parseLocaleString()
• StringToLocaleConverter - uses StringUtils.parseLocale()

@Test
public void badLocaleParsing() {
    assertNull(StringUtils.parseLocale("InvalidValue"));
    assertNotNull(StringUtils.parseLocaleString("InvalidValue"));
}

[sbrannen]

Oops... I accidentally left off @EnableWebMvc on my @Configuration class.

With that in place, I now see the NullPointerException you referred to.

Thanks for the analysis and feedback!

[sbrannen]

The following code block appears to be the source of the issue.

https://github.com/spring-projects/spring-framework/blob/e6d206b45a460c2ac2e1ccde8098815163cb6124/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java#L123-L125

What's missing is a check _after_ the conversion.

In the example in the above comments, the initial value is "InvalidLocale" which gets converted to null.

Adding something like the following immediately after the convertIfNecessary() line results in the desired 400 response status.

if (arg == null && namedValueInfo.required) {
    handleMissingValue(namedValueInfo.name, nestedParameter, webRequest);
}

Of course, handleMissingValue() is not really appropriate here, since the value is present but converted to null because the value is invalid/unsupported for the desired conversion to Locale. I didn't invoke handleNullValue() here, because the current implementation of handleNullValue() would not throw an exception for this use case. Though, we could override that method in RequestParamMethodArgumentResolver if we choose to go this route.

In summary, I think we need a way to verify that a converted value conforms to the required=true attribute in @RequestParam, but I'm not certain of how we can do that in a general-purpose fashion since I suppose null may sometimes be an acceptable value, depending on the registered converters.

@rstoyanchev, what are you thoughts on how we could best address this issue.

Rossen, while I've got your attention, do we actually support @NotNull and other JSR-303 annotations on @RequestParam arguments in Spring MVC? I found where we support that for model attributes and request bodies, but I didn't find where we support that for @RequestParam arguments.

[sbrannen]

Tentatively slated for 5.2 M1 for further investigation and design work.

[sbrannen]

Update: I was thinking we could suggest that users register a custom Locale Converter as follows.

public class ExceptionThrowingStringToLocaleConverter implements Converter<String, Locale> {

    @Override
    @Nullable
    public Locale convert(String source) {
        Locale locale = StringUtils.parseLocale(source);
        if (locale == null) {
            throw new RuntimeException("Failed to convert [" + source + "] to a Locale");
        }
        return locale;
    }

}

When I register one of those by implementing WebMvcConfigurer.addFormatters(FormatterRegistry) in my @Configuration class, my converter does in fact get used, but...

1. The RuntimeException gets caught and wrapped in a ConversionFailedException which triggers a lookup of the LocaleEditor (via org.springframework.beans.TypeConverterDelegate.findDefaultEditor(Class<?>)).
2. The LocaleEditor is then used to convert "InvalidLocale" to a Locale containing "invalidlocale".

So we're back where we started, and even a custom Converter doesn't help due to the fallback to LocaleEditor.

[jhoeller]

This turns out to be a bug in StringUtils.parseLocale (which is a recent 5.0.4 introduction meant to parse the BCP 47 language tag format as well) versus StringUtils.parseLocaleString (which only parses the good old Locale.toString() format) since they are not meant to deviate like that. The JDK's Locale.forLanguageTag used there converts non-well-formed language tag values (with more than 8 characters) to empty values; however, it does not enforce a non-empty value to be actually valid...

Specifically, null only happens for a value with more than 8 characters like "InvalidValue" or also simply "invalidva". Parsing the short enough "invalid" as input consistently returns a Locale instance with both parse methods, as does parsing "InvalidValue_InvalidCountry" (since this is not a language tag to begin with). In particular given these very subtle differences in input value format, the only clean way out is to consistently return a Locale instance with the invalid value here like parseLocaleString does, ignoring language tag restrictions. null is only meant to be returned for empty input, after all.

As for the semantics of @RequestParam(required=...), this refers to the request parameter itself, not to a converted value passed into the handle method argument. I'm afraid there is no way to enforce a non-null value for the latter out of the box. That said, with the fix above applied, this shouldn't be null for a Locale anymore since empty input should be caught before conversion even kicks in there, and invalid locale specifications consistently turn into a Locale instance now.

In order to enforce an actually valid locale at the application level, you could check Locale.toLanguageTag for not returning "und" (or for inclusion in Locale.getAvailableLocals()). Please note that you need to do this in any case in order to consistently handle non-well-formed and plain invalid language values (i.e. invalid values with any number of characters). Alternatively, you could pass in the original String value for the parameter and handle it yourself, fully controlling Locale conversion.

[sbrannen]

Related to #22048