jooby-project / jooby

The modular web framework for Java and Kotlin
https://jooby.io
Apache License 2.0
1.68k stars 199 forks source link

SingleValue converter exceptions need "name" in the exception. #3465

Closed agentgt closed 1 week ago

agentgt commented 2 weeks ago

When a SingleValue does its conversion it is painful to figure out which parameter actually caused the exception even though SingleValue has the name.

java.lang.IllegalArgumentException: UUID string too large
    at java.base/java.util.UUID.fromString1(UUID.java:266)
    at java.base/java.util.UUID.fromString(UUID.java:260)
    at io.jooby.internal.converter.UUIDConverter.convert(UUIDConverter.java:21)
    at io.jooby.internal.ValueConverters.convert(ValueConverters.java:119)
    at io.jooby.internal.ValueConverters.convert(ValueConverters.java:71)
    at io.jooby.DefaultContext.convertOrNull(DefaultContext.java:410)
    at io.jooby.internal.SingleValue.toNullable(SingleValue.java:95)
    at SomeController_.get(SomeController_.java:37)

In SingleValue we have:

  @NonNull @Override
  public <T> T to(@NonNull Class<T> type) {
    return ctx.convert(this, type);
  }

  @Nullable @Override
  public <T> T toNullable(@NonNull Class<T> type) {
    return ctx.convertOrNull(this, type);
  }

In theory we do not need to change API at all but have the builtin converters check if the passed in this has a Value.name() and wrap the exception.

jknack commented 2 weeks ago

I kind of like bc we don't have super long stacktrace by not wrapping the exception.

agentgt commented 2 weeks ago

Yeah I'm concerned it somehow might impact performance.

I think if one wanted it they could just wrap a context with a ForwardingContext and do their own exception wrapping?

EDIT I should say I don't know if it really would impact performance but I have seen where doing something like:

try {
  blah.doSomething();
catch(Exception e) {
  throw new SomeException("...", e);
}

Slows down things especially given some of those calls are converting string to native.

jknack commented 2 weeks ago

yea that is what I tried to avoid and the reason of SneakyThrows.propagate

agentgt commented 2 weeks ago

yea that is what I tried to avoid and the reason of SneakyThrows.propagate

Yes I use this technique as well in some other libraries.

I think the ForwardingContext is the best solution for this and I think it will work (and override convert).

I will close this issue once I make sure that option can work.

agentgt commented 2 weeks ago

No I guess ForwardingContext will not work as query(), form() etc take the concrete Context (this).