GH-3114: Honor SpEL contract in ExpressionEvalMap #3115
Conversation
Fixes spring-projects#3114 The contract of SpEL with its `getValue(EvaluationContext context, @nullable Object rootObject)` is that we need to deal with provided `rootObject` even if it is `null` and don't consult with `context.getRootObject()` * Fix `ExpressionEvalMap` to have an internal `rootExplicitlySet` to indicate that `root` explicitly provided by consumer, even if it is null. According this flag call respective `Expression.getValue()` * Add `@Nullable` to methods and their arguments into `ExpressionEvalMap` & `FunctionExpression` to honor `Expression` contracts * Populate an `HttpEntity` explicitly into `ExpressionEvalMap` from the `HttpRequestHandlingEndpointSupport` and `WebFluxInboundEndpoint` for full picture
| * } | ||
| * }) | ||
| * } | ||
| * }) | ||
| * .build(); |
There was a problem hiding this comment.
Change from tabs to spaces has messed up alignment.
There was a problem hiding this comment.
Doesn't look like, but I've changed everything into spaces. I don't think tabs are good for JavaDocs.
There was a problem hiding this comment.
Right, but you had mixed tabs and spaces here. The indentation is still wrong (extra space before }). Will fix on merge.
| this.context = context; | ||
| this.root = root; | ||
| this.rootExplicitlySet = rootExplicitlySet; |
There was a problem hiding this comment.
It's not clear what the purpose of this is; we still get an NPE if the root has explicitly been set to null.
is that we need to deal with provided rootObject even if it is
null
When I changed the HRHES to .withRoot(null) BOOM!!
There was a problem hiding this comment.
That's correct: it is exactly what SpEL contract does in the getValue(EvaluationContext context, @Nullable Object rootObject).
If you explicitly provide a root as null, then it is used as is, without any consultation with context.
So, we fix an NPE on our level because we really have a root, but we still don't break SpEL contract when root is explicitly provided as null
There was a problem hiding this comment.
So you are saying the NPE is ok, because it's in "user" code (in this case, the lambda) so the user is responsible for a null check?
There was a problem hiding this comment.
That's right if (s)he provides a null explicitly, but in our case we just miss the proper root to propagate.
See all the Expression.getValue() with root - it is @Nullable. Therefore without checking for null it is really going to lead into NPE.
But in our case we provide all the stuff for expression environment, therefore it is safe not check because we guarantee now a non-null root.
See changes in those HTTP components.
|
Merged as 516ecbc |
Fixes #3114
The contract of SpEL with its
getValue(EvaluationContext context, @Nullable Object rootObject)isthat we need to deal with provided
rootObjecteven if it isnulland don't consult with
context.getRootObject()ExpressionEvalMapto have an internalrootExplicitlySetto indicate that
rootexplicitly provided by consumer, even if it is null.According this flag call respective
Expression.getValue()@Nullableto methods and their arguments intoExpressionEvalMap&
FunctionExpressionto honorExpressioncontractsHttpEntityexplicitly intoExpressionEvalMapfrom theHttpRequestHandlingEndpointSupportandWebFluxInboundEndpointforfull picture