INT-2822: 'requires-reply' for outbound gateways #768

Closed
wants to merge 8 commits into
from

Projects

None yet

3 participants

@artembilan
Member
  • Add requires-reply attribute for all adapters outbound gateways as true by default
  • WS-outbound-gateway is still without it, because it has its own specific attribute ignore-empty-responses
  • Make requires-reply as false by default for jdbc:stored-proc-outbound-gateway inasmuch as jdbc:stored-proc-outbound-adapter
    doesn't have ability to configure returning-resultset
  • add parser tests for requires-reply

JIRA: https://jira.springsource.org/browse/INT-2822

@artembilan
Member

Added commit about requires-reply for <ws:outbound-gateway>. See: http://forum.springsource.org/showthread.php?135953

@artembilan
Member

Rabased and polished

@markfisher
Member

Sorry but it's not immediately obvious to me.... do 'ignore-empty-responses' and 'requires-reply' potentially conflict with each other?

If so, it seems like we should deprecate 'ignore-empty-responses' (for 3.0 and remove in the next minor version upgrade) and include a note in the migration guide that people should go ahead and change to 'requires-reply' instead.

@artembilan
Member

Hi, Mark!
Thanks for your attention.
In case we'll deprecate ignore-empty-responses and rely only on requires-reply we have to decide what to do with empty Strings in the Response: or we leave them as is and let to end-developer to make decision about it, or return null anyway.
IMO: empty Strings in the Response is some rare case and I don't see any problem to leave it to the conscience of the end-developer.
And one more point out of this PR: https://jira.springsource.org/browse/INT-2553

So, I'm waiting your conclusion and will go ahead.

@artembilan
Member

Pushed commit about deprecation ignore-empty-responses for ws:outbound-gateway.
Added note to the What's New

@artembilan
Member

Added commit about removal ignore-empty-responses tip from Reference Manual.
Special note will be added to the Migration Guide after merge this PR.

@artembilan
Member

Pushed polishing after rebase

@artembilan
Member

Pushed polishing commit after rebase.

@garyrussell garyrussell and 1 other commented on an outdated diff Aug 15, 2013
...integration/ws/AbstractWebServiceOutboundGateway.java
public void setIgnoreEmptyResponses(boolean ignoreEmptyResponses) {
- this.ignoreEmptyResponses = ignoreEmptyResponses;
+ //No-op
}
@garyrussell
garyrussell Aug 15, 2013 Member

This is not really a deprecation, right? We're changing the behavior (ignoring the setting and advising them to use the requires-reply).

I think I'm ok with that, but maybe we should log a WARN and refer them to the migration guide???

@artembilan
artembilan Aug 16, 2013 Member

Thank you for WARN: will be added.
Migration Guide note will be added after merge.
I leave this method here (not remove it), because I saw similar solution somewhere in SPR

@garyrussell
Member

Otherwise LGTM

artembilan added some commits Mar 18, 2013
@artembilan artembilan INT-2822: 'requires-reply' for outbound gateways
* Add `requires-reply` attribute for all adapters outbound gateways as `true` by default
* WS-outbound-gateway is still without it, because it has its own specific attribute `ignore-empty-responses`
* Make `requires-reply` as `false` by default for `jdbc:stored-proc-outbound-gateway` inasmuch as `jdbc:stored-proc-outbound-adapter`
doesn't have ability to configure `returning-resultset`
* add parser tests for `requires-reply`

JIRA: https://jira.springsource.org/browse/INT-2822
f1fba65
@artembilan artembilan INT-2822 'requires-reply' for ws:outbound-gateway 8061322
@artembilan artembilan INT-2822: Polishing after rebase b866f9c
@artembilan artembilan INT-2822: deprecate 'ignore-empty-responses'
* Add 'requires-reply' section into What's New
c4c5b69
@artembilan artembilan INT-2822: remove 'ignore-empty-responses' from RM 08c6e97
@artembilan artembilan INT-2822: Polishing after rebase 14f21cd
@artembilan artembilan INT-2822: Rebased and polished 3f11833
@artembilan artembilan INT-2822: Rebased and polished
Add WARN within deprecated `AbstractWebServiceOutboundGateway#setIgnoreEmptyResponses`
758f552
@artembilan
Member

Gary, pushed commit regarding WARN.

@garyrussell garyrussell commented on the diff Aug 16, 2013
...integration/ws/AbstractWebServiceOutboundGateway.java
@@ -167,15 +168,7 @@ public final Object handleRequestMessage(Message<?> requestMessage) {
throw new MessageDeliveryException(requestMessage, "Failed to determine URI for " +
"Web Service request in outbound gateway: " + this.getComponentName());
}
- Object responsePayload = this.doHandle(uri.toString(), requestMessage, this.requestCallback);
- if (responsePayload != null) {
- boolean shouldIgnore = (this.ignoreEmptyResponses
- && responsePayload instanceof String && !StringUtils.hasText((String) responsePayload));
- if (!shouldIgnore) {
- return responsePayload;
- }
- }
- return null;
+ return this.doHandle(uri.toString(), requestMessage, this.requestCallback);
@garyrussell
garyrussell Aug 16, 2013 Member

I was just about to merge this but, after further thought, it seems to me we shouldn't remove (or perhaps even deprecate) this property.

This property really means "treat an empty string the same as null" (which would then either be rejected or accepted based on requires-reply).

Maybe instead of turning it into a no-op, we should retain the functionality with a warning that it will be removed later.

Or, maybe rename it "empty-result-is-null" ???

cc @markfisher since he raised the original question

@markfisher
markfisher Aug 16, 2013 Member

Sorry; I probably caused the confusion with my initial comment. Looking at it now, I think we might want to keep both 'ignore-emtpy-responses' and 'requires-reply', because there may be cases where the reply Message has an empty String (but that is non-null and therefore different) and that is to be treated as a valid reply. I believe that is why we have that boolean in the first place, based on the original request:
https://jira.springsource.org/browse/INT-630

@garyrussell
garyrussell Aug 17, 2013 Member

I reverted the ignore-empty-responses and pushed a commit here (also with some doc polishing).

garyrussell@a937ca8

@artembilan
artembilan Aug 19, 2013 Member

Hi there!

Thanks, guys, for your review.
IMO it's OK now. However when I have removed this property I meant that the same "empty string reply" we may have and with many other outbound gateways. So, configure downstream filter is more flexible solution in any case. However now removing ignore-empty-responses should be within separate JIRA. If we decide to do it, of course.

@garyrussell
Member

Merged.

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