INT-2938 Filter - Provide Option for Discard #760

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Member

garyrussell commented Mar 5, 2013

Provide an option to determine whether or not the discard
(and exception throwing if configured) occurs within the
advice chain (default), or after it completes.

INT-2938 Filter Discard Advice Option - Schema

Schema, parser, tests for option on filter element.

@artembilan artembilan commented on an outdated diff Apr 3, 2013

...integration/handler/PostProcessingMessageHandler.java
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.integration.handler;
+
+import org.springframework.integration.Message;
+
+
+/**
+ * Implementations of this interface are a subclass of
@artembilan

artembilan Apr 3, 2013

Member

are subclasses of ?

@artembilan artembilan commented on an outdated diff Apr 3, 2013

...integration/handler/PostProcessingMessageHandler.java
+
+/**
+ * Implementations of this interface are a subclass of
+ * {@link AbstractMessageHandler} that performs post processing after the
+ * {@link AbstractMessageHandler#handleMessageInternal(org.springframework.integration.Message)}
+ * call.
+ *
+ * @author Gary Russell
+ * @since 3.0
+ *
+ */
+public interface PostProcessingMessageHandler {
+
+ /**
+ * Take some further action on the result and/or message.
+ * @param result The result from handleMessageinternal().
@artembilan

artembilan Apr 3, 2013

Member

{@link AbstractMessageHandler#handleMessageInternal}

@artembilan artembilan commented on an outdated diff Apr 3, 2013

...stractReplyProducingPostProcessingMessageHandler.java
+
+ private volatile boolean postProcessWithinAdvice = true;
+
+ public void setPostProcessWithinAdvice(boolean postProcessWithinAdvice) {
+ this.postProcessWithinAdvice = postProcessWithinAdvice;
+ }
+
+ /**
+ * Returns true if the post processing should occur within
+ * the scope of any configured advice classes. If false, the
+ * post processing will occur after the advice chain returns. Default true.
+ * @return true if post processing should be within any advice or if
+ * there is no advice chain.
+ */
+ protected boolean isPostProcessWithinAdviceIfPresent() {
+ return this.hasAdviceChain() ? this.postProcessWithinAdvice : true;
@artembilan

artembilan Apr 3, 2013

Member

return !this.hasAdviceChain() || this.postProcessWithinAdvice;

@artembilan artembilan commented on an outdated diff Apr 3, 2013

...stractReplyProducingPostProcessingMessageHandler.java
+ this.postProcessWithinAdvice = postProcessWithinAdvice;
+ }
+
+ /**
+ * Returns true if the post processing should occur within
+ * the scope of any configured advice classes. If false, the
+ * post processing will occur after the advice chain returns. Default true.
+ * @return true if post processing should be within any advice or if
+ * there is no advice chain.
+ */
+ protected boolean isPostProcessWithinAdviceIfPresent() {
+ return this.hasAdviceChain() ? this.postProcessWithinAdvice : true;
+ }
+
+ @Override
+ protected Object handleRequestMessage(Message<?> requestMessage) {
@artembilan

artembilan Apr 3, 2013

Member

Should be final as and doInvokeAdvisedRequestHandler too.

In general postProcess implementation now looks not so clear and and very stiffly:
doInvokeAdvisedRequestHandler, in the end, delegates to this handleRequestMessage. So check isPostProcessWithinAdviceIfPresent() twice looks like redundant. Unfortunately, I don't see more flexible solution yet :-(.
Maybe through a ThreadLocal flag?

@artembilan artembilan and 1 other commented on an outdated diff Apr 3, 2013

...ork/integration/config/xml/spring-integration-3.0.xsd
@@ -2377,7 +2377,26 @@
<xsd:complexType name="filter-type">
<xsd:complexContent>
- <xsd:extension base="expressionOrInnerEndpointDefinitionAware">
+ <xsd:extension base="expressionOrInnerEndpointDefinitionAwareNoAdviceChain">
+ <xsd:sequence>
+ <xsd:element name="request-handler-advice-chain" minOccurs="0" maxOccurs="1">
+ <xsd:complexType>
+ <xsd:complexContent>
+ <xsd:extension base="adviceChainType">
+ <xsd:attribute name="advise-discard" type="xsd:string" default="true">
@artembilan

artembilan Apr 3, 2013

Member

English is not my best ability, but in Russian it sound like:
"I advise you to discard from it" :-). So, maybe name it as a property of MessageFilter discard-within-advice ?

@ghillert

ghillert Apr 4, 2013

Member

As a German speaker, I would also prefer discard-within-advice, especially also since it would better align with the setter discardWithinAdvice.

Member

garyrussell commented Apr 4, 2013

Pushed PR Review fixes + Dockbook update.

Member

artembilan commented Apr 5, 2013

If we decide to leave AbstractReplyProducingPostProcessingMessageHandler as Gary has done, then LGTM!
My apps look forward to meet this fix ;-)

