Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using a custom ParamConverter fails when not sending that parameter with the request #40287

Closed
michelcouto opened this issue Apr 25, 2024 · 4 comments · Fixed by #42434
Closed
Labels
area/rest kind/bug Something isn't working
Milestone

Comments

@michelcouto
Copy link
Contributor

Describe the bug

When using a custom ParamConverter, the call fails when we do not sending any value for that parameter in the request.

Expected behavior

The request would be executed

Actual behavior

A 404 error happens due to a NPE.

How to Reproduce?

Take this example code:

@GET
    @Path("/greet")
    public Response greet(@QueryParam("number") @Parameter(allowEmptyValue = true) Optional<Integer> numberOpt) {
        if (numberOpt.isPresent()) {
            Integer number= numberOpt.get();
            if (number == null) {
                return Response.ok("Hello! You provided an empty number.").build();
            } else {
                return Response.ok("Hello, " + number+ "!").build();
            }
        } else {
            return Response.ok("Hello, world! No number was provided.").build();
        }
    }

This is the converter:

public class OptionalIntegerParamConverter implements ParamConverter<Optional<Integer>> {

	@Override
    public Optional<Integer> fromString(String value) {
		if (value == null) {
			return null;
		}
		
        if (value.trim().isEmpty()) {
        	return Optional.empty();
        }
        
        try {
        	int parsedInt = Integer.parseInt(value);
        	return Optional.of(parsedInt);
        } catch (NumberFormatException e) {
            throw new IllegalArgumentException("Invalid integer value");
        }
    }
    @Override
    public String toString(Optional<Integer> value) {
    	if (!value.isPresent()) {
    		return null;
    	}
    	
    	Integer intValue = value.get();
        if (intValue == null) {
            return null;
        } else {
            return intValue.toString();
        }
    }

}

}

And this is its provider:

@Provider
public class OptionalIntegerParamConverterProvider implements ParamConverterProvider {
    @SuppressWarnings("unchecked")
	@Override
    public <T> ParamConverter<T> getConverter(Class<T> rawType, Type genericType, Annotation[] annotations) {
    	if (rawType.equals(Optional.class)) {
            if (genericType instanceof ParameterizedType) {
                ParameterizedType parameterizedType = (ParameterizedType) genericType;
                Type[] typeArguments = parameterizedType.getActualTypeArguments();
                if (typeArguments.length == 1 && typeArguments[0].equals(Integer.class)) {
                    return (ParamConverter<T>) new OptionalIntegerParamConverter();
                }
            }
        }
        
        return null; // return null if no converter is needed for the type
    }
}

If that is setup, then trying to call the service without providing any value to the number parameter will fail within RuntimeResolvedConverter

public Object convert(Object parameter) {
        if (runtimeConverter != null)
            return runtimeConverter.fromString(parameter.toString());
        return quarkusConverter.convert(parameter);
    }

it fails because parameter is null.

If I provide no conversion, then it fails only when checking the Send empty value, since "" cannot be converted to an Optional

Thus, a simple change in RuntimeResolvedConverter would fix this:

public Object convert(Object parameter) {
        if (runtimeConverter != null)
            return runtimeConverter.fromString(parameter != null ? parameter.toString() : null);
        return quarkusConverter.convert(parameter);
    }

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@michelcouto michelcouto added the kind/bug Something isn't working label Apr 25, 2024
Copy link

quarkus-bot bot commented Apr 25, 2024

/cc @FroMage (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@geoand
Copy link
Contributor

geoand commented Apr 26, 2024

Thanks for reporting!

As you've already done an analysis on the code, would you like to open a Pull Request with the proposed change?
Ideally, a test would also be included.

Thanks

@michelcouto
Copy link
Contributor Author

Hi!

I have openned a Pull Request with the fix, and also some related tests:
#40311

Since this is the first time I am contributing here, I am not sure that my tests were added correctly...
Please review.

@geoand
Copy link
Contributor

geoand commented Apr 27, 2024

Thanks!

I will check on Monday

gastaldi added a commit to gastaldi/quarkus that referenced this issue Aug 9, 2024
@geoand geoand changed the title resteasy-reactive Using a custom ParamConverter fails when not sending that parameter with the request rUsing a custom ParamConverter fails when not sending that parameter with the request Aug 9, 2024
@geoand geoand changed the title rUsing a custom ParamConverter fails when not sending that parameter with the request Using a custom ParamConverter fails when not sending that parameter with the request Aug 9, 2024
gastaldi added a commit to gastaldi/quarkus that referenced this issue Aug 9, 2024
@geoand geoand closed this as completed in 56d40f1 Aug 9, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Aug 9, 2024
frne pushed a commit to frne/quarkus that referenced this issue Aug 13, 2024
@gsmet gsmet modified the milestones: 3.14.0.CR1, 3.8.6 Aug 14, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Aug 14, 2024
- Fixes quarkusio#40287

Co-Authored-By: Michel do Couto <[email protected]>
(cherry picked from commit 56d40f1)
gsmet pushed a commit to gsmet/quarkus that referenced this issue Aug 19, 2024
- Fixes quarkusio#40287

Co-Authored-By: Michel do Couto <[email protected]>
(cherry picked from commit 56d40f1)
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
3 participants