Nullable body spec #6689
Nullable body spec #6689
Conversation
* #6389: jaeger tracing properties already tested * #6389: jaeger tracing properties already tested * The bean count went up to 39 * PR Fixes * PR Fixes - Update jaeger.adoc * simplify the jeager.adoc Co-authored-by: Pavol Gressa <[email protected]>
The new spec (NullableBodySpec2) runs only under Java 9, where annotations on generic type args (amongst other type annotation issues) were fixed.
| if (source.getAttribute(REQUEST_BINDING_UNSATISFIED).isPresent() && typeVariable.get().isNullable()) { | ||
| return () -> Optional.of(source); | ||
| } else { | ||
| source.setAttribute(REQUEST_BINDING_UNSATISFIED, true); | ||
| return ArgumentBinder.BindingResult.UNSATISFIED; | ||
| } |
There was a problem hiding this comment.
This seems like a hack, can you explain this change?
There was a problem hiding this comment.
This chunk of code is here to handle HttpRequest<@Nullable T> and similar.
For a request with an empty body, the first time the argument binder is called, the body is not yet decoded. The previous version of the code would simply return UNSATISFIED. When the second pass (w/ a decoded body) comes around, the situation is still that body is still null, and returns UNSATISFIED again, even though the the request has @Nullable T body type.
The first solution to try was just:
if (typeVariable.get().isNullable()) {
return () -> Optional.of(source);
} else {
return ArgumentBinder.BindingResult.UNSATISFIED;
}
First attempt to find would see the body was empty and typeVariable nullable, so it returned the lambda () -> Optional.of(source). This works for when the body is null. But if the request has a non-null body, the first pass sees the type variable as nullable, and returns the lambda, but this interferes with the second pass (after non-null body is decoded) which should return new FullHttpRequest(...).
To get around this, I use a custom attribute on the first pass to maintain existing behavior (pre-body-decode). But for the second pass, if the body is still null (determined because the attribute exists) AND the type variable is nullable, then returns the lambda () -> Optional.of(source), which makes all @Nullable T cases happy without messing up other cases/tests.
| /** | ||
| * @return A converter that returns bytebufs to objects | ||
| */ | ||
| protected TypeConverter<ByteBuf, Object> byteBufToObjectConverter() { | ||
| return (object, targetType, context) -> conversionService.convert(object.toString(context.getCharset()), targetType, context); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this not a breaking change?
There was a problem hiding this comment.
I couldn't find any uses of this (which does a strange toString conversion that interferes with other more useful conversions). No tests broke, and no one seemed to think this was a problem when it was brought up during last engineering meeting.
There was a problem hiding this comment.
@graemerocher The presence of the converter prevents proper conversion of ByteBuf to container types. In general I don't think it makes sense because if we don't know the target type we shouldn't assume that a string based conversion will succeed or will be desirable
|
Matthew Moss seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Fixes #3064