Having @QueryParam without @DefaultValue throws expensive IllegalArgumentException for every missing querystring parameter

[paulrutter]

When performance testing our new Jersey 2.38 application, i noticed that some endpoints were much slower than before (when using Apache Wink).

I narrowed it down with Yourkit and noticed that IllegalArgumentException is thrown for every querystring parameter that's not present in the request querystring. When adding a @DefaultValue, this doesn't occur though.

See relevant code here: https://github.com/eclipse-ee4j/jersey/blob/2.38/core-common/src/main/java/org/glassfish/jersey/internal/inject/ParamConverters.java#L63

Throwing exceptions to catch "regular" application flow is generally a bad habit and decreases performance of the application (https://www.baeldung.com/java-exceptions-performance).
Is this intended behavior? E.g. when specifying a @QueryParam without @DefaultValue, does that automatically make it a required querystring parameter?

The JAX-RS specification doesn't seem to reflect this, having an optional querystring parameter without a default value seems legit and should just return null.
It becomes more noticable when you have endpoints with a lot of optional querystring parameters.
More info can be found here https://stackoverflow.com/a/35625547/3032647.

I would suggest that the relevant method could also just return null in this case:

      @Override
      public T fromString(final String value) {
          if (value == null) {
              //throw new IllegalArgumentException(LocalizationMessages.METHOD_PARAMETER_CANNOT_BE_NULL("value"));
              return null;
          }
          try {
              return _fromString(value);
          } catch (final InvocationTargetException ex) {
              // if the value is an empty string, return null
              if (value.isEmpty()) {
                  return null;
              }
              final Throwable cause = ex.getCause();
              if (cause instanceof WebApplicationException) {
                  throw (WebApplicationException) cause;
              } else {
                  throw new ExtractorException(cause);
              }
          } catch (final Exception ex) {
              throw new ProcessingException(ex);
          }
      }

The same goes for querystring parameters of different types, like int. These throw a NumberFormatException.

[jansupol]

In general, this is a very good point. I do not know why Jersey was implemented that way - I try to dig deeper to see if that's necessary. We would love to have Jersey as fast possible - and replacing these exception-catching mechanisms can improve the performance.

However, we need to check if the IAE can in any case be thrown. There might be a requirement from the Javadoc or somewhere to see the IAE. I hope not, though.

[paulrutter]

From what i can find in the code,

jersey/core-server/src/main/java/org/glassfish/jersey/server/internal/inject/SingleValueExtractor.java

Line 65 in 7b6d2f8

if (value == null) {

, the IAE is caught and then it returns defaultValue, which is null because there is no default value registered.
The latter was already checked when calling the fromString method, so it seems superfluous to use an exception for it.

The question is if IAE can also be thrown in other (valid) cases, but it would still be viable to keep the catch clause for IAE in for those situations. But just don't throw it when a null value is passed.

[jansupol]

The Javadoc to ParamConverter mandates an IllegalArgumentException is thrown if the supplied string cannot be parsed or is null.

In Jersey, the IAE is always converted to null. But in theory, any user can grab all ParamConverterProviders and use the ParamConverter directly, claiming we are not spec compliant.

That's what likely never will happen, though. But a property should enable requesting spec-compliant behavior.

[paulrutter]

Thanks for fixing this, will try it out and run our performance tests against 2.40.

[paulrutter]

@jansupol i verified that the changes in 2.40 indeed work as expected. The @DefaultValue annotations are no longer needed and yield the same performance metrics now. Thanks!