@artembilan artembilan commented on the diff Apr 5, 2013

src/reference/docbook/handler-advice.xml
+ <section id="advising-filters">
+ <title>Advising Filters</title>
+ <para>
+ There is an additional consideration when advising <classname>Filter</classname>s. By default, any discard
+ actions (when the filter returns false) are performed <emphasis>within</emphasis> the scope of the
+ advice chain. This could include all the flow downstream of the <emphasis>discard channel</emphasis>.
+ So, for example if an element downstream of the <emphasis>discard-channel</emphasis> throws an exception,
+ and there is a retry advice, the process will be retried. This is also the case if
+ <emphasis>throwExceptionOnRejection</emphasis> is set to true (the exception is thrown within the
+ scope of the advice).
+ </para>
+ <para>
+ Setting <emphasis>discard-within-advice</emphasis> to "false" modifies this behavior and the discard
+ (or exception) occurs after the advice chain is called.
+ </para>
+ </section>
@artembilan

artembilan Apr 5, 2013

Member

Should it be presented in "What's new" ?

Member

garyrussell commented Apr 5, 2013

Do you have an alternative solution to AbstractReplyProducingPostProcessingMessageHandler ???

Member

artembilan commented Apr 8, 2013

Here I thought more about KISS & DRY and maybe it can be implemented via some PostProcessCallback, but from other side it is an internal final solution and it's not a public API. So, we can leave it as is: there are a lot of more bad places in the project.
IMO - LGTM

@markfisher markfisher commented on an outdated diff Apr 24, 2013

...springframework/integration/filter/MessageFilter.java
@@ -82,6 +83,15 @@ public void setDiscardChannel(MessageChannel discardChannel) {
this.discardChannel = discardChannel;
}
+ /**
+ * Set to 'true' if you wish the discard processing to occur within of any
@markfisher

markfisher Apr 24, 2013

Member

typo: "within of any"

garyrussell added some commits Feb 21, 2013

@garyrussell @garyrussell garyrussell INT-2938 Filter - Provide Option for Discard
Provide an option to determine whether or not the discard
(and exception throwing if configured) occurs within the
advice chain (default), or after it completes.

INT-2938 Filter Discard Advice Option - Schema

Schema, parser, tests for option on filter element.
bffdd82
@garyrussell @garyrussell garyrussell INT-2938 Polishing - PR Comments + Doc f494dd2
@garyrussell @garyrussell garyrussell INT-2938 Add to "What's New" 275e265
@garyrussell garyrussell INT-2938 Polishing - Javadoc Typo 2e68b93

@markfisher markfisher commented on the diff May 23, 2013

...stractReplyProducingPostProcessingMessageHandler.java
+ implements PostProcessingMessageHandler {
+
+ private volatile boolean postProcessWithinAdvice = true;
+
+ public void setPostProcessWithinAdvice(boolean postProcessWithinAdvice) {
+ this.postProcessWithinAdvice = postProcessWithinAdvice;
+ }
+
+ /**
+ * Returns true if the post processing should occur within
+ * the scope of any configured advice classes. If false, the
+ * post processing will occur after the advice chain returns. Default true.
+ * @return true if post processing should be within any advice or if
+ * there is no advice chain.
+ */
+ protected boolean isPostProcessWithinAdviceIfPresent() {
@markfisher

markfisher May 23, 2013

Member

As discussed offline, while merging I will remove this method and instead rely upon just the boolean instance var. I will move the javadoc to the setter method for that var, and I will add a comment that it's "only applicable if an advice chain is present'". Finally, that will also require moving the hasAdviceChain() call into the if condition within handleRequestMessage().

Member

markfisher commented May 23, 2013

LGTM; merging

markfisher closed this May 23, 2013

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