[jira] [Comment Edited] (CAMEL-10340) camel-aws - SQS option deleteAfterRead not work if set deleteIfFiltered=false

["Jon Woodforth (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CAMEL-10340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15689471#comment-15689471 ] 

Jon Woodforth edited comment on CAMEL-10340 at 11/23/16 9:14 AM:
-----------------------------------------------------------------

Hi,

As per comments on the commit here:  https://github.com/apache/camel/commit/50dc4ea14c0ad6d3d93cfe4ddb386a006d1fafcc

This change has changed the behaviour of our routes and deviates from the documentation. Can I ask if this is what was intended? ...this doesn't seem to match what was in the Camel-SQS documentation.

The code before was:

{code}
    private boolean shouldDelete(Exchange exchange) {
        return getConfiguration().isDeleteAfterRead()
                && (getConfiguration().isDeleteIfFiltered()
                    || (!getConfiguration().isDeleteIfFiltered()
                        && passedThroughFilter(exchange)));
    }
{code}

Which matched the documentation:

deleteAfterRead: Delete message from SQS after it has been read (and processed by the route).
deleteIfFiltered: If false and exchange does not make it through a Camel filter upstream in the route, then don't send DeleteMessage.

With the above code, deleteAfterRead needed to be set to "true" for ANY message to be deleted, filtered or not. Then, it was up to the configuration of deleteIfFiltered - if this was set to false then as the documentation states the message would not be deleted if if didn't pass through the filter.

So, our config of:
deleteAfterRead = true (default)
deleteIfFiltered = false

...gave the right outcome. Messages that were filtered "out" (i.e. not allowed through the filter) were not deleted from SQS.

However, the new code commit has changed this behaviour.

Firstly:

{code}
return getConfiguration().isDeleteAfterRead() && (...filteringLogic...)
{code}

..has changed to:

{code}
return getConfiguration().isDeleteAfterRead() || shouldDeleteByFilter;
{code}

.. an && becoming an || which means for the filtering logic to take effect the deleteAfterRead must not be set to false.  This isn't covered in the documentation and is a change of behaviour from before.

Secondly:

{code}
|| (!getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange)));
{code}

..has changed to:

{code}
... && getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange);
{code}

.. this time the negation has been removed, so the logic has now changed to not looking at whether the message was filtered out, but whether is was allowed through the filter.

With the new code that has been committed, to get the same behaviour as before you now have to set deleteAfterRead to "false" and deletedIfFiltered to "true" so that messages filtered out are not deleted, and those that pass the filter are.

New config:
deleteAfterRead = false
deleteIfFiltered = true

Is this what was intended? Seems to be a departure from the documentation? If so is the documentation going to be updated? ..or should the logic be put back how it was?

I believe, reading the posts above, that the code worked the way it was; if deleteAfterRead was set to true and deleteIfFiltered was set to false then the scenarios above would have worked - messages that did not pass the filter would not have been deleted; those that did pass the filter would have been.  I don't see why the code was changed!


was (Author: jonw9):
Hi,

As per comments on the commit here:  https://github.com/apache/camel/commit/50dc4ea14c0ad6d3d93cfe4ddb386a006d1fafcc

This change has changed the behaviour of our routes and deviates from the documentation. Can I ask if this is what was intended? ...this doesn't seem to match what was in the Camel-SQS documentation.

The code before was:

    private boolean shouldDelete(Exchange exchange) {
        return getConfiguration().isDeleteAfterRead()
                && (getConfiguration().isDeleteIfFiltered()
                    || (!getConfiguration().isDeleteIfFiltered()
                        && passedThroughFilter(exchange)));
    }

Which matched the documentation:

deleteAfterRead: Delete message from SQS after it has been read (and processed by the route).
deleteIfFiltered: If false and exchange does not make it through a Camel filter upstream in the route, then don't send DeleteMessage.

With the above code, deleteAfterRead needed to be set to "true" for ANY message to be deleted, filtered or not. Then, it was up to the configuration of deleteIfFiltered - if this was set to false then as the documentation states the message would not be deleted if if didn't pass through the filter.

So, our config of:
deleteAfterRead = true (default)
deleteIfFiltered = false

...gave the right outcome. Messages that were filtered "out" (i.e. not allowed through the filter) were not deleted from SQS.

However, the new code commit has changed this behaviour.

Firstly:

{code}
return getConfiguration().isDeleteAfterRead() && (...filteringLogic...)
{code}

..has changed to:

{code}
return getConfiguration().isDeleteAfterRead() || shouldDeleteByFilter;
{code}

.. an && becoming an || which means for the filtering logic to take effect the deleteAfterRead must not be set to false.  This isn't covered in the documentation and is a change of behaviour from before.

Secondly:

{code}
|| (!getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange)));
{code}

..has changed to:

{code}
... && getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange);
{code}

.. this time the negation has been removed, so the logic has now changed to not looking at whether the message was filtered out, but whether is was allowed through the filter.

With the new code that has been committed, to get the same behaviour as before you now have to set deleteAfterRead to "false" and deletedIfFiltered to "true" so that messages filtered out are not deleted, and those that pass the filter are.

New config:
deleteAfterRead = false
deleteIfFiltered = true

Is this what was intended? Seems to be a departure from the documentation? If so is the documentation going to be updated? ..or should the logic be put back how it was?

I believe, reading the posts above, that the code worked the way it was; if deleteAfterRead was set to true and deleteIfFiltered was set to false then the scenarios above would have worked - messages that did not pass the filter would not have been deleted; those that did pass the filter would have been.  I don't see why the code was changed!

> camel-aws - SQS option deleteAfterRead not work if set deleteIfFiltered=false
> -----------------------------------------------------------------------------
>
>                 Key: CAMEL-10340
>                 URL: https://issues.apache.org/jira/browse/CAMEL-10340
>             Project: Camel
>          Issue Type: Bug
>          Components: camel-aws
>    Affects Versions: 2.17.3
>            Reporter: Yi Yan
>            Assignee: Andrea Cosentino
>            Priority: Minor
>             Fix For: 2.18.0, 2.17.4
>
>         Attachments: SqsConsumerDeleteTest.java
>
>
> I'm using aws-sqs 2.17.3, if I set deleteAfterRead=true and deleteIfFiltered=false in my DSL, the message will not be deleted. If I want to delete the message after read it, I have to set deleteAfterRead and deleteIfFiltered both with true when I use the two options in one DSL, but in fact there is no filter in my route, the message should be removed whatever the deleteIfFiltered option set to ture or false.
> {code:title=SqsConsumerDeleteTest.java|borderStyle=solid}
> from("aws-sqs:my-quque"
>     + "?amazonSQSClient=#conn_cAWSConnection_1"
>     + "&deleteAfterRead=" + true + "&deleteIfFiltered="
>     + false).to("log:qs_route.cLog_1" + "?level=DEBUG").to("mock:mock_1");
> {code}
> I attached my test file, after run the test method, the sqs message still exists in the sqs queue after 30 seconds.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)