Handle an empty query parameters with @Default

[ikhoon]

Say we have an annotated service that uses @Default annotation to optionally bind query parameters.

class MyService {
    @Get("/list")
    public String list(@Default @Param Integer page) {
        ...
    }
}

As per the Javadoc of @Default, when a value is not specified for @Default, a non-existent parameter is converted into null. page is null if /list is the request path.

armeria/core/src/main/java/com/linecorp/armeria/server/annotation/Default.java

Lines 35 to 36 in ccb29fe

	* When {@link Default} annotation exists but {@link Default#value()} is not specified, {@code null}
	* value would be set if the parameter is not present in the request.

However, it does not work well if there is a parameter but no value. A NumberFormatException is raised when /list?page= is sent as the request path.

java.lang.NumberFormatException: For input string: ""
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
	at java.base/java.lang.Integer.parseInt(Integer.java:671)
	at java.base/java.lang.Integer.valueOf(Integer.java:988)
	at com.linecorp.armeria.internal.server.annotation.AnnotatedServiceTypeUtil.stringToType(AnnotatedServiceTypeUtil.java:177)
	at com.linecorp.armeria.internal.server.annotation.AnnotatedValueResolver.convert(AnnotatedValueResolver.java:1021)

It would be better to solve this problem as follows the value of the parameter is empty.

• Inject an empty string ("") if the type of page is String.
• Inject a null value if the type of page is not String such as Integer, Boolean and so on.

[ikhoon]

In addition to the bugfix, we also need to improve the error message when an incorrect request is made so that users can easily identify the problem.

[facewise]

@ikhoon
Hello. Can I pick up this issue?

[ikhoon]

Yes, please. 😀

[facewise]

@ikhoon
What if the type of parameter is primitive such as int, long?
These don't allow null values.

class MyService {
    @Get("/list")
    public String list(@Default @Param int page) {
        ...
    }
}

and the parameter exists but the value has not been present like
/list?page=

In this case, which response do you prefer?

[facewise]

@ikhoon

Plus, in terms of handling empty query parameters, I am considering couple of cases like below:

Case 1

Say service is like:

class MyService {
    @Get("/list")
    public String list(@Default("1") @Param List<Integer> params) {
        ...
    }
}

But the request is sent like /list?params=.

Then which action would be processed?

1. Inject null to params
2. Inject [1] to params
3. Throw an exception

Case 2

If service is like

class MyService {
    @Get("/list")
    public String list(@Default @Param List<Integer> params) {
        ...
    }
}

and the request is sent like /list?params=.

Then which action would be processed?

1. Inject null to params
2. Inject [null] to params
3. Throw an exception

Case 3

If service is like

class MyService {
    @Get("/list")
    public String list(@Default @Param List<String> params) {
        ...
    }
}

and the request is sent like /list?params=.

Then which action would be processed?

1. Inject null to params
2. Inject [""] to params
3. Inject [null] to params
4. Throw an exception

Please give me your opinion for enhancement. Thank you.

[ikhoon]

What if the type of parameter is primitive such as int, long?
In this case, which response do you prefer?

In that case, IllegalArgumentException might be good. IllegalArgumentException will be translated into a 404 Bad request by the default error handler.

Case 1

2. that injects [1] to params looks good to me.

Case 2 and Case 3

I prefer to inject [] an empty list.

Thanks for your thorough analysis. 👍