Skip to content

GH-3114: Honor SpEL contract in ExpressionEvalMap #3115

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

GH-3114: Honor SpEL contract in ExpressionEvalMap #3115

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04bfd96
GH-3114: Honor SpEL contract in ExpressionEvalMap
artembilan Nov 22, 2019
16df9d2
* Fix JavaDocs indentations in the `ExpressionEvalMap`
artembilan Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions ...tion-core/src/main/java/org/springframework/integration/expression/ExpressionEvalMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@
* .usingCallback(new EvaluationCallback() {
* Object evaluate(Expression expression) {
* // return some expression evaluation
* }
* })
* }
* })
* .build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change from tabs to spaces has messed up alignment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like, but I've changed everything into spaces. I don't think tabs are good for JavaDocs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but you had mixed tabs and spaces here. The indentation is still wrong (extra space before }). Will fix on merge.

*}
* </pre>
Expand Down Expand Up @@ -84,6 +84,7 @@ private ExpressionEvalMap(Map<String, ?> original, EvaluationCallback evaluation
* from {@link #original} and returns the result of evaluation using {@link #evaluationCallback}.
*/
@Override
@Nullable
public Object get(Object key) {
Object value = this.original.get(key);
if (value != null) {
Expand All @@ -106,9 +107,9 @@ else if (value instanceof String) {

@Override
public Set<Map.Entry<String, Object>> entrySet() {
return this.original.entrySet()
return this.original.keySet()
.stream()
.map(e -> new SimpleImmutableEntry<>(e.getKey(), get(e.getKey())))
.map((key) -> new SimpleImmutableEntry<>(key, get(key)))
.collect(Collectors.toSet());
}

Expand Down Expand Up @@ -206,22 +207,35 @@ public interface EvaluationCallback {
*/
public static class ComponentsEvaluationCallback implements EvaluationCallback {

@Nullable
private final EvaluationContext context;

@Nullable
private final Object root;

private final boolean rootExplicitlySet;

@Nullable
private final Class<?> returnType;

public ComponentsEvaluationCallback(EvaluationContext context, Object root, Class<?> returnType) {
public ComponentsEvaluationCallback(@Nullable EvaluationContext context, @Nullable Object root,
boolean rootExplicitlySet, @Nullable Class<?> returnType) {

this.context = context;
this.root = root;
this.rootExplicitlySet = rootExplicitlySet;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.returnType = returnType;
}

@Override
public Object evaluate(Expression expression) {
if (this.context != null) {
return expression.getValue(this.context, this.root, this.returnType);
if (this.rootExplicitlySet) {
return expression.getValue(this.context, this.root, this.returnType);
}
else {
return expression.getValue(this.context, this.returnType);
}
}
return expression.getValue(this.root, this.returnType);
}
Expand All @@ -238,10 +252,15 @@ public static final class ExpressionEvalMapBuilder {

private EvaluationCallback evaluationCallback;

@Nullable
private EvaluationContext context;

@Nullable
private Object root;

private boolean rootExplicitlySet;

@Nullable
private Class<?> returnType;

private final ExpressionEvalMapComponentsBuilder evalMapComponentsBuilder =
Expand All @@ -267,8 +286,9 @@ public ExpressionEvalMapComponentsBuilder usingEvaluationContext(EvaluationConte
return this.evalMapComponentsBuilder;
}

public ExpressionEvalMapComponentsBuilder withRoot(Object root) {
public ExpressionEvalMapComponentsBuilder withRoot(@Nullable Object root) {
this.root = root;
this.rootExplicitlySet = true;
return this.evalMapComponentsBuilder;

}
Expand All @@ -295,7 +315,8 @@ public ExpressionEvalMap build() {
else {
return new ExpressionEvalMap(ExpressionEvalMapBuilder.this.expressions,
new ComponentsEvaluationCallback(ExpressionEvalMapBuilder.this.context,
ExpressionEvalMapBuilder.this.root, ExpressionEvalMapBuilder.this.returnType));
ExpressionEvalMapBuilder.this.root, ExpressionEvalMapBuilder.this.rootExplicitlySet,
ExpressionEvalMapBuilder.this.returnType));
}
}

Expand All @@ -315,7 +336,7 @@ public ExpressionEvalMapComponentsBuilder usingEvaluationContext(EvaluationConte
}

@Override
public ExpressionEvalMapComponentsBuilder withRoot(Object root) {
public ExpressionEvalMapComponentsBuilder withRoot(@Nullable Object root) {
return ExpressionEvalMapBuilder.this.withRoot(root);
}

Expand All @@ -340,7 +361,7 @@ public interface ExpressionEvalMapComponentsBuilder extends ExpressionEvalMapFin

ExpressionEvalMapComponentsBuilder usingEvaluationContext(EvaluationContext context);

ExpressionEvalMapComponentsBuilder withRoot(Object root);
ExpressionEvalMapComponentsBuilder withRoot(@Nullable Object root);

ExpressionEvalMapComponentsBuilder withReturnType(Class<?> returnType);

Expand Down
52 changes: 34 additions & 18 deletions ...ion-core/src/main/java/org/springframework/integration/expression/FunctionExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.springframework.expression.TypedValue;
import org.springframework.expression.common.ExpressionUtils;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.lang.Nullable;
import org.springframework.util.Assert;

/**
Expand All @@ -47,6 +48,7 @@
*
* @author Artem Bilan
* @author Gary Russell
*
* @since 5.0
*/
public class FunctionExpression<S> implements Expression {
Expand All @@ -65,46 +67,58 @@ public FunctionExpression(Function<S, ?> function) {
}

@Override
@Nullable
public Object getValue() throws EvaluationException {
return this.function.apply(null);
}

@Override
@Nullable
@SuppressWarnings("unchecked")
public Object getValue(Object rootObject) throws EvaluationException {
public Object getValue(@Nullable Object rootObject) throws EvaluationException {
return this.function.apply((S) rootObject);
}

@Override
public <T> T getValue(Class<T> desiredResultType) throws EvaluationException {
@Nullable
public <T> T getValue(@Nullable Class<T> desiredResultType) throws EvaluationException {
return getValue(this.defaultContext, desiredResultType);
}

@Override
public <T> T getValue(Object rootObject, Class<T> desiredResultType) throws EvaluationException {
@Nullable
public <T> T getValue(@Nullable Object rootObject, @Nullable Class<T> desiredResultType)
throws EvaluationException {

return getValue(this.defaultContext, rootObject, desiredResultType);
}

@Override
@Nullable
public Object getValue(EvaluationContext context) throws EvaluationException {
Object root = context.getRootObject().getValue();
return root == null ? getValue() : getValue(root);
return getValue(context.getRootObject().getValue());
}

@Override
public Object getValue(EvaluationContext context, Object rootObject) throws EvaluationException {
@Nullable
public Object getValue(EvaluationContext context, @Nullable Object rootObject) throws EvaluationException {
return getValue(rootObject);
}

@Override
public <T> T getValue(EvaluationContext context, Class<T> desiredResultType) throws EvaluationException {
@Nullable
public <T> T getValue(EvaluationContext context, @Nullable Class<T> desiredResultType) throws EvaluationException {
return ExpressionUtils.convertTypedValue(context, new TypedValue(getValue(context)), desiredResultType);
}

@Override
public <T> T getValue(EvaluationContext context, Object rootObject, Class<T> desiredResultType)
@Nullable
public <T> T getValue(EvaluationContext context, @Nullable Object rootObject, @Nullable Class<T> desiredResultType)
throws EvaluationException {
return ExpressionUtils.convertTypedValue(context, new TypedValue(getValue(rootObject)), desiredResultType);

return ExpressionUtils.convertTypedValue(context,
new TypedValue(getValue(context, rootObject)),
desiredResultType);
}

@Override
Expand All @@ -113,7 +127,7 @@ public Class<?> getValueType() throws EvaluationException {
}

@Override
public Class<?> getValueType(Object rootObject) throws EvaluationException {
public Class<?> getValueType(@Nullable Object rootObject) throws EvaluationException {
throw this.readOnlyException;
}

Expand All @@ -123,7 +137,7 @@ public Class<?> getValueType(EvaluationContext context) throws EvaluationExcepti
}

@Override
public Class<?> getValueType(EvaluationContext context, Object rootObject) throws EvaluationException {
public Class<?> getValueType(EvaluationContext context, @Nullable Object rootObject) throws EvaluationException {
throw this.readOnlyException;
}

Expand All @@ -133,7 +147,7 @@ public TypeDescriptor getValueTypeDescriptor() throws EvaluationException {
}

@Override
public TypeDescriptor getValueTypeDescriptor(Object rootObject) throws EvaluationException {
public TypeDescriptor getValueTypeDescriptor(@Nullable Object rootObject) throws EvaluationException {
throw this.readOnlyException;
}

Expand All @@ -143,23 +157,25 @@ public TypeDescriptor getValueTypeDescriptor(EvaluationContext context) throws E
}

@Override
public TypeDescriptor getValueTypeDescriptor(EvaluationContext context, Object rootObject)
public TypeDescriptor getValueTypeDescriptor(EvaluationContext context, @Nullable Object rootObject)
throws EvaluationException {
throw this.readOnlyException;
}

@Override
public void setValue(EvaluationContext context, Object value) throws EvaluationException {
public void setValue(EvaluationContext context, @Nullable Object value) throws EvaluationException {
throw this.readOnlyException;
}

@Override
public void setValue(Object rootObject, Object value) throws EvaluationException {
public void setValue(@Nullable Object rootObject, @Nullable Object value) throws EvaluationException {
throw this.readOnlyException;
}

@Override
public void setValue(EvaluationContext context, Object rootObject, Object value) throws EvaluationException {
public void setValue(EvaluationContext context, @Nullable Object rootObject, @Nullable Object value)
throws EvaluationException {

throw this.readOnlyException;
}

Expand All @@ -169,12 +185,12 @@ public boolean isWritable(EvaluationContext context) throws EvaluationException
}

@Override
public boolean isWritable(EvaluationContext context, Object rootObject) throws EvaluationException {
public boolean isWritable(EvaluationContext context, @Nullable Object rootObject) throws EvaluationException {
return false;
}

@Override
public boolean isWritable(Object rootObject) throws EvaluationException {
public boolean isWritable(@Nullable Object rootObject) throws EvaluationException {
return false;
}

Expand Down
1 change: 1 addition & 0 deletions ...java/org/springframework/integration/http/inbound/HttpRequestHandlingEndpointSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ private Message<?> actualDoHandleRequest(HttpServletRequest servletRequest, Requ
headers.putAll(
ExpressionEvalMap.from(getHeaderExpressions())
.usingEvaluationContext(evaluationContext)
.withRoot(httpEntity)
.build());
}

Expand Down
6 changes: 4 additions & 2 deletions ...integration-http/src/test/java/org/springframework/integration/http/dsl/HttpDslTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,9 @@ public void testMultiPartFiles() throws Exception {

Message<?> result = this.multiPartFilesChannel.receive(10_000);

assertThat(result).isNotNull();
assertThat(result.getHeaders()).containsEntry("contentLength", -1L);
assertThat(result)
.isNotNull()
.extracting(Message::getPayload)
.satisfies((payload) ->
assertThat((Map<String, ?>) payload)
Expand Down Expand Up @@ -321,7 +322,8 @@ public IntegrationFlow httpProxyErrorFlow() {
@Bean
public IntegrationFlow multiPartFilesFlow() {
return IntegrationFlows
.from(Http.inboundChannelAdapter("/multiPartFiles"))
.from(Http.inboundChannelAdapter("/multiPartFiles")
.headerFunction("contentLength", (entity) -> entity.getHeaders().getContentLength()))
.channel((c) -> c.queue("multiPartFilesChannel"))
.get();
}
Expand Down
1 change: 1 addition & 0 deletions ...src/main/java/org/springframework/integration/webflux/inbound/WebFluxInboundEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ private Mono<Tuple2<Message<Object>, RequestEntity<?>>> buildMessage(RequestEnti
headers.putAll(
ExpressionEvalMap.from(getHeaderExpressions())
.usingEvaluationContext(evaluationContext)
.withRoot(httpEntity)
.build());
}

Expand Down