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.

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

INT-2570 Support url-expression on Outbound Http #458

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@garyrussell
Member

garyrussell commented May 24, 2012

Add url-expression as an alternative to url for outbound
http adapters. One of url or url-expression must be
provided (but not both).

The Message is the root object for the expression's
evaluation context; the bean factory can also be
used to resolve '@beanName' expressions.

garyrussell added some commits May 24, 2012

INT-2570 Support url-expression on Outbound Http
Add url-expression as an alternative to url for outbound
http adapters. One of url or url-expression must be
provided (but not both).

The Message is the root object for the expression's
evaluation context; the bean factory can also be
used to resolve '@beanName' expressions.
INT-2570 url-expression Reference Docs
Add note to the reference guide.
configureEvaluationContext(uriContext);
uriContext.setRootObject(requestMessage);
uri = (String) this.uriExpression.getValue(uriContext);
Assert.notNull(uri, "URI Expression cannot resolve to null");

This comment has been minimized.

@artembilan

artembilan May 24, 2012

Member

"URI Expression cannot be resolved to null" ?

@Override
public <T> ResponseEntity<T> exchange(String url, HttpMethod method, HttpEntity<?> requestEntity,
Class<T> responseType, Map<String, ?> uriVariables) throws RestClientException {
resolvedUri = url;

This comment has been minimized.

@artembilan

artembilan May 24, 2012

Member

Gary, maybe will it be enough to add this operator to MockRestTemplate2 and remove @SuppressWarnings("unused") ? ;-)
Or let it be: after all, this is just a test ...

This comment has been minimized.

@garyrussell

garyrussell May 24, 2012

Member

...2 has to be instantiated outside of the test class's scope, for other tests, so I would have to change the 'resolvedUri' field to be static too, which doesn't feel right. Plus, the test using ...2 has noting to do with 'resolvedUri'.

This comment has been minimized.

@artembilan

artembilan May 24, 2012

Member

So, maybe it will be OK to do it with Mockito.doAnswer ?
E.g. FileTransferringMessageHandlerTests#testRemoteDirWithEmptyString

@@ -279,6 +279,22 @@ In the case of the Outbound Gateway, the reply message produced by the gateway w
order="3"
auto-startup="false"/>]]></programlisting>
</para>
<note>
<para>
2.2. Introduced a second method to specify the URL; you can use either the 'url' attribute

This comment has been minimized.

@artembilan

artembilan May 24, 2012

Member

IMO it is bad practice to mention during entire manual about version, when changes was made...
For this one we have Appendix D.
Also about 'method' word. As a developer I was a bit confused what do you mean :-)
Maybe 'way'?

StandardEvaluationContext uriContext = new StandardEvaluationContext();
configureEvaluationContext(uriContext);
uriContext.setRootObject(requestMessage);
uri = (String) this.uriExpression.getValue(uriContext);

This comment has been minimized.

@olegz

olegz May 24, 2012

Contributor

You can use:

uri = this.uriExpression.getValue(uriContext, String.class);
if (uri == null) {
StandardEvaluationContext uriContext = new StandardEvaluationContext();
configureEvaluationContext(uriContext);
uriContext.setRootObject(requestMessage);

This comment has been minimized.

@olegz

olegz May 24, 2012

Contributor

You can eliminate this line by

StandardEvaluationContext uriContext = new StandardEvaluationContext(requestMessage)

. . i think

This comment has been minimized.

@olegz

olegz May 24, 2012

Contributor

Also, I am sure you can use the same instance of the EvaluationContext that is used by uri-variables (see line 284)

This comment has been minimized.

@garyrussell

garyrussell May 24, 2012

Member

I was influenced by createEvaluationContext() in HttpRequestHandlingEndpointSupport, which creates a new instance for each request.

Since we set the root object, doesn't that imply it's not thread-safe?

This comment has been minimized.

@olegz

olegz May 25, 2012

Contributor

Well it depends on how it is used, that's why I point you to line 284.

Object value = entry.getValue().getValue(this.evaluationContext, requestMessage, String.class);

As you can see we pass a Message in getValue(..) call essentially swapping the root object

This comment has been minimized.

@garyrussell

garyrussell May 25, 2012

Member

I'll take a look; but why didn't the inbound use that technique?

This comment has been minimized.

@olegz

olegz May 25, 2012

Contributor

That I can't tell (can't remember), but it doesn't matter I guess. . . Its the difference between fully initializing EvaluationContext with the root object OR initializing it without root object while passing it with every evaluation. I think both code was initially written when we were all also learning SpEL API so there is a bit of an inconsistency there, but I hope you can see that its doing the same thing

This comment has been minimized.

@garyrussell

garyrussell May 25, 2012

Member

I see what you mean; you probably need to reject my other PR, adding the view-expression to the inbound endpoint because that, too, was based on the existing code in that class.

This comment has been minimized.

@olegz

olegz May 25, 2012

Contributor

I agree, i'l close it

expressionDef.getConstructorArgumentValues().addGenericArgumentValue(urlExpressionAttribute);
builder.addConstructorArgValue(expressionDef);
}
}

This comment has been minimized.

@olegz

olegz May 24, 2012

Contributor

I think you can simplify the overall logic by always setting an expression (uri or uri-expression)
We do it in many places
You'll have org.springframework.expression.common.LiteralExpression for url and the logic you have for uri-expression, than in HttpRequestExecutingMessageHandler you can have a single attribute (e.g., uriExpression)

This comment has been minimized.

@olegz

olegz May 24, 2012

Contributor

Just to add. What I said may require modification to the original constructor in HREMH which now takes URL as String, but since its already a breaking change, might as well make it more concise now. wdyt?

This comment has been minimized.

@garyrussell

garyrussell May 25, 2012

Member

It's one thing to have a breaking change that impacts some people that use {url} compared to changing a constructor - a much bigger breaking change IMHO.

I suppose we could leave the old constructor and just have it create a LiteralExpression from the String.

wdyt?

This comment has been minimized.

@olegz

olegz May 25, 2012

Contributor

I see your point. Yeah, after all the thing that really matters to avoid unnecessary IF statements so if both attributes at the end will be Expression than its fine. So yes I agree.

}
}
private void configureEvaluationContext(StandardEvaluationContext evaluationContext) {

This comment has been minimized.

@artembilan

artembilan May 24, 2012

Member

So, right now it is something like HttpRequestHandlingEndpointSupport#createEvaluationContext()
Maybe is there any reason to move it somewhere up as public static method into IntegrationContextUtils or as protected in the IntegrationObjectSupport?

* Create a handler that will send requests to the provided URI using a provided RestTemplate
* @param uriExpression A SpEL Expression that can be resolved agains the message object and
* bean factory.
* @param restTemplateq

This comment has been minimized.

@artembilan

artembilan May 25, 2012

Member

typo: 'restTemplate'

@garyrussell

This comment has been minimized.

Member

garyrussell commented May 25, 2012

Guys, thanks for a great review - much simpler now; see #459

Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment