Add config option for strict request argument error checking

[yawkat]

When the value of a request argument (e.g. a @QueryValue) is invalid, but the parameter is marked @Nullable, strict conversion checking will produce an error. Without this option enabled, the parameter will simply be set to null.

I've added a parameter to the RequestArgumentSatisfier constructor. The class is @Internal and a code search of the micronaut-projects org shows no subclasses outside micronaut-core, so this seems safe.

Fixes #5135

[timyates]

Should we also be failing for Optionals?

Adding a test of

        HttpMethod.GET  | '/parameterNullable/optional-named?maximum=inv'         | null      | HttpStatus.BAD_REQUEST

and a Get handler of

        @Get("/optional-named")
        String optionalNamed(@QueryValue('maximum') Optional<Integer> max) {
            "Parameter Value: $max"
        }

Fails as max is set to Optional.empty(), and the request succeeds

[timyates]

Also, given:

        HttpMethod.GET  | '/parameterNullable/named-list?max=inv&max=2'           | null                  | HttpStatus.BAD_REQUEST

With a handler of:

        @Get("/named-list")
        String namedList(@QueryValue('max') @Nullable List<Integer> maxList) {
            "Parameter Value: $maxList"
        }

We get an OK response with a result of Parameter Value: [2]

~~I think we need to make this change https://github.com/micronaut-projects/micronaut-core/pull/6808/files#diff-fb96e6a08d20e10f1121573768dd86c523b72869333bf2278a20278a155b9c16 so the collection errors get passed up~~

EDIT: Nope...that's not enough 🤔 posted the fix (I think) in the review comment

[yawkat]

Optional should be easy, it's just the code path above the ones that already changed. Will discuss in the meeting

[yawkat]

also decided to test making this a ApplicationContextBuilder-level setting instead, will test.

[graemerocher]

This should go into 3.3.x IMO as although this is a new configuration option, the existing behaviour is broken and inconsistent. I understand the motivation to not break existing applications but it makes no sense that the following:

@Get("/{num}")
String get(@Nullable Long num) {
    //
}

With a request of /junk results in an exception, whilst the following:

@Get("/{?num}")
String get(@QueryValue @Nullable Long num) {

}

With a request of /?num=junk does not and ignores the error silently.

[yawkat]

@jameskleeh one problem with doing this as a more general setting is that we have to decide now where to implement this strict conversion checking. We can't add the stricter checks to other places later, without breaking compatibility.

[kevin-wise]

any idea when this might get merged?

[graemerocher]

@yawkat can you rebase this and maybe we should make this the default for 4.x?

[yawkat]

Yea I made this PR draft because I wanted to try to change it at a higher level (in ConversionService) but I dont think thats viable, so this PR should be fine once rebased.

[sdelamo]

@yawkat should we close this?

[yawkat]

no it's still on my todo. just not this approach

[CLAassistant]

All committers have signed the CLA.